Re: new *z*printf-gnu modules
I wrote: > This is more hairy than you might think. Because of the GCC attribute syntax. > > stdio.in.h does > > /* Don't break __attribute__((format(printf,M,N))). */ > #define printf __printf__ > > because there are not many symbols that can be used in place of 'printf'. > > Do you have an idea for doing this nicely? #define printf(fmt,...) zprintf(fmt,__VA_ARGS__) This is good enough, because no one uses printf as a function pointer. Bruno
Re: Integer overflows in memchr
Hi,
Po Lu wrote:
> It has been brought to my attention that a defective memchr is
> distributed with certain Android E-book readers with Android 2.3.5 that
> is liable to integer overflows with large values of N:
>
> memchr ("", 0, SIZE_MAX);
This memchr invocation is wrong. memchr is defined in ISO C23 § 7.26.5.2
"The memchr generic function locates the first occurrence of c (...)
in the initial n characters (...) of the object pointed to by s."
The term "object" being defined in § 3.15:
"region of data storage in the execution environment, the contents
of which can represent values"
If you call memchr (s, c, n) where n > - (uintptr_t) s, you are pretending
that the NULL pointer is part of a region of data storage, which is wrong.
Similarly, if you call memchr (s, c, n) where n == - (uintptr_t) s, you are
pretending that the address (uintptr_t)(-1) is part of a region of data
storage, which is impossible on all OSes we know of.
Thus, it is the application's duty to provide a sane value for n. The
following code will usually avoid the overflow (although it still violates
the specification):
const char *s = "";
size_t n = SIZE_MAX;
if (n > ~(uintptr_t)s)
n = ~(uintptr_t)s;
memchr (s, 0, n);
> This extends to
> affect strnlen, which is implemented around memchr, ...
> producing crashes in this function in Emacs:
>
> static int
> store_mode_line_string (const char *string, Lisp_Object lisp_string,
> bool copy_string,
> int field_width, int precision, Lisp_Object props)
> {
> ptrdiff_t len;
> int n = 0;
>
> if (string != NULL)
> {
> len = strnlen (string, precision <= 0 ? SIZE_MAX : precision);
Similarly here. strnlen is specified by POSIX
https://pubs.opengroup.org/onlinepubs/9699919799/functions/strlen.html
"The strnlen() function shall compute the smaller of the number of bytes
in the array to which s points, not including any terminating NUL character,
or the value of the maxlen argument. The strnlen() function shall never
examine more than maxlen bytes of the array pointed to by s."
In other words, if precision is 0, you are allowing the implementation to
look at SIZE_MAX bytes, which includes the NULL pointer. Thus the
implementation is allowed to crash.
> Perhaps Gnulib
> should provide a module with replacements for memchr and affected string
> functions, viz., strnlen, memmem, wmemchr, mbstowcs, mbsrtowcs,
> wcsrtombs, and wcstombs.
I think it is better if the Emacs code gets fixed. This will also eliminate
warnings from static analysis tools (e.g. "gcc -fanalyzer") that routinely
complain about an overflow when you pass SIZE_MAX as an object size or array
size.
Bruno
Re: Integer overflows in memchr
Bruno Haible writes: > This memchr invocation is wrong. memchr is defined in ISO C23 § 7.26.5.2 > "The memchr generic function locates the first occurrence of c (...) >in the initial n characters (...) of the object pointed to by s." > The term "object" being defined in § 3.15: > "region of data storage in the execution environment, the contents >of which can represent values" > > If you call memchr (s, c, n) where n > - (uintptr_t) s, you are pretending > that the NULL pointer is part of a region of data storage, which is wrong. > > Similarly, if you call memchr (s, c, n) where n == - (uintptr_t) s, you are > pretending that the address (uintptr_t)(-1) is part of a region of data > storage, which is impossible on all OSes we know of. > > Thus, it is the application's duty to provide a sane value for n. The > following code will usually avoid the overflow (although it still violates > the specification): > >const char *s = ""; >size_t n = SIZE_MAX; >if (n > ~(uintptr_t)s) > n = ~(uintptr_t)s; >memchr (s, 0, n); Does this not imply that Android's strnlen implementation is incorrect? > Similarly here. strnlen is specified by POSIX > https://pubs.opengroup.org/onlinepubs/9699919799/functions/strlen.html > "The strnlen() function shall compute the smaller of the number of bytes >in the array to which s points, not including any terminating NUL > character, >or the value of the maxlen argument. The strnlen() function shall never >examine more than maxlen bytes of the array pointed to by s." > In other words, if precision is 0, you are allowing the implementation to > look at SIZE_MAX bytes, which includes the NULL pointer. Thus the > implementation is allowed to crash. I believe that the semantics of the POSIX specification of this GNU function omit the implied guarantee that strnlen will never examine bytes beyond the first null byte, made in (libc)String Length: If the array S of size MAXLEN contains a null byte, the ‘strnlen’ function returns the length of the string S in bytes. Otherwise it returns MAXLEN. Therefore this function is equivalent to ‘(strlen -- (S) < MAXLEN ? strlen (S) : MAXLEN)’ but it is more efficient and --- works even if S is not null-terminated so long as MAXLEN does not exceed the size of S's array. char string[32] = "hello, world"; strnlen (string, 32) ⇒ 12 strnlen (string, 5) ⇒ 5 This function is a GNU extension and is declared in ‘string.h’. assuming that the proviso that follows the highlighted passage is only applicable to the case where S is not NULL-terminated as Emacs's strings are.
Re: idpriv-drop: Handle large user and group ids.
Hi Collin, > I've committed these two patches adjusting idpriv-drop and > idpriv-droptemp which previously stored the result of getuid () and > getgid () in an 'int'. The first patch is obviously fine. The second patch makes the assumption that uid_t and gid_t are at least 32-bits wide. I had written the code that way because on some systems (SunOS 4 IIRC, older IRIX, and Linux [1]) uid_t and gid_t had been 'unsigned short', and thus a comparison of an 'unsigned short' with -1 always returned false. But nowadays, for more than 20 years, they are 32-bit. > For other modules where an ID is used as the result to printf it is > often casted to an int. Should I change those to intmax_t? I feel like > that would be the most portable way. This would be overkill. uid_t changed from 16 bits to 32 bits because it had been a practical problem: some internet providers provided shell access to servers to more than 65000 customers. There is no practical problem with 32 bits; no one needs more than 2 billion users or groups on a single machine. Cf. also the accepted answer of [2]. There's no point in addressing hypothetical problems that most likely will never exist. Bruno [1] https://docs.kernel.org/admin-guide/highuid.html [2] https://stackoverflow.com/questions/20533606/
Re: gnulib macro to check for PIC for a dependent library?
On Tue, Jun 25, 2024 at 06:46:04PM +0200, Bruno Haible wrote:
> > Building shared libraries which depend upon
> > other libraries breaks whenever a dependency found (by something like
> > AC_SEARCH_LIBS) was not built with -fPIC (or whatever
> > the name of this option might be on the platform in question).
> > And, preferably, one might want to check that the dependency is
> > a dynamic library itself.
> >
> > Are there any gnulib (or autoconf) macros available which can help here?
>
> None that I know of.
>
> You might get better feedback from a libtool mailing list than from here.
> But my understanding is that
>
> 1) Static libraries are generally built without -fPIC. Building static
>libraries with -fPIC was commonly done in the 1990ies, because ELF
>and its dependency mechanisms were not in use everywhere at that time.
>Since the time shared libraries work fine, there is hardly any use any
>more to position-independent static code (except, of course, on systems
>where everything is compiled with -fPIE).
>
> 2) libtool adds an option --with-pic to the generated configure script of a
>package. But hardly anyone makes use of it.
>
> 3) Many packages come only with a shared library, not with a .a file any
>more. See:
>$ ll /usr/lib/x86_64-linux-gnu/lib*.so | wc -l
>422
>$ ll /usr/lib/x86_64-linux-gnu/lib*.a | wc -l
>207
>
>The reason is that distros prefer shared libraries: When there is a
>bug fix to a package, they have to rebuild that one package and not
>also a dozen of other packages.
>
> 4) On x86, it was possible to put non-PIC code into a shared library, and
>that worked (albeit with a performance hit). On x86_64, this does not
>work any more. And despite of that, the fact that most static libraries
>are not built with -fPIC does not hurt anyone (apparently).
>
> > Thinking of writing such a macro myself
>
> I think it would not really be useful. Your time is better spent in making
> all packages that currently ship only a static library ship a shared library
> as well.
Unfortunately there are cases where maintainers have building shared
libs turned off by default, and do not provide any meaningful way to query
the configuration. (Typically they are not using autotools or cmake or
anything which makes building shared libraries easy)
And they won't consider changing this :-(
As a result we (SageMath) get complaints such as: "I built such and such
library myself, with default settings, and installed it in /usr/local, and now
I get that
'recompile with -fPIC enabled' error popping out mid-build".
Would such a macro be not much more than an appropriately wrapped call
to dlopen() ? (We don't do Windows, so the latter is not an obstacle)
As I matter of fact, I wrote a while ago a more complicated macro, where
I extract the absolute path to a library. We ended up not using it.
AC_DEFUN([SAGE_ABSOLUTE_LIB], [
dnl
dnl ARG1 - the name of the dynamic library to find absolute name of
dnl ARG2 - a function in the library to test for
dnl
AC_MSG_CHECKING([[absolute path to lib]$1[...]])
AC_LANG_PUSH(C)
ABS_LIB_SAVED_LIBS=$LIBS
LIBS="[-l]$1[ -ldl]"
AC_RUN_IFELSE([
AC_LANG_PROGRAM(
[[
#include
#define __USE_GNU
#include
#ifdef __APPLE__
#define EXT "dylib"
#else
#define EXT "so"
#endif
]],
dnl run as $ cc -o conftest conftest.c -lARG1 -ldl && ./conftest
dnl output should be like /usr/lib/x86_64-linux-gnu/libARG1.so
[[ /* int main()... */
Dl_info info;
int res;
void *ha;
void *z = dlopen("lib$1."EXT, RTLD_LAZY); /* load libARG1 */
if (z) {
ha = dlsym(z, "$2"); /* get address of inflateEnd in libz
*/
res = dladdr(ha, &info); /* get info for the function
*/
printf("%s\n", info.dli_fname);
return 0; /* dladdr return value on success is
platform-dependent */
}
printf("dlopen() call failed!\n");
return 1;
]])], [
[computed_]$1[libdir]=`./conftest$EXEEXT`
AC_MSG_RESULT([ got it: "$[computed_]$1[libdir]"])
], [
AC_MSG_RESULT([ failure.])
[computed_]$1[libdir]="/usr/lib"dnl a pretty random choice
])
LIBS=$ABS_LIB_SAVED_LIBS
AC_LANG_POP(C)
])
Dima
>
> Bruno
>
>
>
>
signature.asc
Description: PGP signature
Re: Integer overflows in memchr
Po Lu wrote: > I believe that the semantics of the POSIX specification of this GNU > function omit the implied guarantee that strnlen will never examine > bytes beyond the first null byte There is no such guarantee, not even implied. > , made in (libc)String Length: > > If the array S of size MAXLEN contains a null byte, the ‘strnlen’ ^^ > function returns the length of the string S in bytes. Otherwise it > returns MAXLEN. When the text says "the array S of size MAXLEN", it means that the bytes S[0], S[1], ..., S[MAXLEN-1] must be accessible. Which is not the case if you pass MAXLEN as > ~(uintptr_t)S. The implementation could, for example, examine S[0], S[MAXLEN-1], S[1], S[MAXLEN-2], ... in this order and thus achieve the "more efficient" statement. > Does this not imply that Android's strnlen implementation is incorrect? Android's strnlen [1] is not incorrect, because the same requirements that hold for memchr also hold for strnlen. Bruno [1] https://android.googlesource.com/platform/bionic.git/+/refs/heads/main/libc/bionic/strnlen.cpp
Re: Integer overflows in memchr
Bruno Haible writes:
> When the text says "the array S of size MAXLEN", it means that the bytes
> S[0], S[1], ..., S[MAXLEN-1] must be accessible. Which is not the case if
> you pass MAXLEN as > ~(uintptr_t)S.
>
> The implementation could, for example, examine
> S[0], S[MAXLEN-1], S[1], S[MAXLEN-2], ...
> in this order and thus achieve the "more efficient" statement.
Then the libc documentation is self-contradictory, because this is not
even remotely equivalent to (strlen (S) < MAXLEN ? strlen (S) : MAXLEN),
not to mention inconsistent with established usage, in Emacs no less
than in Gnulib and glibc themselves. lib/fnmatch.c:
int
fnmatch (const char *pattern, const char *string, int flags)
{
if (__glibc_unlikely (MB_CUR_MAX != 1))
{
mbstate_t ps;
size_t n;
const char *p;
WCHAR_T *wpattern_malloc = NULL;
WCHAR_T *wpattern;
WCHAR_T *wstring_malloc = NULL;
WCHAR_T *wstring;
size_t alloca_used = 0;
/* Convert the strings into wide characters. */
memset (&ps, '\0', sizeof (ps));
p = pattern;
n = strnlen (pattern, 1024);
What guarantees that all 1024 bytes subsequent to PATTERN will be
accessible?
Re: Integer overflows in memchr
On 6/26/24 07:57, Bruno Haible wrote:
Po Lu wrote:
I believe that the semantics of the POSIX specification of this GNU
function omit the implied guarantee that strnlen will never examine
bytes beyond the first null byte
There is no such guarantee, not even implied.
There seems to be some confusion here. Here's what POSIX.1-2024 says:
"The strnlen() function shall compute the smaller of the number of bytes
in the array to which s points, not including any terminating NUL
character, or the value of the maxlen argument. The strnlen() function
shall never examine more than maxlen bytes of the array pointed to by s."
This means it's OK to call strnlen ("", SIZE_MAX), because the second
arg of strnlen can be larger than the number of bytes in the byte array
that the first arg points to, so long as that array contains a null
byte. strnlen is unusual in this sense.
In contrast, it's not OK to call memchr ("", 0, SIZE_MAX).
(libc)String Length:
If the array S of size MAXLEN contains a null byte, the ‘strnlen’
^^
That's a bug in the glibc manual, which I just now fixed as noted here:
https://sourceware.org/pipermail/libc-alpha/2024-June/157798.html
Android's strnlen [1] is not incorrect, because the same requirements
that hold for memchr also hold for strnlen.
[1] https://android.googlesource.com/platform/bionic.git/+/refs/heads/main/libc/bionic/strnlen.cpp
Actually it's not incorrect because Android's memchr no longer has
undefined behavior with calls like memchr ("", 0, SIZE_MAX); it reliably
returns its first argument in that particular example. Although this
goes beyond what C and POSIX require for memchr, it's OK for Android
memchr to do that as an extension, and it's OK for Android's strnlen to
rely on Android's extensions to memchr.
I installed the attached patch to document the Android 5.0 glitch. Since
Android 5.0 is no longer supported I hope we don't need to worry about
this in Gnulib. That being said, if it's important for free software to
run on these old unsupported platforms and if someone wants to update
the Gnulib strnlen module to work around this Android 5.0 bug, I would
think that'd be OK.
From b79238db4ac7b8e710c8cab4307ce6cb1c3937d8 Mon Sep 17 00:00:00 2001
From: Paul Eggert
Date: Wed, 26 Jun 2024 16:14:26 +0100
Subject: [COMMITTED] Fix strnlen doc re array size
* manual/string.texi: For strnlen (s, maxlen), do not say that s must
be of size maxlen, as it can be smaller if it is null-terminated.
This should help avoid confusion such as seen in
.
Mention that strnlen and wcsnlen have been in POSIX since
POSIX.1-2008.
---
manual/string.texi | 17 ++---
1 file changed, 10 insertions(+), 7 deletions(-)
diff --git a/manual/string.texi b/manual/string.texi
index b91299fd6a..0b667bd3fb 100644
--- a/manual/string.texi
+++ b/manual/string.texi
@@ -309,12 +309,12 @@ This function was introduced in @w{Amendment 1} to @w{ISO C90}.
@end deftypefun
@deftypefun size_t strnlen (const char *@var{s}, size_t @var{maxlen})
-@standards{GNU, string.h}
+@standards{POSIX.1, string.h}
@safety{@prelim{}@mtsafe{}@assafe{}@acsafe{}}
-If the array @var{s} of size @var{maxlen} contains a null byte,
-the @code{strnlen} function returns the length of the string @var{s} in
-bytes. Otherwise it
-returns @var{maxlen}. Therefore this function is equivalent to
+This returns the offset of the first null byte in the array @var{s},
+except that it returns @var{maxlen} if the first @var{maxlen} bytes
+are all non-null.
+Therefore this function is equivalent to
@code{(strlen (@var{s}) < @var{maxlen} ? strlen (@var{s}) : @var{maxlen})}
but it
is more efficient and works even if @var{s} is not null-terminated so
@@ -328,7 +328,9 @@ strnlen (string, 5)
@result{} 5
@end smallexample
-This function is a GNU extension and is declared in @file{string.h}.
+This function is part of POSIX.1-2008 and later editions, but was
+available in @theglibc{} and other systems as an extension long before
+it was standardized. It is declared in @file{string.h}.
@end deftypefun
@deftypefun size_t wcsnlen (const wchar_t *@var{ws}, size_t @var{maxlen})
@@ -337,7 +339,8 @@ This function is a GNU extension and is declared in @file{string.h}.
@code{wcsnlen} is the wide character equivalent to @code{strnlen}. The
@var{maxlen} parameter specifies the maximum number of wide characters.
-This function is a GNU extension and is declared in @file{wchar.h}.
+This function is part of POSIX.1-2008 and later editions, and is
+declared in @file{wchar.h}.
@end deftypefun
@node Copying Strings and Arrays
--
2.45.2
Re: Integer overflows in memchr
On 6/26/24 12:33, Paul Eggert wrote:
I installed the attached patch to document the Android 5.0 glitch
Oops, attached the wrong patch. Here's the patch I meant to attach.From a03728ed45b8fae66fb30aa808dde76e4d57d865 Mon Sep 17 00:00:00 2001
From: Paul Eggert
Date: Wed, 26 Jun 2024 17:16:06 +0100
Subject: [PATCH] strnlen: document Android bug
* doc/posix-functions/strnlen.texi (strnlen):
Mention Android 5.0 bug reported by Po Lu in this thread:
https://lists.gnu.org/r/bug-gnulib/2024-06/msg00271.html
---
ChangeLog| 7 +++
doc/posix-functions/strnlen.texi | 5 +
2 files changed, 12 insertions(+)
diff --git a/ChangeLog b/ChangeLog
index 05bec51810..c0526832f6 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,10 @@
+2024-06-26 Paul Eggert
+
+ strnlen: document Android bug
+ * doc/posix-functions/strnlen.texi (strnlen):
+ Mention Android 5.0 bug reported by Po Lu in this thread:
+ https://lists.gnu.org/r/bug-gnulib/2024-06/msg00271.html
+
2024-06-25 Collin Funk
idpriv-droptemp: Handle large user and group ids.
diff --git a/doc/posix-functions/strnlen.texi b/doc/posix-functions/strnlen.texi
index 26806fbb58..e305a41c8c 100644
--- a/doc/posix-functions/strnlen.texi
+++ b/doc/posix-functions/strnlen.texi
@@ -15,4 +15,9 @@ Mac OS X 10.5, FreeBSD 6.0, NetBSD 5.0, OpenBSD 3.8, HP-UX 11, Solaris 10, mingw
Portability problems not fixed by Gnulib:
@itemize
+@item
+On some platforms, calls like @code{strnlen (s, maxlen)} can crash if
+@var{s} is null-terminated but address arithmetic overflows
+(i.e., @code{s + maxlen < s}):
+Android 5.0.
@end itemize
--
2.34.1
Re: idpriv-drop: Handle large user and group ids.
On Wed, 26 Jun 2024, Bruno Haible wrote: > The second patch makes the assumption that uid_t and gid_t are > at least 32-bits wide. I had written the code that way because on some > systems (SunOS 4 IIRC, older IRIX, and Linux [1]) uid_t and gid_t > had been 'unsigned short', and thus a comparison of an 'unsigned short' > with -1 always returned false. But nowadays, for more than 20 years, > they are 32-bit. OpenServer 5 uses 'unsigned short'. ... tim@timosr5d 6% egrep 'gid_t|uid_t' /usr/include/sys/types.h typedef unsigned short uid_t; typedef unsigned short gid_t; ... Still a current product. > > Bruno > > [1] https://docs.kernel.org/admin-guide/highuid.html > [2] https://stackoverflow.com/questions/20533606/ -- Tim RiceMultitalents t...@multitalents.net
Re: SCO OpenServer
Tim Rice wrote: > OpenServer 5 uses 'unsigned short'. > ... > tim@timosr5d 6% egrep 'gid_t|uid_t' /usr/include/sys/types.h > typedef unsigned short uid_t; > typedef unsigned short gid_t; > ... > > Still a current product. SCO OpenServer 5 is not supported by Gnulib [1]. Adding support for SCO OpenServer 5 to any Gnulib module would make no sense, since SCO OpenServer 5 is a museum system, as you can see - from the hardware system requirements [2]: USB 3 not supported, screen widths of 1920 not supported, etc. - from the software list [3]: Java 1.3.1 (out of support since 2006). So, Collin, no need to revise your second patch. Bruno [1] https://www.gnu.org/software/gnulib/manual/html_node/Supported-Platforms.html [2] https://www.sco.com/products/openserver507/System_Requirements.html [3] https://www.sco.com/products/openserver507/ [4] https://en.wikipedia.org/wiki/Java_version_history
Re: SCO OpenServer
On Wed, 26 Jun 2024, Bruno Haible wrote: > Tim Rice wrote: > > OpenServer 5 uses 'unsigned short'. > > ... > > tim@timosr5d 6% egrep 'gid_t|uid_t' /usr/include/sys/types.h > > typedef unsigned short uid_t; > > typedef unsigned short gid_t; > > ... > > > > Still a current product. > > SCO OpenServer 5 is not supported by Gnulib [1]. Not surprising. It's a pain to work with. > Adding support for SCO OpenServer 5 to any Gnulib module would make no This was more for reference than asking to add for support for OpenServer 5. Many modules already support OpenServer 5 and if I need changes I'll provide patches. Just please do not remove any bits related to OpenServer 5 until Xinuos announces it is EOL. > sense, since SCO OpenServer 5 is a museum system, as you can see True, old. Sadly people still insist on using them to run their company. > - from the hardware system requirements [2]: > USB 3 not supported, screen widths of 1920 not supported, etc. > - from the software list [3]: Java 1.3.1 (out of support since 2006). Historical links, but you are right, for current hardware one would need to load VMware or something that runs VirtualBox. > So, Collin, no need to revise your second patch. Agreed. > Bruno > > [1] > https://www.gnu.org/software/gnulib/manual/html_node/Supported-Platforms.html > [2] https://www.sco.com/products/openserver507/System_Requirements.html > [3] https://www.sco.com/products/openserver507/ > [4] https://en.wikipedia.org/wiki/Java_version_history > -- Tim RiceMultitalents t...@multitalents.net
Re: Integer overflows in memchr
Paul Eggert writes:
> Actually it's not incorrect because Android's memchr no longer has
> undefined behavior with calls like memchr ("", 0, SIZE_MAX); it
> reliably returns its first argument in that particular
> example. Although this goes beyond what C and POSIX require for
> memchr, it's OK for Android memchr to do that as an extension, and
> it's OK for Android's strnlen to rely on Android's extensions to
> memchr.
>
> I installed the attached patch to document the Android 5.0
> glitch. Since Android 5.0 is no longer supported I hope we don't need
> to worry about this in Gnulib. That being said, if it's important for
> free software to run on these old unsupported platforms and if someone
> wants to update the Gnulib strnlen module to work around this Android
> 5.0 bug, I would think that'd be OK.
Google continue to support SDKs from 19 onwards in the standard support
library:
https://android-developers.googleblog.com/2023/10/androidx-minsdkversion-19.html
which is the real threshold at which official support ends. Granted,
Emacs supports versions still older than 4.4, e.g. 2.3.5 and 2.2, where
this bug was encountered by real users and reported.
Re: Integer overflows in memchr
Paul Eggert writes:
> On 6/26/24 12:33, Paul Eggert wrote:
>> I installed the attached patch to document the Android 5.0 glitch
>
> Oops, attached the wrong patch. Here's the patch I meant to attach.
Btw, doesn't this also mean that lib/strnlen.c (in Emacs, at least) is
incorrect?
size_t
strnlen (const char *string, size_t maxlen)
{
const char *end = memchr (string, '\0', maxlen);
return end ? (size_t) (end - string) : maxlen;
}
is virtually identical to Android's implementation and equally liable to
overflow in memchr with excessive falues of MAXLEN.
