* [PATCH v3 1/6] Import idx.h from gnulib
2020-12-29 19:34 [PATCH v3 0/6] Multiple fixes to realpath Adhemerval Zanella
@ 2020-12-29 19:34 ` Adhemerval Zanella
2020-12-29 19:34 ` [PATCH v3 2/6] Import filename.h " Adhemerval Zanella
` (4 subsequent siblings)
5 siblings, 0 replies; 18+ messages in thread
From: Adhemerval Zanella @ 2020-12-29 19:34 UTC (permalink / raw)
To: libc-alpha, Paul Eggert; +Cc: bug-gnulib
And use to simplify stdlib/canonicalize.c implementation.
---
include/idx.h | 114 ++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 114 insertions(+)
create mode 100644 include/idx.h
diff --git a/include/idx.h b/include/idx.h
new file mode 100644
index 0000000000..024b44ae98
--- /dev/null
+++ b/include/idx.h
@@ -0,0 +1,114 @@
+/* A type for indices and sizes.
+ Copyright (C) 2020 Free Software Foundation, Inc.
+ This file is part of the GNU C Library.
+
+ The GNU C Library is free software; you can redistribute it and/or
+ modify it under the terms of the GNU Lesser General Public
+ License as published by the Free Software Foundation; either
+ version 2.1 of the License, or (at your option) any later version.
+
+ The GNU C Library is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public
+ License along with the GNU C Library; if not, see
+ <https://www.gnu.org/licenses/>. */
+
+#ifndef _IDX_H
+#define _IDX_H
+
+/* Get ptrdiff_t. */
+#include <stddef.h>
+
+/* Get PTRDIFF_MAX. */
+#include <stdint.h>
+
+/* The type 'idx_t' holds an (array) index or an (object) size.
+ Its implementation promotes to a signed integer type,
+ which can hold the values
+ 0..2^63-1 (on 64-bit platforms) or
+ 0..2^31-1 (on 32-bit platforms).
+
+ Why a signed integer type?
+
+ * Security: Signed types can be checked for overflow via
+ '-fsanitize=undefined', but unsigned types cannot.
+
+ * Comparisons without surprises: ISO C99 § 6.3.1.8 specifies a few
+ surprising results for comparisons, such as
+
+ (int) -3 < (unsigned long) 7 => false
+ (int) -3 < (unsigned int) 7 => false
+ and on 32-bit machines:
+ (long) -3 < (unsigned int) 7 => false
+
+ This is surprising because the natural comparison order is by
+ value in the realm of infinite-precision signed integers (ℤ).
+
+ The best way to get rid of such surprises is to use signed types
+ for numerical integer values, and use unsigned types only for
+ bit masks and enums.
+
+ Why not use 'size_t' directly?
+
+ * Because 'size_t' is an unsigned type, and a signed type is better.
+ See above.
+
+ Why not use 'ptrdiff_t' directly?
+
+ * Maintainability: When reading and modifying code, it helps to know that
+ a certain variable cannot have negative values. For example, when you
+ have a loop
+
+ int n = ...;
+ for (int i = 0; i < n; i++) ...
+
+ or
+
+ ptrdiff_t n = ...;
+ for (ptrdiff_t i = 0; i < n; i++) ...
+
+ you have to ask yourself "what if n < 0?". Whereas in
+
+ idx_t n = ...;
+ for (idx_t i = 0; i < n; i++) ...
+
+ you know that this case cannot happen.
+
+ Similarly, when a programmer writes
+
+ idx_t = ptr2 - ptr1;
+
+ there is an implied assertion that ptr1 and ptr2 point into the same
+ object and that ptr1 <= ptr2.
+
+ * Being future-proof: In the future, range types (integers which are
+ constrained to a certain range of values) may be added to C compilers
+ or to the C standard. Several programming languages (Ada, Haskell,
+ Common Lisp, Pascal) already have range types. Such range types may
+ help producing good code and good warnings. The type 'idx_t' could
+ then be typedef'ed to a range type that is signed after promotion. */
+
+/* In the future, idx_t could be typedef'ed to a signed range type.
+ The clang "extended integer types", supported in Clang 11 or newer
+ <https://clang.llvm.org/docs/LanguageExtensions.html#extended-integer-types>,
+ are a special case of range types. However, these types don't support binary
+ operators with plain integer types (e.g. expressions such as x > 1).
+ Therefore, they don't behave like signed types (and not like unsigned types
+ either). So, we cannot use them here. */
+
+/* Use the signed type 'ptrdiff_t'. */
+/* Note: ISO C does not mandate that 'size_t' and 'ptrdiff_t' have the same
+ size, but it is so on all platforms we have seen since 1990. */
+typedef ptrdiff_t idx_t;
+
+/* IDX_MAX is the maximum value of an idx_t. */
+#define IDX_MAX PTRDIFF_MAX
+
+/* So far no need has been found for an IDX_WIDTH macro.
+ Perhaps there should be another macro IDX_VALUE_BITS that does not
+ count the sign bit and is therefore one less than PTRDIFF_WIDTH. */
+
+#endif /* _IDX_H */
--
2.25.1
^ permalink raw reply [flat|nested] 18+ messages in thread* [PATCH v3 2/6] Import filename.h from gnulib
2020-12-29 19:34 [PATCH v3 0/6] Multiple fixes to realpath Adhemerval Zanella
2020-12-29 19:34 ` [PATCH v3 1/6] Import idx.h from gnulib Adhemerval Zanella
@ 2020-12-29 19:34 ` Adhemerval Zanella
2020-12-29 19:34 ` [PATCH v3 3/6] malloc: Add scratch_buffer_dupfree Adhemerval Zanella
` (3 subsequent siblings)
5 siblings, 0 replies; 18+ messages in thread
From: Adhemerval Zanella @ 2020-12-29 19:34 UTC (permalink / raw)
To: libc-alpha, Paul Eggert; +Cc: bug-gnulib
And use to simplify stdlib/canonicalize.c implementation.
---
include/filename.h | 110 +++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 110 insertions(+)
create mode 100644 include/filename.h
diff --git a/include/filename.h b/include/filename.h
new file mode 100644
index 0000000000..4598fb1d63
--- /dev/null
+++ b/include/filename.h
@@ -0,0 +1,110 @@
+/* Basic filename support macros.
+ Copyright (C) 2001-2004, 2007-2020 Free Software Foundation, Inc.
+
+ This program is free software: you can redistribute it and/or modify
+ it under the terms of the GNU General Public License as published by
+ the Free Software Foundation; either version 3 of the License, or
+ (at your option) any later version.
+
+ This program is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ GNU General Public License for more details.
+
+ You should have received a copy of the GNU General Public License
+ along with this program. If not, see <https://www.gnu.org/licenses/>. */
+
+/* From Paul Eggert and Jim Meyering. */
+
+#ifndef _FILENAME_H
+#define _FILENAME_H
+
+#include <string.h>
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+
+/* Filename support.
+ ISSLASH(C) tests whether C is a directory separator
+ character.
+ HAS_DEVICE(Filename) tests whether Filename contains a device
+ specification.
+ FILE_SYSTEM_PREFIX_LEN(Filename) length of the device specification
+ at the beginning of Filename,
+ index of the part consisting of
+ alternating components and slashes.
+ FILE_SYSTEM_DRIVE_PREFIX_CAN_BE_RELATIVE
+ 1 when a non-empty device specification
+ can be followed by an empty or relative
+ part,
+ 0 when a non-empty device specification
+ must be followed by a slash,
+ 0 when device specification don't exist.
+ IS_ABSOLUTE_FILE_NAME(Filename)
+ tests whether Filename is independent of
+ any notion of "current directory".
+ IS_RELATIVE_FILE_NAME(Filename)
+ tests whether Filename may be concatenated
+ to a directory filename.
+ Note: On native Windows, OS/2, DOS, "c:" is neither an absolute nor a
+ relative file name!
+ IS_FILE_NAME_WITH_DIR(Filename) tests whether Filename contains a device
+ or directory specification.
+ */
+#if defined _WIN32 || defined __CYGWIN__ \
+ || defined __EMX__ || defined __MSDOS__ || defined __DJGPP__
+ /* Native Windows, Cygwin, OS/2, DOS */
+# define ISSLASH(C) ((C) == '/' || (C) == '\\')
+ /* Internal macro: Tests whether a character is a drive letter. */
+# define _IS_DRIVE_LETTER(C) \
+ (((C) >= 'A' && (C) <= 'Z') || ((C) >= 'a' && (C) <= 'z'))
+ /* Help the compiler optimizing it. This assumes ASCII. */
+# undef _IS_DRIVE_LETTER
+# define _IS_DRIVE_LETTER(C) \
+ (((unsigned int) (C) | ('a' - 'A')) - 'a' <= 'z' - 'a')
+# define HAS_DEVICE(Filename) \
+ (_IS_DRIVE_LETTER ((Filename)[0]) && (Filename)[1] == ':')
+# define FILE_SYSTEM_PREFIX_LEN(Filename) (HAS_DEVICE (Filename) ? 2 : 0)
+# ifdef __CYGWIN__
+# define FILE_SYSTEM_DRIVE_PREFIX_CAN_BE_RELATIVE 0
+# else
+ /* On native Windows, OS/2, DOS, the system has the notion of a
+ "current directory" on each drive. */
+# define FILE_SYSTEM_DRIVE_PREFIX_CAN_BE_RELATIVE 1
+# endif
+# if FILE_SYSTEM_DRIVE_PREFIX_CAN_BE_RELATIVE
+# define IS_ABSOLUTE_FILE_NAME(Filename) \
+ ISSLASH ((Filename)[FILE_SYSTEM_PREFIX_LEN (Filename)])
+# else
+# define IS_ABSOLUTE_FILE_NAME(Filename) \
+ (ISSLASH ((Filename)[0]) || HAS_DEVICE (Filename))
+# endif
+# define IS_RELATIVE_FILE_NAME(Filename) \
+ (! (ISSLASH ((Filename)[0]) || HAS_DEVICE (Filename)))
+# define IS_FILE_NAME_WITH_DIR(Filename) \
+ (strchr ((Filename), '/') != NULL || strchr ((Filename), '\\') != NULL \
+ || HAS_DEVICE (Filename))
+#else
+ /* Unix */
+# define ISSLASH(C) ((C) == '/')
+# define HAS_DEVICE(Filename) ((void) (Filename), 0)
+# define FILE_SYSTEM_PREFIX_LEN(Filename) ((void) (Filename), 0)
+# define FILE_SYSTEM_DRIVE_PREFIX_CAN_BE_RELATIVE 0
+# define IS_ABSOLUTE_FILE_NAME(Filename) ISSLASH ((Filename)[0])
+# define IS_RELATIVE_FILE_NAME(Filename) (! ISSLASH ((Filename)[0]))
+# define IS_FILE_NAME_WITH_DIR(Filename) (strchr ((Filename), '/') != NULL)
+#endif
+
+/* Deprecated macros. For backward compatibility with old users of the
+ 'filename' module. */
+#define IS_ABSOLUTE_PATH IS_ABSOLUTE_FILE_NAME
+#define IS_PATH_WITH_DIR IS_FILE_NAME_WITH_DIR
+
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif /* _FILENAME_H */
--
2.25.1
^ permalink raw reply [flat|nested] 18+ messages in thread* [PATCH v3 3/6] malloc: Add scratch_buffer_dupfree
2020-12-29 19:34 [PATCH v3 0/6] Multiple fixes to realpath Adhemerval Zanella
2020-12-29 19:34 ` [PATCH v3 1/6] Import idx.h from gnulib Adhemerval Zanella
2020-12-29 19:34 ` [PATCH v3 2/6] Import filename.h " Adhemerval Zanella
@ 2020-12-29 19:34 ` Adhemerval Zanella
2020-12-29 19:34 ` [PATCH v3 4/6] stdlib: Sync canonicalize with gnulib [BZ #10635] [BZ #26592] [BZ #26341] [BZ #24970] Adhemerval Zanella
` (2 subsequent siblings)
5 siblings, 0 replies; 18+ messages in thread
From: Adhemerval Zanella @ 2020-12-29 19:34 UTC (permalink / raw)
To: libc-alpha, Paul Eggert; +Cc: bug-gnulib
It returns a copy of the buffer up to a defined size. It will be used
on realpath sync with gnulib.
---
include/scratch_buffer.h | 16 +++++++++++++
malloc/Makefile | 1 +
malloc/Versions | 1 +
malloc/scratch_buffer_dupfree.c | 41 +++++++++++++++++++++++++++++++++
malloc/tst-scratch_buffer.c | 26 +++++++++++++++++++--
5 files changed, 83 insertions(+), 2 deletions(-)
create mode 100644 malloc/scratch_buffer_dupfree.c
diff --git a/include/scratch_buffer.h b/include/scratch_buffer.h
index c39da78629..48d651b41a 100644
--- a/include/scratch_buffer.h
+++ b/include/scratch_buffer.h
@@ -132,4 +132,20 @@ scratch_buffer_set_array_size (struct scratch_buffer *buffer,
(buffer, nelem, size));
}
+/* Return a copy of *BUFFER's first SIZE bytes as a heap-allocated block,
+ deallocating *BUFFER if it was heap-allocated. SIZE must be at
+ most *BUFFER's size. Return NULL (setting errno) on memory
+ exhaustion. */
+void *__libc_scratch_buffer_dupfree (struct scratch_buffer *buffer,
+ size_t size);
+libc_hidden_proto (__libc_scratch_buffer_dupfree)
+
+/* Alias for __libc_scratch_dupfree. */
+static __always_inline void *
+scratch_buffer_dupfree (struct scratch_buffer *buffer, size_t size)
+{
+ void *r = __libc_scratch_buffer_dupfree (buffer, size);
+ return __glibc_likely (r != NULL) ? r : NULL;
+}
+
#endif /* _SCRATCH_BUFFER_H */
diff --git a/malloc/Makefile b/malloc/Makefile
index 39ffaa7161..ef70b547f9 100644
--- a/malloc/Makefile
+++ b/malloc/Makefile
@@ -74,6 +74,7 @@ tests-exclude-mcheck = tst-mcheck tst-malloc-usable \
tests-mcheck = $(filter-out $(tests-exclude-mcheck),$(tests))
routines = malloc morecore mcheck mtrace obstack reallocarray \
+ scratch_buffer_dupfree \
scratch_buffer_grow scratch_buffer_grow_preserve \
scratch_buffer_set_array_size \
dynarray_at_failure \
diff --git a/malloc/Versions b/malloc/Versions
index 94c8ba8040..6693c46ee2 100644
--- a/malloc/Versions
+++ b/malloc/Versions
@@ -75,6 +75,7 @@ libc {
__libc_thread_freeres;
# struct scratch_buffer support
+ __libc_scratch_buffer_dupfree;
__libc_scratch_buffer_grow;
__libc_scratch_buffer_grow_preserve;
__libc_scratch_buffer_set_array_size;
diff --git a/malloc/scratch_buffer_dupfree.c b/malloc/scratch_buffer_dupfree.c
new file mode 100644
index 0000000000..5561e99b0a
--- /dev/null
+++ b/malloc/scratch_buffer_dupfree.c
@@ -0,0 +1,41 @@
+/* Variable-sized buffer with on-stack default allocation.
+ Copyright (C) 2020 Free Software Foundation, Inc.
+ This file is part of the GNU C Library.
+
+ The GNU C Library is free software; you can redistribute it and/or
+ modify it under the terms of the GNU Lesser General Public
+ License as published by the Free Software Foundation; either
+ version 2.1 of the License, or (at your option) any later version.
+
+ The GNU C Library is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public
+ License along with the GNU C Library; if not, see
+ <https://www.gnu.org/licenses/>. */
+
+#ifndef _LIBC
+# include <libc-config.h>
+#endif
+
+#include <scratch_buffer.h>
+#include <string.h>
+
+void *
+__libc_scratch_buffer_dupfree (struct scratch_buffer *buffer, size_t size)
+{
+ void *data = buffer->data;
+ if (data == buffer->__space.__c)
+ {
+ void *copy = malloc (size);
+ return copy != NULL ? memcpy (copy, data, size) : NULL;
+ }
+ else
+ {
+ void *copy = realloc (data, size);
+ return copy != NULL ? copy : data;
+ }
+}
+libc_hidden_def (__libc_scratch_buffer_dupfree)
diff --git a/malloc/tst-scratch_buffer.c b/malloc/tst-scratch_buffer.c
index ef5fb0a8eb..6008113174 100644
--- a/malloc/tst-scratch_buffer.c
+++ b/malloc/tst-scratch_buffer.c
@@ -16,7 +16,10 @@
License along with the GNU C Library; if not, see
<https://www.gnu.org/licenses/>. */
+#include <array_length.h>
#include <scratch_buffer.h>
+#include <support/check.h>
+#include <support/support.h>
#include <stdbool.h>
#include <stdio.h>
#include <string.h>
@@ -148,8 +151,27 @@ do_test (void)
&& array_size_must_fail (4, ((size_t)-1) / 4)))
return 1;
}
+ {
+ struct scratch_buffer buf;
+ scratch_buffer_init (&buf);
+ memset (buf.data, '@', buf.length);
+
+ size_t sizes[] = { 16, buf.length, buf.length + 16 };
+ for (int i = 0; i < array_length (sizes); i++)
+ {
+ /* The extra size is unitialized through realloc. */
+ size_t l = sizes[i] > buf.length ? sizes[i] : buf.length;
+ void *r = scratch_buffer_dupfree (&buf, l);
+ void *c = xmalloc (l);
+ memset (c, '@', l);
+ TEST_COMPARE_BLOB (r, l, buf.data, l);
+ free (r);
+ free (c);
+ }
+
+ scratch_buffer_free (&buf);
+ }
return 0;
}
-#define TEST_FUNCTION do_test ()
-#include "../test-skeleton.c"
+#include <support/test-driver.c>
--
2.25.1
^ permalink raw reply [flat|nested] 18+ messages in thread* [PATCH v3 4/6] stdlib: Sync canonicalize with gnulib [BZ #10635] [BZ #26592] [BZ #26341] [BZ #24970]
2020-12-29 19:34 [PATCH v3 0/6] Multiple fixes to realpath Adhemerval Zanella
` (2 preceding siblings ...)
2020-12-29 19:34 ` [PATCH v3 3/6] malloc: Add scratch_buffer_dupfree Adhemerval Zanella
@ 2020-12-29 19:34 ` Adhemerval Zanella
2020-12-30 1:21 ` Paul Eggert
2020-12-29 19:34 ` [PATCH v3 5/6] support: Add support_small_thread_stack_size Adhemerval Zanella
2020-12-29 19:34 ` [PATCH v3 6/6] stdlib: Add testcase fro BZ #26241 Adhemerval Zanella
5 siblings, 1 reply; 18+ messages in thread
From: Adhemerval Zanella @ 2020-12-29 19:34 UTC (permalink / raw)
To: libc-alpha, Paul Eggert; +Cc: bug-gnulib
It sync with gnulib version b29d62dfa with the following change:
--
diff --git a/stdlib/canonicalize.c b/stdlib/canonicalize.c
index 04fe95253f..c8f085b779 100644
--- a/stdlib/canonicalize.c
+++ b/stdlib/canonicalize.c
@@ -41,6 +41,7 @@
#include <filename.h>
#include <idx.h>
#include <scratch_buffer.h>
+#include <intprops.h>
#ifdef _LIBC
# include <shlib-compat.h>
@@ -339,6 +340,11 @@ realpath_stk (const char *name, char *resolved,
if (end_in_extra_buffer)
end_idx = end - extra_buf;
idx_t len = strlen (end);
+ if (INT_ADD_OVERFLOW (len, n))
+ {
+ __set_errno (ENAMETOOLONG);
+ goto error_nomem;
+ }
while (extra_buffer.length <= len + n)
{
if (!scratch_buffer_grow_preserve (&extra_buffer))
--
It is required to avoid stdlib/test-bz22786 regression, where it passes
string larger than PTRDIFF_T as the input argument. Althought it uses
a pointer larger than the one malloc would return (BZ#23741), it is
still a semantic support for glibc and ENAMETOOLONG should be returned.
The patch also fixes multiple realpath issues:
- Portability fixes for errno clobbering on free (BZ#10635). The
function does not call free directly anymore, although it might be
done through scratch_buffer_free. The free errno clobbering is
being tracked by BZ#17924.
- Pointer arithmetic overflows in realpath (BZ#26592).
- Realpath cyclically call __alloca(path_max) to consume too much
stack space (BZ#26341).
- Realpath mishandles EOVERFLOW; stat not needed anyway (BZ#24970).
The check is done through faccessat now.
Checked on x86_64-linux-gnu.
---
stdlib/canonicalize.c | 547 +++++++++++++++++++---------
sysdeps/unix/sysv/linux/faccessat.c | 3 +-
2 files changed, 386 insertions(+), 164 deletions(-)
diff --git a/stdlib/canonicalize.c b/stdlib/canonicalize.c
index 3fcb399a5d..c8f085b779 100644
--- a/stdlib/canonicalize.c
+++ b/stdlib/canonicalize.c
@@ -16,43 +16,200 @@
License along with the GNU C Library; if not, see
<https://www.gnu.org/licenses/>. */
-#include <assert.h>
+#ifndef _LIBC
+/* Don't use __attribute__ __nonnull__ in this compilation unit. Otherwise gcc
+ optimizes away the name == NULL test below. */
+# define _GL_ARG_NONNULL(params)
+
+# define _GL_USE_STDLIB_ALLOC 1
+# include <libc-config.h>
+#endif
+
+/* Specification. */
#include <stdlib.h>
-#include <string.h>
-#include <unistd.h>
-#include <limits.h>
-#include <sys/stat.h>
+
#include <errno.h>
+#include <fcntl.h>
+#include <limits.h>
+#include <stdbool.h>
#include <stddef.h>
+#include <string.h>
+#include <sys/stat.h>
+#include <unistd.h>
#include <eloop-threshold.h>
-#include <shlib-compat.h>
+#include <filename.h>
+#include <idx.h>
+#include <scratch_buffer.h>
+#include <intprops.h>
+
+#ifdef _LIBC
+# include <shlib-compat.h>
+# define GCC_LINT 1
+# define _GL_ATTRIBUTE_PURE __attribute__ ((__pure__))
+#else
+# define __canonicalize_file_name canonicalize_file_name
+# define __realpath realpath
+# include "pathmax.h"
+# define __faccessat faccessat
+# if defined _WIN32 && !defined __CYGWIN__
+# define __getcwd _getcwd
+# elif HAVE_GETCWD
+# if IN_RELOCWRAPPER
+ /* When building the relocatable program wrapper, use the system's getcwd
+ function, not the gnulib override, otherwise we would get a link error.
+ */
+# undef getcwd
+# endif
+# if defined VMS && !defined getcwd
+ /* We want the directory in Unix syntax, not in VMS syntax.
+ The gnulib override of 'getcwd' takes 2 arguments; the original VMS
+ 'getcwd' takes 3 arguments. */
+# define __getcwd(buf, max) getcwd (buf, max, 0)
+# else
+# define __getcwd getcwd
+# endif
+# else
+# define __getcwd(buf, max) getwd (buf)
+# endif
+# define __mempcpy mempcpy
+# define __pathconf pathconf
+# define __rawmemchr rawmemchr
+# define __readlink readlink
+# define __stat stat
+#endif
-/* Return the canonical absolute name of file NAME. A canonical name
- does not contain any `.', `..' components nor any repeated path
- separators ('/') or symlinks. All path components must exist. If
- RESOLVED is null, the result is malloc'd; otherwise, if the
- canonical name is PATH_MAX chars or more, returns null with `errno'
- set to ENAMETOOLONG; if the name fits in fewer than PATH_MAX chars,
- returns the name in RESOLVED. If the name cannot be resolved and
- RESOLVED is non-NULL, it contains the path of the first component
- that cannot be resolved. If the path can be resolved, RESOLVED
- holds the same value as the value returned. */
+/* Suppress bogus GCC -Wmaybe-uninitialized warnings. */
+#if defined GCC_LINT || defined lint
+# define IF_LINT(Code) Code
+#else
+# define IF_LINT(Code) /* empty */
+#endif
-char *
-__realpath (const char *name, char *resolved)
+#ifndef DOUBLE_SLASH_IS_DISTINCT_ROOT
+# define DOUBLE_SLASH_IS_DISTINCT_ROOT false
+#endif
+
+#if defined _LIBC || !FUNC_REALPATH_WORKS
+
+/* Return true if FILE's existence can be shown, false (setting errno)
+ otherwise. Follow symbolic links. */
+static bool
+file_accessible (char const *file)
+{
+# if defined _LIBC || HAVE_FACCESSAT
+ return __faccessat (AT_FDCWD, file, F_OK, AT_EACCESS) == 0;
+# else
+ struct stat st;
+ return __stat (file, &st) == 0 || errno == EOVERFLOW;
+# endif
+}
+
+/* True if concatenating END as a suffix to a file name means that the
+ code needs to check that the file name is that of a searchable
+ directory, since the canonicalize_filename_mode_stk code won't
+ check this later anyway when it checks an ordinary file name
+ component within END. END must either be empty, or start with a
+ slash. */
+
+static bool _GL_ATTRIBUTE_PURE
+suffix_requires_dir_check (char const *end)
+{
+ /* If END does not start with a slash, the suffix is OK. */
+ while (ISSLASH (*end))
+ {
+ /* Two or more slashes act like a single slash. */
+ do
+ end++;
+ while (ISSLASH (*end));
+
+ switch (*end++)
+ {
+ default: return false; /* An ordinary file name component is OK. */
+ case '\0': return true; /* Trailing "/" is trouble. */
+ case '.': break; /* Possibly "." or "..". */
+ }
+ /* Trailing "/.", or "/.." even if not trailing, is trouble. */
+ if (!*end || (*end == '.' && (!end[1] || ISSLASH (end[1]))))
+ return true;
+ }
+
+ return false;
+}
+
+/* Append this to a file name to test whether it is a searchable directory.
+ On POSIX platforms "/" suffices, but "/./" is sometimes needed on
+ macOS 10.13 <https://bugs.gnu.org/30350>, and should also work on
+ platforms like AIX 7.2 that need at least "/.". */
+
+#if defined _LIBC || defined LSTAT_FOLLOWS_SLASHED_SYMLINK
+static char const dir_suffix[] = "/";
+#else
+static char const dir_suffix[] = "/./";
+#endif
+
+/* Return true if DIR is a searchable dir, false (setting errno) otherwise.
+ DIREND points to the NUL byte at the end of the DIR string.
+ Store garbage into DIREND[0 .. strlen (dir_suffix)]. */
+
+static bool
+dir_check (char *dir, char *dirend)
+{
+ strcpy (dirend, dir_suffix);
+ return file_accessible (dir);
+}
+
+static idx_t
+get_path_max (void)
+{
+# ifdef PATH_MAX
+ long int path_max = PATH_MAX;
+# else
+ /* The caller invoked realpath with a null RESOLVED, even though
+ PATH_MAX is not defined as a constant. The glibc manual says
+ programs should not do this, and POSIX says the behavior is undefined.
+ Historically, glibc here used the result of pathconf, or 1024 if that
+ failed; stay consistent with this (dubious) historical practice. */
+ int err = errno;
+ long int path_max = __pathconf ("/", _PC_PATH_MAX);
+ __set_errno (err);
+# endif
+ return path_max < 0 ? 1024 : path_max <= IDX_MAX ? path_max : IDX_MAX;
+}
+
+/* Act like __realpath (see below), with an additional argument
+ rname_buf that can be used as temporary storage.
+
+ If GCC_LINT is defined, do not inline this function with GCC 10.1
+ and later, to avoid creating a pointer to the stack that GCC
+ -Wreturn-local-addr incorrectly complains about. See:
+ https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93644
+ Although the noinline attribute can hurt performance a bit, no better way
+ to pacify GCC is known; even an explicit #pragma does not pacify GCC.
+ When the GCC bug is fixed this workaround should be limited to the
+ broken GCC versions. */
+#if __GNUC_PREREQ (10, 1)
+# if defined GCC_LINT || defined lint
+__attribute__ ((__noinline__))
+# elif __OPTIMIZE__ && !__NO_INLINE__
+# define GCC_BOGUS_WRETURN_LOCAL_ADDR
+# endif
+#endif
+static char *
+realpath_stk (const char *name, char *resolved,
+ struct scratch_buffer *rname_buf)
{
- char *rpath, *dest, *extra_buf = NULL;
- const char *start, *end, *rpath_limit;
- long int path_max;
+ char *dest;
+ char const *start;
+ char const *end;
int num_links = 0;
if (name == NULL)
{
/* As per Single Unix Specification V2 we must return an error if
- either parameter is a null pointer. We extend this to allow
- the RESOLVED parameter to be NULL in case the we are expected to
- allocate the room for the return value. */
+ either parameter is a null pointer. We extend this to allow
+ the RESOLVED parameter to be NULL in case the we are expected to
+ allocate the room for the return value. */
__set_errno (EINVAL);
return NULL;
}
@@ -60,166 +217,230 @@ __realpath (const char *name, char *resolved)
if (name[0] == '\0')
{
/* As per Single Unix Specification V2 we must return an error if
- the name argument points to an empty string. */
+ the name argument points to an empty string. */
__set_errno (ENOENT);
return NULL;
}
-#ifdef PATH_MAX
- path_max = PATH_MAX;
-#else
- path_max = __pathconf (name, _PC_PATH_MAX);
- if (path_max <= 0)
- path_max = 1024;
-#endif
+ struct scratch_buffer extra_buffer, link_buffer;
+ scratch_buffer_init (&extra_buffer);
+ scratch_buffer_init (&link_buffer);
+ scratch_buffer_init (rname_buf);
+ char *rname_on_stack = rname_buf->data;
+ char *rname = rname_on_stack;
+ bool end_in_extra_buffer = false;
+ bool failed = true;
- if (resolved == NULL)
- {
- rpath = malloc (path_max);
- if (rpath == NULL)
- return NULL;
- }
- else
- rpath = resolved;
- rpath_limit = rpath + path_max;
+ /* This is always zero for Posix hosts, but can be 2 for MS-Windows
+ and MS-DOS X:/foo/bar file names. */
+ idx_t prefix_len = FILE_SYSTEM_PREFIX_LEN (name);
- if (name[0] != '/')
+ if (!IS_ABSOLUTE_FILE_NAME (name))
{
- if (!__getcwd (rpath, path_max))
- {
- rpath[0] = '\0';
- goto error;
- }
- dest = __rawmemchr (rpath, '\0');
+ while (!__getcwd (rname, rname_buf->length))
+ {
+ if (errno != ERANGE)
+ {
+ dest = rname;
+ goto error;
+ }
+ if (!scratch_buffer_grow (rname_buf))
+ goto error_nomem;
+ rname = rname_buf->data;
+ }
+ dest = __rawmemchr (rname, '\0');
+ start = name;
+ prefix_len = FILE_SYSTEM_PREFIX_LEN (rname);
}
else
{
- rpath[0] = '/';
- dest = rpath + 1;
+ dest = __mempcpy (rname, name, prefix_len);
+ *dest++ = '/';
+ if (DOUBLE_SLASH_IS_DISTINCT_ROOT)
+ {
+ if (prefix_len == 0 /* implies ISSLASH (name[0]) */
+ && ISSLASH (name[1]) && !ISSLASH (name[2]))
+ *dest++ = '/';
+ *dest = '\0';
+ }
+ start = name + prefix_len;
}
- for (start = end = name; *start; start = end)
+ for ( ; *start; start = end)
{
- struct stat64 st;
- int n;
-
- /* Skip sequence of multiple path-separators. */
- while (*start == '/')
- ++start;
-
- /* Find end of path component. */
- for (end = start; *end && *end != '/'; ++end)
- /* Nothing. */;
-
- if (end - start == 0)
- break;
- else if (end - start == 1 && start[0] == '.')
- /* nothing */;
- else if (end - start == 2 && start[0] == '.' && start[1] == '.')
- {
- /* Back up to previous component, ignore if at root already. */
- if (dest > rpath + 1)
- while ((--dest)[-1] != '/');
- }
+ /* Skip sequence of multiple file name separators. */
+ while (ISSLASH (*start))
+ ++start;
+
+ /* Find end of component. */
+ for (end = start; *end && !ISSLASH (*end); ++end)
+ /* Nothing. */;
+
+ /* Length of this file name component; it can be zero if a file
+ name ends in '/'. */
+ idx_t startlen = end - start;
+
+ if (startlen == 0)
+ break;
+ else if (startlen == 1 && start[0] == '.')
+ /* nothing */;
+ else if (startlen == 2 && start[0] == '.' && start[1] == '.')
+ {
+ /* Back up to previous component, ignore if at root already. */
+ if (dest > rname + prefix_len + 1)
+ for (--dest; dest > rname && !ISSLASH (dest[-1]); --dest)
+ continue;
+ if (DOUBLE_SLASH_IS_DISTINCT_ROOT
+ && dest == rname + 1 && !prefix_len
+ && ISSLASH (*dest) && !ISSLASH (dest[1]))
+ dest++;
+ }
else
- {
- size_t new_size;
-
- if (dest[-1] != '/')
- *dest++ = '/';
-
- if (dest + (end - start) >= rpath_limit)
- {
- ptrdiff_t dest_offset = dest - rpath;
- char *new_rpath;
-
- if (resolved)
- {
- __set_errno (ENAMETOOLONG);
- if (dest > rpath + 1)
- dest--;
- *dest = '\0';
- goto error;
- }
- new_size = rpath_limit - rpath;
- if (end - start + 1 > path_max)
- new_size += end - start + 1;
- else
- new_size += path_max;
- new_rpath = (char *) realloc (rpath, new_size);
- if (new_rpath == NULL)
- goto error;
- rpath = new_rpath;
- rpath_limit = rpath + new_size;
-
- dest = rpath + dest_offset;
- }
-
- dest = __mempcpy (dest, start, end - start);
- *dest = '\0';
-
- if (__lstat64 (rpath, &st) < 0)
- goto error;
-
- if (S_ISLNK (st.st_mode))
- {
- char *buf = __alloca (path_max);
- size_t len;
-
- if (++num_links > __eloop_threshold ())
- {
- __set_errno (ELOOP);
- goto error;
- }
-
- n = __readlink (rpath, buf, path_max - 1);
- if (n < 0)
- goto error;
- buf[n] = '\0';
-
- if (!extra_buf)
- extra_buf = __alloca (path_max);
-
- len = strlen (end);
- if (path_max - n <= len)
- {
- __set_errno (ENAMETOOLONG);
- goto error;
- }
-
- /* Careful here, end may be a pointer into extra_buf... */
- memmove (&extra_buf[n], end, len + 1);
- name = end = memcpy (extra_buf, buf, n);
-
- if (buf[0] == '/')
- dest = rpath + 1; /* It's an absolute symlink */
- else
- /* Back up to previous component, ignore if at root already: */
- if (dest > rpath + 1)
- while ((--dest)[-1] != '/');
- }
- else if (!S_ISDIR (st.st_mode) && *end != '\0')
- {
- __set_errno (ENOTDIR);
- goto error;
- }
- }
+ {
+ if (!ISSLASH (dest[-1]))
+ *dest++ = '/';
+
+ while (rname + rname_buf->length - dest
+ < startlen + sizeof dir_suffix)
+ {
+ idx_t dest_offset = dest - rname;
+ if (!scratch_buffer_grow_preserve (rname_buf))
+ goto error_nomem;
+ rname = rname_buf->data;
+ dest = rname + dest_offset;
+ }
+
+ dest = __mempcpy (dest, start, startlen);
+ *dest = '\0';
+
+ char *buf;
+ ssize_t n;
+ while (true)
+ {
+ buf = link_buffer.data;
+ idx_t bufsize = link_buffer.length;
+ n = __readlink (rname, buf, bufsize - 1);
+ if (n < bufsize - 1)
+ break;
+ if (!scratch_buffer_grow (&link_buffer))
+ goto error_nomem;
+ }
+ if (0 <= n)
+ {
+ if (++num_links > __eloop_threshold ())
+ {
+ __set_errno (ELOOP);
+ goto error;
+ }
+
+ buf[n] = '\0';
+
+ char *extra_buf = extra_buffer.data;
+ idx_t end_idx IF_LINT (= 0);
+ if (end_in_extra_buffer)
+ end_idx = end - extra_buf;
+ idx_t len = strlen (end);
+ if (INT_ADD_OVERFLOW (len, n))
+ {
+ __set_errno (ENAMETOOLONG);
+ goto error_nomem;
+ }
+ while (extra_buffer.length <= len + n)
+ {
+ if (!scratch_buffer_grow_preserve (&extra_buffer))
+ goto error_nomem;
+ extra_buf = extra_buffer.data;
+ }
+ if (end_in_extra_buffer)
+ end = extra_buf + end_idx;
+
+ /* Careful here, end may be a pointer into extra_buf... */
+ memmove (&extra_buf[n], end, len + 1);
+ name = end = memcpy (extra_buf, buf, n);
+ end_in_extra_buffer = true;
+
+ if (IS_ABSOLUTE_FILE_NAME (buf))
+ {
+ idx_t pfxlen = FILE_SYSTEM_PREFIX_LEN (buf);
+
+ dest = __mempcpy (rname, buf, pfxlen);
+ *dest++ = '/'; /* It's an absolute symlink */
+ if (DOUBLE_SLASH_IS_DISTINCT_ROOT)
+ {
+ if (ISSLASH (buf[1]) && !ISSLASH (buf[2]) && !pfxlen)
+ *dest++ = '/';
+ *dest = '\0';
+ }
+ /* Install the new prefix to be in effect hereafter. */
+ prefix_len = pfxlen;
+ }
+ else
+ {
+ /* Back up to previous component, ignore if at root
+ already: */
+ if (dest > rname + prefix_len + 1)
+ for (--dest; dest > rname && !ISSLASH (dest[-1]); --dest)
+ continue;
+ if (DOUBLE_SLASH_IS_DISTINCT_ROOT && dest == rname + 1
+ && ISSLASH (*dest) && !ISSLASH (dest[1]) && !prefix_len)
+ dest++;
+ }
+ }
+ else if (! (suffix_requires_dir_check (end)
+ ? dir_check (rname, dest)
+ : errno == EINVAL))
+ goto error;
+ }
}
- if (dest > rpath + 1 && dest[-1] == '/')
+ if (dest > rname + prefix_len + 1 && ISSLASH (dest[-1]))
--dest;
- *dest = '\0';
-
- assert (resolved == NULL || resolved == rpath);
- return rpath;
+ if (DOUBLE_SLASH_IS_DISTINCT_ROOT && dest == rname + 1 && !prefix_len
+ && ISSLASH (*dest) && !ISSLASH (dest[1]))
+ dest++;
+ failed = false;
error:
- assert (resolved == NULL || resolved == rpath);
- if (resolved == NULL)
- free (rpath);
- return NULL;
+ *dest++ = '\0';
+ if (resolved != NULL && dest - rname <= get_path_max ())
+ rname = strcpy (resolved, rname);
+
+error_nomem:
+ scratch_buffer_free (&extra_buffer);
+ scratch_buffer_free (&link_buffer);
+
+ if (failed || rname == resolved)
+ {
+ scratch_buffer_free (rname_buf);
+ return failed ? NULL : resolved;
+ }
+
+ return scratch_buffer_dupfree (rname_buf, dest - rname);
+}
+
+/* Return the canonical absolute name of file NAME. A canonical name
+ does not contain any ".", ".." components nor any repeated file name
+ separators ('/') or symlinks. All file name components must exist. If
+ RESOLVED is null, the result is malloc'd; otherwise, if the
+ canonical name is PATH_MAX chars or more, returns null with 'errno'
+ set to ENAMETOOLONG; if the name fits in fewer than PATH_MAX chars,
+ returns the name in RESOLVED. If the name cannot be resolved and
+ RESOLVED is non-NULL, it contains the name of the first component
+ that cannot be resolved. If the name can be resolved, RESOLVED
+ holds the same value as the value returned. */
+
+char *
+__realpath (const char *name, char *resolved)
+{
+ #ifdef GCC_BOGUS_WRETURN_LOCAL_ADDR
+ #warning "GCC might issue a bogus -Wreturn-local-addr warning here."
+ #warning "See <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93644>."
+ #endif
+ struct scratch_buffer rname_buffer;
+ return realpath_stk (name, resolved, &rname_buffer);
}
libc_hidden_def (__realpath)
versioned_symbol (libc, __realpath, realpath, GLIBC_2_3);
+#endif /* !FUNC_REALPATH_WORKS || defined _LIBC */
#if SHLIB_COMPAT(libc, GLIBC_2_0, GLIBC_2_3)
diff --git a/sysdeps/unix/sysv/linux/faccessat.c b/sysdeps/unix/sysv/linux/faccessat.c
index 5d078371b5..5bb1051c06 100644
--- a/sysdeps/unix/sysv/linux/faccessat.c
+++ b/sysdeps/unix/sysv/linux/faccessat.c
@@ -24,7 +24,7 @@
int
-faccessat (int fd, const char *file, int mode, int flag)
+__faccessat (int fd, const char *file, int mode, int flag)
{
int ret = INLINE_SYSCALL_CALL (faccessat2, fd, file, mode, flag);
#if __ASSUME_FACCESSAT2
@@ -73,3 +73,4 @@ faccessat (int fd, const char *file, int mode, int flag)
return INLINE_SYSCALL_ERROR_RETURN_VALUE (EACCES);
#endif /* !__ASSUME_FACCESSAT2 */
}
+weak_alias (__faccessat, faccessat)
--
2.25.1
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v3 4/6] stdlib: Sync canonicalize with gnulib [BZ #10635] [BZ #26592] [BZ #26341] [BZ #24970]
2020-12-29 19:34 ` [PATCH v3 4/6] stdlib: Sync canonicalize with gnulib [BZ #10635] [BZ #26592] [BZ #26341] [BZ #24970] Adhemerval Zanella
@ 2020-12-30 1:21 ` Paul Eggert
2020-12-30 3:39 ` Paul Eggert
2020-12-30 12:34 ` Adhemerval Zanella
0 siblings, 2 replies; 18+ messages in thread
From: Paul Eggert @ 2020-12-30 1:21 UTC (permalink / raw)
To: Adhemerval Zanella; +Cc: bug-gnulib, libc-alpha
[-- Attachment #1: Type: text/plain, Size: 992 bytes --]
On 12/29/20 11:34 AM, Adhemerval Zanella wrote:
> idx_t len = strlen (end);
> + if (INT_ADD_OVERFLOW (len, n))
> + {
> + __set_errno (ENAMETOOLONG);
> + goto error_nomem;
> + }
The other patches in this glibc patch series look good to me. However,
this patch has some problems. First, the overflow check does not handle
the case where strlen (end) does not fit into len. Second, ENAMETOOLONG
is not the right errno; it should be ENOMEM because not enough memory
can be allocated (this is what scratch_buffer, malloc, etc. do in
similar situations). Third (and less important), the overflow check is
not needed on practical 64-bit platforms either now or in the forseeable
future.
I installed the attached patch into Gnulib to fix the bug in a way I
hope is better. The idea is that you should be able to sync this into
glibc without needing a patch like the above.
[-- Attachment #2: 0001-canonicalize-fix-ptrdiff_t-overflow-bug.patch --]
[-- Type: text/x-patch, Size: 5658 bytes --]
From b4e94717557545d613bca58a27d4ef698d551ed2 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Tue, 29 Dec 2020 17:08:11 -0800
Subject: [PATCH] canonicalize: fix ptrdiff_t overflow bug
Problem reported by Adhemerval Zanella in:
https://sourceware.org/pipermail/libc-alpha/2020-December/121182.html
* lib/canonicalize-lgpl.c, lib/canonicalize.c:
Include intprops.h.
(NARROW_ADDRESSES): New constant.
* lib/canonicalize-lgpl.c (realpath_stk):m
* lib/canonicalize.c (canonicalize_filename_mode_stk):
Work even if strlen (END) does not fit in idx_t, or if adding
N to it overflows.
* modules/canonicalize, modules/canonicalize-lgpl (Depends-on):
Add intprops.
---
ChangeLog | 15 +++++++++++++++
lib/canonicalize-lgpl.c | 12 +++++++++++-
lib/canonicalize.c | 12 +++++++++++-
modules/canonicalize | 1 +
modules/canonicalize-lgpl | 1 +
5 files changed, 39 insertions(+), 2 deletions(-)
diff --git a/ChangeLog b/ChangeLog
index d03007b3e..0ef300f0b 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,18 @@
+2020-12-29 Paul Eggert <eggert@cs.ucla.edu>
+
+ canonicalize: fix ptrdiff_t overflow bug
+ Problem reported by Adhemerval Zanella in:
+ https://sourceware.org/pipermail/libc-alpha/2020-December/121182.html
+ * lib/canonicalize-lgpl.c, lib/canonicalize.c:
+ Include intprops.h.
+ (NARROW_ADDRESSES): New constant.
+ * lib/canonicalize-lgpl.c (realpath_stk):m
+ * lib/canonicalize.c (canonicalize_filename_mode_stk):
+ Work even if strlen (END) does not fit in idx_t, or if adding
+ N to it overflows.
+ * modules/canonicalize, modules/canonicalize-lgpl (Depends-on):
+ Add intprops.
+
2020-12-28 Bruno Haible <bruno@clisp.org>
havelib: Fix for Solaris 11 OpenIndiana and Solaris 11 OmniOS.
diff --git a/lib/canonicalize-lgpl.c b/lib/canonicalize-lgpl.c
index 04fe95253..e8b10f0e7 100644
--- a/lib/canonicalize-lgpl.c
+++ b/lib/canonicalize-lgpl.c
@@ -40,6 +40,7 @@
#include <eloop-threshold.h>
#include <filename.h>
#include <idx.h>
+#include <intprops.h>
#include <scratch_buffer.h>
#ifdef _LIBC
@@ -85,6 +86,10 @@
# define IF_LINT(Code) /* empty */
#endif
+/* True if adding two valid object sizes might overflow idx_t.
+ As a practical matter, this cannot happen on 64-bit machines. */
+enum { NARROW_ADDRESSES = IDX_MAX >> 31 >> 31 == 0 };
+
#ifndef DOUBLE_SLASH_IS_DISTINCT_ROOT
# define DOUBLE_SLASH_IS_DISTINCT_ROOT false
#endif
@@ -338,7 +343,12 @@ realpath_stk (const char *name, char *resolved,
idx_t end_idx IF_LINT (= 0);
if (end_in_extra_buffer)
end_idx = end - extra_buf;
- idx_t len = strlen (end);
+ size_t len = strlen (end);
+ if (NARROW_ADDRESSES && INT_ADD_OVERFLOW (len, n))
+ {
+ __set_errno (ENOMEM);
+ goto error;
+ }
while (extra_buffer.length <= len + n)
{
if (!scratch_buffer_grow_preserve (&extra_buffer))
diff --git a/lib/canonicalize.c b/lib/canonicalize.c
index a4d3aab96..eee3dbee6 100644
--- a/lib/canonicalize.c
+++ b/lib/canonicalize.c
@@ -29,6 +29,7 @@
#include <filename.h>
#include <idx.h>
+#include <intprops.h>
#include <scratch_buffer.h>
#include "attribute.h"
@@ -43,6 +44,10 @@
# define IF_LINT(Code) /* empty */
#endif
+/* True if adding two valid object sizes might overflow idx_t.
+ As a practical matter, this cannot happen on 64-bit machines. */
+enum { NARROW_ADDRESSES = IDX_MAX >> 31 >> 31 == 0 };
+
#ifndef DOUBLE_SLASH_IS_DISTINCT_ROOT
# define DOUBLE_SLASH_IS_DISTINCT_ROOT false
#endif
@@ -389,7 +394,12 @@ canonicalize_filename_mode_stk (const char *name, canonicalize_mode_t can_mode,
idx_t end_idx IF_LINT (= 0);
if (end_in_extra_buffer)
end_idx = end - extra_buf;
- idx_t len = strlen (end);
+ size_t len = strlen (end);
+ if (NARROW_ADDRESSES && INT_ADD_OVERFLOW (len, n))
+ {
+ errno = ENOMEM;
+ goto error;
+ }
while (extra_buffer.length <= len + n)
{
if (!scratch_buffer_grow_preserve (&extra_buffer))
diff --git a/modules/canonicalize b/modules/canonicalize
index 5003f2682..a6cf76f17 100644
--- a/modules/canonicalize
+++ b/modules/canonicalize
@@ -19,6 +19,7 @@ free-posix
getcwd
hash-triple-simple
idx
+intprops
memmove
mempcpy
nocrash
diff --git a/modules/canonicalize-lgpl b/modules/canonicalize-lgpl
index a96f9011e..b8e87a607 100644
--- a/modules/canonicalize-lgpl
+++ b/modules/canonicalize-lgpl
@@ -18,6 +18,7 @@ fcntl-h [test $HAVE_CANONICALIZE_FILE_NAME = 0 || test $REPLACE_CANONI
filename [test $HAVE_CANONICALIZE_FILE_NAME = 0 || test $REPLACE_CANONICALIZE_FILE_NAME = 1]
free-posix [test $HAVE_CANONICALIZE_FILE_NAME = 0 || test $REPLACE_CANONICALIZE_FILE_NAME = 1]
idx [test $HAVE_CANONICALIZE_FILE_NAME = 0 || test $REPLACE_CANONICALIZE_FILE_NAME = 1]
+intprops [test $HAVE_CANONICALIZE_FILE_NAME = 0 || test $REPLACE_CANONICALIZE_FILE_NAME = 1]
libc-config [test $HAVE_CANONICALIZE_FILE_NAME = 0 || test $REPLACE_CANONICALIZE_FILE_NAME = 1]
malloc-posix [test $HAVE_CANONICALIZE_FILE_NAME = 0 || test $REPLACE_CANONICALIZE_FILE_NAME = 1]
memmove [test $HAVE_CANONICALIZE_FILE_NAME = 0 || test $REPLACE_CANONICALIZE_FILE_NAME = 1]
--
2.27.0
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v3 4/6] stdlib: Sync canonicalize with gnulib [BZ #10635] [BZ #26592] [BZ #26341] [BZ #24970]
2020-12-30 1:21 ` Paul Eggert
@ 2020-12-30 3:39 ` Paul Eggert
2020-12-30 6:19 ` Paul Eggert
2020-12-30 12:34 ` Adhemerval Zanella
1 sibling, 1 reply; 18+ messages in thread
From: Paul Eggert @ 2020-12-30 3:39 UTC (permalink / raw)
To: Adhemerval Zanella; +Cc: bug-gnulib, libc-alpha
[-- Attachment #1: Type: text/plain, Size: 295 bytes --]
On 12/29/20 5:21 PM, Paul Eggert wrote:
> I installed the attached patch into Gnulib to fix the bug in a way I
> hope is better.
Unfortunately that patch didn't correctly treat size-calculation
overflow like other out-of-memory situations. I installed the attached
further patch into Gnulib.
[-- Attachment #2: 0001-canonicalize-fix-size-overflow-treatment.patch --]
[-- Type: text/x-patch, Size: 3997 bytes --]
From 649e713c1e5452204253cb1029ea22c1b6effa2e Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Tue, 29 Dec 2020 19:34:59 -0800
Subject: [PATCH] canonicalize: fix size overflow treatment
This also has some minor cleanups.
* lib/canonicalize-lgpl.c, lib/canonicalize.c: No need to include
stddef.h, since the code no longer refers directly to ptrdiff_t.
* lib/canonicalize-lgpl.c (realpath_stk):
* lib/canonicalize.c (canonicalize_filename_mode_stk):
Treat size overflow like other out-of-memory.
* lib/canonicalize.c: No need to include stdlib.h, since
the code no longer refers to stdlib.h functions (other
than those that canonicalize.h must declare).
* lib/canonicalize.c (canonicalize_filename_mode_stk):
Do not bother terminating the string result on error.
---
ChangeLog | 15 +++++++++++++++
lib/canonicalize-lgpl.c | 6 +-----
lib/canonicalize.c | 9 ++-------
3 files changed, 18 insertions(+), 12 deletions(-)
diff --git a/ChangeLog b/ChangeLog
index cca14c910..2af7a42c7 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,18 @@
+2020-12-29 Paul Eggert <eggert@cs.ucla.edu>
+
+ canonicalize: fix size overflow treatment
+ This also has some minor cleanups.
+ * lib/canonicalize-lgpl.c, lib/canonicalize.c: No need to include
+ stddef.h, since the code no longer refers directly to ptrdiff_t.
+ * lib/canonicalize-lgpl.c (realpath_stk):
+ * lib/canonicalize.c (canonicalize_filename_mode_stk):
+ Treat size overflow like other out-of-memory.
+ * lib/canonicalize.c: No need to include stdlib.h, since
+ the code no longer refers to stdlib.h functions (other
+ than those that canonicalize.h must declare).
+ * lib/canonicalize.c (canonicalize_filename_mode_stk):
+ Do not bother terminating the string result on error.
+
2020-12-29 Bruno Haible <bruno@clisp.org>
list-c++, [o]map-c++, [o]set-c++: Fix conflict with 'free-posix' module.
diff --git a/lib/canonicalize-lgpl.c b/lib/canonicalize-lgpl.c
index e8b10f0e7..01b06322d 100644
--- a/lib/canonicalize-lgpl.c
+++ b/lib/canonicalize-lgpl.c
@@ -32,7 +32,6 @@
#include <fcntl.h>
#include <limits.h>
#include <stdbool.h>
-#include <stddef.h>
#include <string.h>
#include <sys/stat.h>
#include <unistd.h>
@@ -345,10 +344,7 @@ realpath_stk (const char *name, char *resolved,
end_idx = end - extra_buf;
size_t len = strlen (end);
if (NARROW_ADDRESSES && INT_ADD_OVERFLOW (len, n))
- {
- __set_errno (ENOMEM);
- goto error;
- }
+ goto error_nomem;
while (extra_buffer.length <= len + n)
{
if (!scratch_buffer_grow_preserve (&extra_buffer))
diff --git a/lib/canonicalize.c b/lib/canonicalize.c
index eee3dbee6..26066831c 100644
--- a/lib/canonicalize.c
+++ b/lib/canonicalize.c
@@ -21,8 +21,6 @@
#include <errno.h>
#include <fcntl.h>
#include <stdbool.h>
-#include <stddef.h>
-#include <stdlib.h>
#include <string.h>
#include <sys/stat.h>
#include <unistd.h>
@@ -396,10 +394,7 @@ canonicalize_filename_mode_stk (const char *name, canonicalize_mode_t can_mode,
end_idx = end - extra_buf;
size_t len = strlen (end);
if (NARROW_ADDRESSES && INT_ADD_OVERFLOW (len, n))
- {
- errno = ENOMEM;
- goto error;
- }
+ xalloc_die ();
while (extra_buffer.length <= len + n)
{
if (!scratch_buffer_grow_preserve (&extra_buffer))
@@ -461,7 +456,6 @@ canonicalize_filename_mode_stk (const char *name, canonicalize_mode_t can_mode,
failed = false;
error:
- *dest++ = '\0';
if (ht)
hash_free (ht);
scratch_buffer_free (&extra_buffer);
@@ -473,6 +467,7 @@ error:
return NULL;
}
+ *dest++ = '\0';
char *result = scratch_buffer_dupfree (rname_buf, dest - rname);
if (!result)
xalloc_die ();
--
2.27.0
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v3 4/6] stdlib: Sync canonicalize with gnulib [BZ #10635] [BZ #26592] [BZ #26341] [BZ #24970]
2020-12-30 3:39 ` Paul Eggert
@ 2020-12-30 6:19 ` Paul Eggert
0 siblings, 0 replies; 18+ messages in thread
From: Paul Eggert @ 2020-12-30 6:19 UTC (permalink / raw)
To: Adhemerval Zanella; +Cc: libc-alpha, bug-gnulib
[-- Attachment #1: Type: text/plain, Size: 361 bytes --]
On 12/29/20 7:39 PM, Paul Eggert wrote:
> Unfortunately that patch didn't correctly treat size-calculation
> overflow like other out-of-memory situations. I installed the attached
> further patch into Gnulib.
Aaaaand even that patch mishandled errno on size-calculation overflow,
so I installed the attached into Gnulib as well. Hope this finally does it.
[-- Attachment #2: 0001-lib-canonicalize-lgpl.c-realpath_stk-Set-errno-prope.patch --]
[-- Type: text/x-patch, Size: 1000 bytes --]
From ed5ce2ad20fb12f60fa2bdb95c0c2a7fe7ce1114 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Tue, 29 Dec 2020 22:17:46 -0800
Subject: [PATCH] * lib/canonicalize-lgpl.c (realpath_stk): Set errno properly.
---
lib/canonicalize-lgpl.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/lib/canonicalize-lgpl.c b/lib/canonicalize-lgpl.c
index 01b06322d..49596f835 100644
--- a/lib/canonicalize-lgpl.c
+++ b/lib/canonicalize-lgpl.c
@@ -344,7 +344,10 @@ realpath_stk (const char *name, char *resolved,
end_idx = end - extra_buf;
size_t len = strlen (end);
if (NARROW_ADDRESSES && INT_ADD_OVERFLOW (len, n))
- goto error_nomem;
+ {
+ __set_errno (ENOMEM);
+ goto error_nomem;
+ }
while (extra_buffer.length <= len + n)
{
if (!scratch_buffer_grow_preserve (&extra_buffer))
--
2.27.0
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 4/6] stdlib: Sync canonicalize with gnulib [BZ #10635] [BZ #26592] [BZ #26341] [BZ #24970]
2020-12-30 1:21 ` Paul Eggert
2020-12-30 3:39 ` Paul Eggert
@ 2020-12-30 12:34 ` Adhemerval Zanella
2020-12-30 13:10 ` Adhemerval Zanella
1 sibling, 1 reply; 18+ messages in thread
From: Adhemerval Zanella @ 2020-12-30 12:34 UTC (permalink / raw)
To: Paul Eggert; +Cc: bug-gnulib, libc-alpha
On 29/12/2020 22:21, Paul Eggert wrote:
> On 12/29/20 11:34 AM, Adhemerval Zanella wrote:
>> idx_t len = strlen (end);
>> + if (INT_ADD_OVERFLOW (len, n))
>> + {
>> + __set_errno (ENAMETOOLONG);
>> + goto error_nomem;
>> + }
>
>
> The other patches in this glibc patch series look good to me. However, this patch has some problems. First, the overflow check does not handle the case where strlen (end) does not fit into len. Second, ENAMETOOLONG is not the right errno; it should be ENOMEM because not enough memory can be allocated (this is what scratch_buffer, malloc, etc. do in similar situations). Third (and less important), the overflow check is not needed on practical 64-bit platforms either now or in the forseeable future.
>
> I installed the attached patch into Gnulib to fix the bug in a way I hope is better. The idea is that you should be able to sync this into glibc without needing a patch like the above.
>
Indeed, the test which triggered only failed on 32-bits platforms
and it uses a exactly INT_MAX size to trigger it. I agree that
it should not be a problem for 64-bit, however I don't think there is
much gain is adding the NARROW_ADDRESSES trick: it makes the code
conditionally build depending of the idx_t size and it is just really
a small optimization that adds code complexity on a somewhat convoluted
code.
For ENAMETOOLONG, I think this is the right error code: it enforces
that we do not support internal objects longer that PTRDIFF_MAX.
The glibc now enforces is through malloc functions and we haven't
done it through mmap because if I remember correctly Carlos has pointed
out some esoteric use case by some projects that allocated 2GB plus
some slack of continuous memory for 32-bit (I really want to start
enforce on mmap as well, maybe I will send a patch for new glibc version).
I think it should be a fair assumption to make it on internal code, such
as realpath (this is another reason why I think NARROW_ADDRESSES is not
necessary).
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v3 4/6] stdlib: Sync canonicalize with gnulib [BZ #10635] [BZ #26592] [BZ #26341] [BZ #24970]
2020-12-30 12:34 ` Adhemerval Zanella
@ 2020-12-30 13:10 ` Adhemerval Zanella
2021-01-02 0:04 ` Paul Eggert
0 siblings, 1 reply; 18+ messages in thread
From: Adhemerval Zanella @ 2020-12-30 13:10 UTC (permalink / raw)
To: Paul Eggert; +Cc: bug-gnulib, libc-alpha
On 30/12/2020 09:34, Adhemerval Zanella wrote:
>
>
> On 29/12/2020 22:21, Paul Eggert wrote:
>> On 12/29/20 11:34 AM, Adhemerval Zanella wrote:
>>> idx_t len = strlen (end);
>>> + if (INT_ADD_OVERFLOW (len, n))
>>> + {
>>> + __set_errno (ENAMETOOLONG);
>>> + goto error_nomem;
>>> + }
>>
>>
>> The other patches in this glibc patch series look good to me. However, this patch has some problems. First, the overflow check does not handle the case where strlen (end) does not fit into len. Second, ENAMETOOLONG is not the right errno; it should be ENOMEM because not enough memory can be allocated (this is what scratch_buffer, malloc, etc. do in similar situations). Third (and less important), the overflow check is not needed on practical 64-bit platforms either now or in the forseeable future.
>>
>> I installed the attached patch into Gnulib to fix the bug in a way I hope is better. The idea is that you should be able to sync this into glibc without needing a patch like the above.
>>
>
> Indeed, the test which triggered only failed on 32-bits platforms
> and it uses a exactly INT_MAX size to trigger it. I agree that
> it should not be a problem for 64-bit, however I don't think there is
> much gain is adding the NARROW_ADDRESSES trick: it makes the code
> conditionally build depending of the idx_t size and it is just really
> a small optimization that adds code complexity on a somewhat convoluted
> code.
>
> For ENAMETOOLONG, I think this is the right error code: it enforces
> that we do not support internal objects longer that PTRDIFF_MAX.
> The glibc now enforces is through malloc functions and we haven't
> done it through mmap because if I remember correctly Carlos has pointed
> out some esoteric use case by some projects that allocated 2GB plus
> some slack of continuous memory for 32-bit (I really want to start
> enforce on mmap as well, maybe I will send a patch for new glibc version).
> I think it should be a fair assumption to make it on internal code, such
> as realpath (this is another reason why I think NARROW_ADDRESSES is not
> necessary).
>
And your fix (from 93e0186d4) does not really solve the issue, since
now that len is a size_t the overflow check won't catch the potentially
allocation larger than PTRDIFF_MAX (the realpath will still fail with
ENOMEM though).
Wouldn't the below be simpler?
size_t len = strlen (end);
if (len > IDX_MAX || INT_ADD_OVERFLOW ((idx_t) len, n))
{
__set_errno (ENAMETOOLONG);
goto error_nomem;
}
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v3 4/6] stdlib: Sync canonicalize with gnulib [BZ #10635] [BZ #26592] [BZ #26341] [BZ #24970]
2020-12-30 13:10 ` Adhemerval Zanella
@ 2021-01-02 0:04 ` Paul Eggert
2021-01-04 12:52 ` Adhemerval Zanella
0 siblings, 1 reply; 18+ messages in thread
From: Paul Eggert @ 2021-01-02 0:04 UTC (permalink / raw)
To: Adhemerval Zanella; +Cc: bug-gnulib, libc-alpha
[-- Attachment #1: Type: text/plain, Size: 2898 bytes --]
On 12/30/20 5:10 AM, Adhemerval Zanella wrote:
>> it is just really
>> a small optimization that adds code complexity on a somewhat convoluted
>> code.
The code is indeed simpler without the NARROW_ADDRESSES optimization, so
I removed that optimization by installing the attached patch into Gnulib.
>> For ENAMETOOLONG, I think this is the right error code: it enforces
>> that we do not support internal objects longer that PTRDIFF_MAX.
This sounds backwards, as the code returns ENOMEM every other place it
tries to create an internal object longer than PTRDIFF_MAX - these
ENOMEM checks are in the malloc calls invoked by scratch_buffer_grow and
scratch_buffer_grow_preserve. It would be odd for canonicalize_file_name
to return ENAMETOOLONG for this one particular way of creating a
too-large object, while at the same time it returns ENOMEM for all the
other ways.
Besides, ENAMETOOLONG is the POSIX error code for exceeding NAME_MAX or
PATH_MAX, which is not what is happening here.
In Gnulib and other GNU apps we've long used the tradition that ENOMEM
means you've run out of memory, regardless of whether it's because your
heap or your address space is too small. This is a good tradition and
it'd be good to use it here too.
>> I think it should be a fair assumption to make it on internal code, such
>> as realpath
Yes, staying less than PTRDIFF_MAX is a vital assumption on internal
objects. I'd go even further and say it's important for user-supplied
objects, too, as so much code relies on pointer subtraction and we can't
realistically prohibit that within glibc.
> (this is another reason why I think NARROW_ADDRESSES is not
> necessary).
Unfortunately, if we merely assume every object has at most PTRDIFF_MAX
bytes, we still must check for overflow when adding the sizes of two
objects. The NARROW_ADDRESSES optimization would have let us avoid that
unnecessary check on 64-bit machines.
> And your fix (from 93e0186d4) does not really solve the issue, since
> now that len is a size_t the overflow check won't catch the potentially
> allocation larger than PTRDIFF_MAX (the realpath will still fail with
> ENOMEM though).
Sure, which means the code is doing the right thing: it's failing with
ENOMEM because it ran out of memory. There is no need for an extra
PTRDIFF_MAX check in canonicalize.c if malloc (via scratch_buffer_grow)
already does the check.
> Wouldn't the below be simpler?
>
> size_t len = strlen (end);
> if (len > IDX_MAX || INT_ADD_OVERFLOW ((idx_t) len, n))
> {
> __set_errno (ENAMETOOLONG);
> goto error_nomem;
> }
It's not simpler than the attached Gnulib patch, because it contains an
unnecessary comparison to IDX_MAX and an unnecessary cast to idx_t.
[-- Attachment #2: 0001-canonicalize-remove-NARROW_ADDRESSES-optimization.patch --]
[-- Type: text/x-patch, Size: 3184 bytes --]
From 8f6b9b66be6672bed1045c27e606dd9fcedcf022 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Fri, 1 Jan 2021 15:54:43 -0800
Subject: [PATCH] canonicalize: remove NARROW_ADDRESSES optimization
* lib/canonicalize-lgpl.c, lib/canonicalize.c (NARROW_ADDRESSES):
Remove, and remove all uses, as the optimization is arguably not
worth the extra complexity. Suggested by Adhemerval Zanella in:
https://sourceware.org/pipermail/libc-alpha/2020-December/121203.html
---
ChangeLog | 8 ++++++++
lib/canonicalize-lgpl.c | 6 +-----
lib/canonicalize.c | 6 +-----
3 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/ChangeLog b/ChangeLog
index 2d498a5e9..fc45e1176 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,11 @@
+2021-01-01 Paul Eggert <eggert@cs.ucla.edu>
+
+ canonicalize: remove NARROW_ADDRESSES optimization
+ * lib/canonicalize-lgpl.c, lib/canonicalize.c (NARROW_ADDRESSES):
+ Remove, and remove all uses, as the optimization is arguably not
+ worth the extra complexity. Suggested by Adhemerval Zanella in:
+ https://sourceware.org/pipermail/libc-alpha/2020-December/121203.html
+
2021-01-01 Bruno Haible <bruno@clisp.org>
stddef: Try harder to get max_align_t defined on OpenBSD.
diff --git a/lib/canonicalize-lgpl.c b/lib/canonicalize-lgpl.c
index 560e24288..698f9ede2 100644
--- a/lib/canonicalize-lgpl.c
+++ b/lib/canonicalize-lgpl.c
@@ -85,10 +85,6 @@
# define IF_LINT(Code) /* empty */
#endif
-/* True if adding two valid object sizes might overflow idx_t.
- As a practical matter, this cannot happen on 64-bit machines. */
-enum { NARROW_ADDRESSES = IDX_MAX >> 31 >> 31 == 0 };
-
#ifndef DOUBLE_SLASH_IS_DISTINCT_ROOT
# define DOUBLE_SLASH_IS_DISTINCT_ROOT false
#endif
@@ -343,7 +339,7 @@ realpath_stk (const char *name, char *resolved,
if (end_in_extra_buffer)
end_idx = end - extra_buf;
size_t len = strlen (end);
- if (NARROW_ADDRESSES && INT_ADD_OVERFLOW (len, n))
+ if (INT_ADD_OVERFLOW (len, n))
{
__set_errno (ENOMEM);
goto error_nomem;
diff --git a/lib/canonicalize.c b/lib/canonicalize.c
index cc32260a8..3a1c8098b 100644
--- a/lib/canonicalize.c
+++ b/lib/canonicalize.c
@@ -42,10 +42,6 @@
# define IF_LINT(Code) /* empty */
#endif
-/* True if adding two valid object sizes might overflow idx_t.
- As a practical matter, this cannot happen on 64-bit machines. */
-enum { NARROW_ADDRESSES = IDX_MAX >> 31 >> 31 == 0 };
-
#ifndef DOUBLE_SLASH_IS_DISTINCT_ROOT
# define DOUBLE_SLASH_IS_DISTINCT_ROOT false
#endif
@@ -393,7 +389,7 @@ canonicalize_filename_mode_stk (const char *name, canonicalize_mode_t can_mode,
if (end_in_extra_buffer)
end_idx = end - extra_buf;
size_t len = strlen (end);
- if (NARROW_ADDRESSES && INT_ADD_OVERFLOW (len, n))
+ if (INT_ADD_OVERFLOW (len, n))
xalloc_die ();
while (extra_buffer.length <= len + n)
{
--
2.27.0
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v3 4/6] stdlib: Sync canonicalize with gnulib [BZ #10635] [BZ #26592] [BZ #26341] [BZ #24970]
2021-01-02 0:04 ` Paul Eggert
@ 2021-01-04 12:52 ` Adhemerval Zanella
2021-01-09 1:24 ` Paul Eggert
0 siblings, 1 reply; 18+ messages in thread
From: Adhemerval Zanella @ 2021-01-04 12:52 UTC (permalink / raw)
To: Paul Eggert; +Cc: bug-gnulib, libc-alpha
On 01/01/2021 21:04, Paul Eggert wrote:
> On 12/30/20 5:10 AM, Adhemerval Zanella wrote:
>
>>> it is just really
>>> a small optimization that adds code complexity on a somewhat convoluted
>>> code.
>
> The code is indeed simpler without the NARROW_ADDRESSES optimization, so I removed that optimization by installing the attached patch into Gnulib.
>
>>> For ENAMETOOLONG, I think this is the right error code: it enforces
>>> that we do not support internal objects longer that PTRDIFF_MAX.
>
> This sounds backwards, as the code returns ENOMEM every other place it tries to create an internal object longer than PTRDIFF_MAX - these ENOMEM checks are in the malloc calls invoked by scratch_buffer_grow and scratch_buffer_grow_preserve. It would be odd for canonicalize_file_name to return ENAMETOOLONG for this one particular way of creating a too-large object, while at the same time it returns ENOMEM for all the other ways.
>
> Besides, ENAMETOOLONG is the POSIX error code for exceeding NAME_MAX or PATH_MAX, which is not what is happening here.>
> In Gnulib and other GNU apps we've long used the tradition that ENOMEM means you've run out of memory, regardless of whether it's because your heap or your address space is too small. This is a good tradition and it'd be good to use it here too.
Right, I think we can now assume that that since the implementation does
not really imposes any NAME_MAX or PATH_MAX limitations, returning memory
allocation errors instead of ENAMETOOLONG is ok. I will adjust the
stdlib/test-bz22786.c, it will require a slight large runtime and memory
requirements (which should be ok).
>
>>> I think it should be a fair assumption to make it on internal code, such
>>> as realpath
>
> Yes, staying less than PTRDIFF_MAX is a vital assumption on internal objects. I'd go even further and say it's important for user-supplied objects, too, as so much code relies on pointer subtraction and we can't realistically prohibit that within glibc.
We do enforce it through memory allocations, however users can still craft
such objects using mmap (some libc does imposes the same PTRDIFF_MAX limit
on mmap as well).
>
>> (this is another reason why I think NARROW_ADDRESSES is not necessary).
>
> Unfortunately, if we merely assume every object has at most PTRDIFF_MAX bytes, we still must check for overflow when adding the sizes of two objects. The NARROW_ADDRESSES optimization would have let us avoid that unnecessary check on 64-bit machines.
>
>> And your fix (from 93e0186d4) does not really solve the issue, since
>> now that len is a size_t the overflow check won't catch the potentially
>> allocation larger than PTRDIFF_MAX (the realpath will still fail with
>> ENOMEM though).
>
> Sure, which means the code is doing the right thing: it's failing with ENOMEM because it ran out of memory. There is no need for an extra PTRDIFF_MAX check in canonicalize.c if malloc (via scratch_buffer_grow) already does the check.
>> Wouldn't the below be simpler?
>>
>> size_t len = strlen (end);
>> if (len > IDX_MAX || INT_ADD_OVERFLOW ((idx_t) len, n))
>> {
>> __set_errno (ENAMETOOLONG);
>> goto error_nomem;
>> }
>
> It's not simpler than the attached Gnulib patch, because it contains an unnecessary comparison to IDX_MAX and an unnecessary cast to idx_t.
The extra comparison might avoid the scratch_buffer resize that will
fail (since malloc will fail to try allocate PTRDIFF_MAX object), but
it will be used only when such objects are provided (which depending
of the system should not happen).
Thanks, I synced with the most recent gnulib version.
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v3 4/6] stdlib: Sync canonicalize with gnulib [BZ #10635] [BZ #26592] [BZ #26341] [BZ #24970]
2021-01-04 12:52 ` Adhemerval Zanella
@ 2021-01-09 1:24 ` Paul Eggert
0 siblings, 0 replies; 18+ messages in thread
From: Paul Eggert @ 2021-01-09 1:24 UTC (permalink / raw)
To: Adhemerval Zanella; +Cc: bug-gnulib, libc-alpha
On 1/4/21 4:52 AM, Adhemerval Zanella wrote:
> The extra comparison might avoid the scratch_buffer resize that will
> fail (since malloc will fail to try allocate PTRDIFF_MAX object), but
> it will be used only when such objects are provided (which depending
> of the system should not happen).
As you say, that comparison isn't needed for glibc. It's also not needed
on non-glibc because Gnulib prevents malloc etc. from succeeding on
sizes greater than PTRDIFF_MAX. So we should be OK here.
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v3 5/6] support: Add support_small_thread_stack_size
2020-12-29 19:34 [PATCH v3 0/6] Multiple fixes to realpath Adhemerval Zanella
` (3 preceding siblings ...)
2020-12-29 19:34 ` [PATCH v3 4/6] stdlib: Sync canonicalize with gnulib [BZ #10635] [BZ #26592] [BZ #26341] [BZ #24970] Adhemerval Zanella
@ 2020-12-29 19:34 ` Adhemerval Zanella
2020-12-30 8:54 ` Florian Weimer
2020-12-29 19:34 ` [PATCH v3 6/6] stdlib: Add testcase fro BZ #26241 Adhemerval Zanella
5 siblings, 1 reply; 18+ messages in thread
From: Adhemerval Zanella @ 2020-12-29 19:34 UTC (permalink / raw)
To: libc-alpha, Paul Eggert; +Cc: bug-gnulib
It returns the minimum stack size large enough to cover most internal
glibc stack usage.
---
support/support_set_small_thread_stack_size.c | 12 +++++++++---
support/xthread.h | 2 ++
2 files changed, 11 insertions(+), 3 deletions(-)
diff --git a/support/support_set_small_thread_stack_size.c b/support/support_set_small_thread_stack_size.c
index 69d66e97db..74a0e38a72 100644
--- a/support/support_set_small_thread_stack_size.c
+++ b/support/support_set_small_thread_stack_size.c
@@ -20,8 +20,8 @@
#include <pthread.h>
#include <support/xthread.h>
-void
-support_set_small_thread_stack_size (pthread_attr_t *attr)
+size_t
+support_small_thread_stack_size (void)
{
/* Some architectures have too small values for PTHREAD_STACK_MIN
which cannot be used for creating threads. Ensure that the stack
@@ -31,5 +31,11 @@ support_set_small_thread_stack_size (pthread_attr_t *attr)
if (stack_size < PTHREAD_STACK_MIN)
stack_size = PTHREAD_STACK_MIN;
#endif
- xpthread_attr_setstacksize (attr, stack_size);
+ return stack_size;
+}
+
+void
+support_set_small_thread_stack_size (pthread_attr_t *attr)
+{
+ xpthread_attr_setstacksize (attr, support_small_thread_stack_size ());
}
diff --git a/support/xthread.h b/support/xthread.h
index 05f8d4a7d9..9aeee80032 100644
--- a/support/xthread.h
+++ b/support/xthread.h
@@ -75,6 +75,8 @@ void xpthread_attr_setstacksize (pthread_attr_t *attr,
void xpthread_attr_setguardsize (pthread_attr_t *attr,
size_t guardsize);
+/* Return the stack size used on support_set_small_thread_stack_size. */
+size_t support_small_thread_stack_size (void);
/* Set the stack size in ATTR to a small value, but still large enough
to cover most internal glibc stack usage. */
void support_set_small_thread_stack_size (pthread_attr_t *attr);
--
2.25.1
^ permalink raw reply [flat|nested] 18+ messages in thread* [PATCH v3 6/6] stdlib: Add testcase fro BZ #26241
2020-12-29 19:34 [PATCH v3 0/6] Multiple fixes to realpath Adhemerval Zanella
` (4 preceding siblings ...)
2020-12-29 19:34 ` [PATCH v3 5/6] support: Add support_small_thread_stack_size Adhemerval Zanella
@ 2020-12-29 19:34 ` Adhemerval Zanella
2021-01-20 4:33 ` DJ Delorie
5 siblings, 1 reply; 18+ messages in thread
From: Adhemerval Zanella @ 2020-12-29 19:34 UTC (permalink / raw)
To: libc-alpha, Paul Eggert; +Cc: bug-gnulib
Old implementation of realpath allocates a PATH_MAX using alloca for
each symlink in the path, leading to MAXSYMLINKS times PATH_MAX
maximum stack usage.
The test create a symlink with __eloop_threshold() loops and creates
a thread with minimum stack size (obtained through
support_small_stack_thread_attribute). The thread issues a stack
allocations that fill the thread allocated stack minus some slack
plus and the realpath usage (which assumes a bounded stack usage).
If realpath uses more than aboud 2 * PATH_MAX plus some slack it
trigger a stackoverflow.
Checked on x86_64-linux-gnu and i686-linux-gnu.
---
stdlib/Makefile | 3 +-
stdlib/tst-canon-bz26341.c | 99 ++++++++++++++++++++++++++++++++++++++
2 files changed, 101 insertions(+), 1 deletion(-)
create mode 100644 stdlib/tst-canon-bz26341.c
diff --git a/stdlib/Makefile b/stdlib/Makefile
index 29b7cd7071..6518d8993b 100644
--- a/stdlib/Makefile
+++ b/stdlib/Makefile
@@ -86,7 +86,7 @@ tests := tst-strtol tst-strtod testmb testrand testsort testdiv \
tst-makecontext-align test-bz22786 tst-strtod-nan-sign \
tst-swapcontext1 tst-setcontext4 tst-setcontext5 \
tst-setcontext6 tst-setcontext7 tst-setcontext8 \
- tst-setcontext9 tst-bz20544
+ tst-setcontext9 tst-bz20544 tst-canon-bz26341
tests-internal := tst-strtod1i tst-strtod3 tst-strtod4 tst-strtod5i \
tst-tls-atexit tst-tls-atexit-nodelete
@@ -101,6 +101,7 @@ LDLIBS-test-atexit-race = $(shared-thread-library)
LDLIBS-test-at_quick_exit-race = $(shared-thread-library)
LDLIBS-test-cxa_atexit-race = $(shared-thread-library)
LDLIBS-test-on_exit-race = $(shared-thread-library)
+LDLIBS-tst-canon-bz26341 = $(shared-thread-library)
LDLIBS-test-dlclose-exit-race = $(shared-thread-library) $(libdl)
LDFLAGS-test-dlclose-exit-race = $(LDFLAGS-rdynamic)
diff --git a/stdlib/tst-canon-bz26341.c b/stdlib/tst-canon-bz26341.c
new file mode 100644
index 0000000000..e0426ab306
--- /dev/null
+++ b/stdlib/tst-canon-bz26341.c
@@ -0,0 +1,99 @@
+/* Check if realpath does not consume extra stack space based on symlink
+ existance in the path (BZ #26341)
+ Copyright (C) 2020 Free Software Foundation, Inc.
+ This file is part of the GNU C Library.
+
+ The GNU C Library is free software; you can redistribute it and/or
+ modify it under the terms of the GNU Lesser General Public
+ License as published by the Free Software Foundation; either
+ version 2.1 of the License, or (at your option) any later version.
+
+ The GNU C Library is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public
+ License along with the GNU C Library; if not, see
+ <https://www.gnu.org/licenses/>. */
+
+#include <stdlib.h>
+#include <string.h>
+#include <sys/param.h>
+#include <unistd.h>
+
+#define __sysconf sysconf
+#include <eloop-threshold.h>
+#include <support/check.h>
+#include <support/support.h>
+#include <support/temp_file.h>
+#include <support/xunistd.h>
+#include <support/xthread.h>
+
+static char *filename;
+static size_t filenamelen;
+static char *linkname;
+
+#ifndef PATH_MAX
+# define PATH_MAX 1024
+#endif
+
+static void
+create_link (void)
+{
+ int fd = create_temp_file ("tst-canon-bz26341", &filename);
+ TEST_VERIFY_EXIT (fd != -1);
+ xclose (fd);
+
+ char *prevlink = filename;
+ int maxlinks = __eloop_threshold ();
+ for (int i = 0; i < maxlinks; i++)
+ {
+ linkname = xasprintf ("%s%d", filename, i);
+ xsymlink (prevlink, linkname);
+ add_temp_file (linkname);
+ prevlink = linkname;
+ }
+
+ filenamelen = strlen (filename);
+}
+
+static void *
+do_realpath (void *arg)
+{
+ /* Old implementation of realpath allocates a PATH_MAX using alloca
+ for each symlink in the path, leading to MAXSYMLINKS times PATH_MAX
+ maximum stack usage.
+ This stack allocations tries fill the thread allocated stack minus
+ both the resolved path (plus some slack) and the realpath (plus some
+ slack).
+ If realpath uses more than 2 * PATH_MAX plus some slack it will trigger
+ a stackoverflow. */
+
+ const size_t realpath_usage = 2 * PATH_MAX + 1024;
+ const size_t thread_usage = 1 * PATH_MAX + 1024;
+ size_t stack_size = support_small_thread_stack_size ()
+ - realpath_usage - thread_usage;
+ char stack[stack_size];
+ char *resolved = stack + stack_size - thread_usage + 1024;
+
+ char *p = realpath (linkname, resolved);
+ TEST_VERIFY (p != NULL);
+ TEST_COMPARE_BLOB (resolved, filenamelen, filename, filenamelen);
+
+ return NULL;
+}
+
+static int
+do_test (void)
+{
+ create_link ();
+
+ pthread_t th = xpthread_create (support_small_stack_thread_attribute (),
+ do_realpath, NULL);
+ xpthread_join (th);
+
+ return 0;
+}
+
+#include <support/test-driver.c>
--
2.25.1
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v3 6/6] stdlib: Add testcase fro BZ #26241
2020-12-29 19:34 ` [PATCH v3 6/6] stdlib: Add testcase fro BZ #26241 Adhemerval Zanella
@ 2021-01-20 4:33 ` DJ Delorie
2021-01-20 14:13 ` Adhemerval Zanella
0 siblings, 1 reply; 18+ messages in thread
From: DJ Delorie @ 2021-01-20 4:33 UTC (permalink / raw)
To: Adhemerval Zanella; +Cc: libc-alpha, eggert, bug-gnulib
Adhemerval Zanella via Libc-alpha <libc-alpha@sourceware.org> writes:
> - tst-setcontext9 tst-bz20544
> + tst-setcontext9 tst-bz20544 tst-canon-bz26341
New test, ok.
> +LDLIBS-tst-canon-bz26341 = $(shared-thread-library)
Ok.
> diff --git a/stdlib/tst-canon-bz26341.c b/stdlib/tst-canon-bz26341.c
> +/* Check if realpath does not consume extra stack space based on symlink
> + existance in the path (BZ #26341)
Is this allowed to be two lines?
> + Copyright (C) 2020 Free Software Foundation, Inc.
Year is wrong now :-)
> +#include <stdlib.h>
> +#include <string.h>
> +#include <sys/param.h>
> +#include <unistd.h>
Ok
> +#define __sysconf sysconf
> +#include <eloop-threshold.h>
> +#include <support/check.h>
> +#include <support/support.h>
> +#include <support/temp_file.h>
> +#include <support/xunistd.h>
> +#include <support/xthread.h>
Ok
> +static char *filename;
> +static size_t filenamelen;
> +static char *linkname;
> +
> +#ifndef PATH_MAX
> +# define PATH_MAX 1024
> +#endif
Ok.
> +static void
> +create_link (void)
> +{
> + int fd = create_temp_file ("tst-canon-bz26341", &filename);
> + TEST_VERIFY_EXIT (fd != -1);
> + xclose (fd);
> +
> + char *prevlink = filename;
> + int maxlinks = __eloop_threshold ();
> + for (int i = 0; i < maxlinks; i++)
> + {
> + linkname = xasprintf ("%s%d", filename, i);
> + xsymlink (prevlink, linkname);
linkname -> prevlink -> filename
> + add_temp_file (linkname);
> + prevlink = linkname;
> + }
> +
> + filenamelen = strlen (filename);
> +}
On exit, linkname has the last link created. Needs a comment to that
effect.
> +static void *
> +do_realpath (void *arg)
> +{
> + /* Old implementation of realpath allocates a PATH_MAX using alloca
> + for each symlink in the path, leading to MAXSYMLINKS times PATH_MAX
> + maximum stack usage.
> + This stack allocations tries fill the thread allocated stack minus
> + both the resolved path (plus some slack) and the realpath (plus some
> + slack).
> + If realpath uses more than 2 * PATH_MAX plus some slack it will trigger
> + a stackoverflow. */
> +
> + const size_t realpath_usage = 2 * PATH_MAX + 1024;
> + const size_t thread_usage = 1 * PATH_MAX + 1024;
> + size_t stack_size = support_small_thread_stack_size ()
> + - realpath_usage - thread_usage;
> + char stack[stack_size];
> + char *resolved = stack + stack_size - thread_usage + 1024;
This points us at PATH_MAX away from the end of stack[]. Ok. Also
forces most of the stack to get used up :-)
> + char *p = realpath (linkname, resolved);
We assume the test will crash if we use more stack than we allocated.
> + TEST_VERIFY (p != NULL);
realpath() must succeed, ok
> + TEST_COMPARE_BLOB (resolved, filenamelen, filename, filenamelen);
And give us the right result, ok
> + return NULL;
> +}
> +
> +static int
> +do_test (void)
> +{
> + create_link ();
> +
> + pthread_t th = xpthread_create (support_small_stack_thread_attribute (),
> + do_realpath, NULL);
> + xpthread_join (th);
> +
> + return 0;
> +}
Run the test in a thread with a small stack, ok.
> +#include <support/test-driver.c>
LGTM with that comment.
Reviewed-by: DJ Delorie <dj@redhat.com>
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v3 6/6] stdlib: Add testcase fro BZ #26241
2021-01-20 4:33 ` DJ Delorie
@ 2021-01-20 14:13 ` Adhemerval Zanella
0 siblings, 0 replies; 18+ messages in thread
From: Adhemerval Zanella @ 2021-01-20 14:13 UTC (permalink / raw)
To: DJ Delorie; +Cc: libc-alpha, eggert, bug-gnulib
On 20/01/2021 01:33, DJ Delorie wrote:
>
> Adhemerval Zanella via Libc-alpha <libc-alpha@sourceware.org> writes:
>> - tst-setcontext9 tst-bz20544
>> + tst-setcontext9 tst-bz20544 tst-canon-bz26341
>
> New test, ok.
>
>> +LDLIBS-tst-canon-bz26341 = $(shared-thread-library)
>
> Ok.
>
>> diff --git a/stdlib/tst-canon-bz26341.c b/stdlib/tst-canon-bz26341.c
>
>> +/* Check if realpath does not consume extra stack space based on symlink
>> + existance in the path (BZ #26341)
>
> Is this allowed to be two lines?
Good question. GNU code guidelines does not really restrict a number of
line, it is just says
Also, please write a brief comment at the start of each source file,
with the file name and a line or two about the overall purpose of the file.
We do have other file with two line comments, so I think it should be
fine.
>
>> + Copyright (C) 2020 Free Software Foundation, Inc.
>
> Year is wrong now :-)
Ack.
>
>> +#include <stdlib.h>
>> +#include <string.h>
>> +#include <sys/param.h>
>> +#include <unistd.h>
>
> Ok
>
>> +#define __sysconf sysconf
>> +#include <eloop-threshold.h>
>> +#include <support/check.h>
>> +#include <support/support.h>
>> +#include <support/temp_file.h>
>> +#include <support/xunistd.h>
>> +#include <support/xthread.h>
>
> Ok
>
>> +static char *filename;
>> +static size_t filenamelen;
>> +static char *linkname;
>> +
>> +#ifndef PATH_MAX
>> +# define PATH_MAX 1024
>> +#endif
>
> Ok.
>
>> +static void
>> +create_link (void)
>> +{
>> + int fd = create_temp_file ("tst-canon-bz26341", &filename);
>> + TEST_VERIFY_EXIT (fd != -1);
>> + xclose (fd);
>> +
>> + char *prevlink = filename;
>> + int maxlinks = __eloop_threshold ();
>> + for (int i = 0; i < maxlinks; i++)
>> + {
>> + linkname = xasprintf ("%s%d", filename, i);
>> + xsymlink (prevlink, linkname);
>
> linkname -> prevlink -> filename
>
>> + add_temp_file (linkname);
>> + prevlink = linkname;
>> + }
>> +
>> + filenamelen = strlen (filename);
>> +}
>
> On exit, linkname has the last link created. Needs a comment to that
> effect.
I added prior the link creations.
/* Create MAXLINKS symbolic links to the temporary filename.
On exit, linkname has the last link created. */
>
>
>> +static void *
>> +do_realpath (void *arg)
>> +{
>> + /* Old implementation of realpath allocates a PATH_MAX using alloca
>> + for each symlink in the path, leading to MAXSYMLINKS times PATH_MAX
>> + maximum stack usage.
>> + This stack allocations tries fill the thread allocated stack minus
>> + both the resolved path (plus some slack) and the realpath (plus some
>> + slack).
>> + If realpath uses more than 2 * PATH_MAX plus some slack it will trigger
>> + a stackoverflow. */
>> +
>> + const size_t realpath_usage = 2 * PATH_MAX + 1024;
>> + const size_t thread_usage = 1 * PATH_MAX + 1024;
>> + size_t stack_size = support_small_thread_stack_size ()
>> + - realpath_usage - thread_usage;
>> + char stack[stack_size];
>> + char *resolved = stack + stack_size - thread_usage + 1024;
>
> This points us at PATH_MAX away from the end of stack[]. Ok. Also
> forces most of the stack to get used up :-)
>
>> + char *p = realpath (linkname, resolved);
>
> We assume the test will crash if we use more stack than we allocated.
>
>> + TEST_VERIFY (p != NULL);
>
> realpath() must succeed, ok
>
>> + TEST_COMPARE_BLOB (resolved, filenamelen, filename, filenamelen);
>
> And give us the right result, ok
>
>> + return NULL;
>> +}
>> +
>> +static int
>> +do_test (void)
>> +{
>> + create_link ();
>> +
>> + pthread_t th = xpthread_create (support_small_stack_thread_attribute (),
>> + do_realpath, NULL);
>> + xpthread_join (th);
>> +
>> + return 0;
>> +}
>
> Run the test in a thread with a small stack, ok.
>
>> +#include <support/test-driver.c>
>
> LGTM with that comment.
>
> Reviewed-by: DJ Delorie <dj@redhat.com>
>
[1] https://www.gnu.org/prep/standards/html_node/Comments.html
^ permalink raw reply [flat|nested] 18+ messages in thread