Re: [PATCH v2 1/2] posix: Add compat glob symbol to not follow dangling symbols
I can confirm that make works properly with the compat glob symbol. Andreas. -- Andreas Schwab, SUSE Labs, sch...@suse.de GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7 "And now for something completely different."
Re: [libvirt] gnulib tests in libvirt broken by newer glibc 2.26
[adding gnulib]
On 09/27/2017 04:36 PM, Christian Ehrhardt wrote:
> Hi,
> there seems to be an incompatibility to the last glibc due to [1].
Gnulib needs to be updated to track the glibc changes (it looks like
that is actually under way already), then libvirt needs to pick up the
updated gnulib. I'll leave the rest of your email intact in case it
helps gnulib readers, but I'll probably help resolve the issue when I
get a chance.
I'm assuming you hit this failure on rawhide, as that is the platform
most likely to be using bleeding-edge glibc with the getopt changes?
>
> Eventually this breaks gnulib unittests (and maybe more).
> Debugging went from an assert, to bidngin different symbols, to changed
> function names to different header resolution.
>
> Because it expects it to behave "posixly" but arguments are changed
> differently.
> FAIL: test-getopt-posix
> ===
>
> ../../../../gnulib/tests/test-getopt.h:754: assertion 'strcmp (argv[1],
> "donald") == 0' failed
>
> # get and build latest libvirt to get the local gnulib
> $ wget http://libvirt.org/sources/libvirt-3.7.0.tar.xz
> $ tar xf libvirt-3.7.0.tar.xz
> $ cd libvirt
> $ apt build-dep libvirt
> $ ./autogen.sh
> $ make -j4
>
>
> You can run the following simplified test derived from the unit test (it is
> much shorter, so easier to debug e.g. in -E).
>
> $ cat << EOF >> test1.c
> #include
> #include
>
> #include
> #include
> #include
>
> int main (void)
> {
> return 0;
> }
> EOF
> $ gcc -I ./gnulib/lib -I . test1.c -H
>
> You can see in -H output already the difference in the paths of the headers
> that it uses:
>
> Glibc <2.26
> . ./config.h
> .. ./config-post.h
> . /usr/include/unistd.h
> [...]
> .. /usr/include/getopt.h
>
> Glibc >=2.26
> . ./config.h
> .. ./config-post.h
> . ./gnulib/lib/unistd.h
> [...]
>
> ... /usr/include/x86_64-linux-gnu/bits/getopt_posix.h
> /usr/include/x86_64-linux-gnu/bits/getopt_core.
>
>
> If you build with -E you'll also see that it now uses getopt from glibc
> instead of the prefixed rpl_getopt from gnulib.
>
> It behaves as if it would not find gnulib and instead fall back to glibc.
> But it finds gnulib, instead due to glibc's changes its implementation is
> fetched before and due to that pulling in glibc's behavior while the unit
> test is verifying against the one it expects from gnulib.
>
>
> Sorry, but I don't see the right fix here yet - I could easily silence the
> test but that is no fix. Especially if there might be implications due to
> handling the args (slightly) differently.
>
> I really wanted to come up with the same test against gnulib alone, but I
> was lost in the build system for too long and could not get the example to
> fail without libvirt (OTOH I'm sure it would).
>
> Therefore I'm reaching out to you for your help and experience on the build
> system what could be done.
>
> [1]: https://sourceware.org/ml/libc-alpha/2017-04/msg00115.html
>
>
>
> --
> libvir-list mailing list
> libvir-l...@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
new module 'strlcpy'
Paul Eggert wrote:
> @@ -48,7 +48,8 @@ get_locale_dependent_values (struct locale_dependent_values
> *result)
>snprintf (result->numeric, sizeof (result->numeric),
> "%g", 3.5);
>/* result->numeric is usually "3,5" */
> - strcpy (result->time, nl_langinfo (MON_1));
> + strncpy (result->time, nl_langinfo (MON_1), sizeof result->time - 1);
> + result->time[sizeof result->time - 1] = '\0';
>/* result->time is usually "janvier" */
> }
>
This change has replaced code with 1 drawback
- The string copy may overrun the buffer.
by code with 3 drawbacks
- The string copy may be silently truncated.
- The code needs 2 lines, instead of 1 line.
- In the common cases, the large result buffer gets needlessly filled
with NULs.
I think the best way to deal with this situation is the function 'strlcpy':
ASSERT (strlcpy (result->time, nl_langinfo (MON_1), sizeof result->time) <
sizeof result->time);
This way,
- The string copy will not overrun the buffer.
- The string copy will always be NUL-terminated.
- Silent truncation does not occur.
- The code fits in one line.
- The code is not needlessly inefficient.
Here's a proposal to add 'strlcpy' to gnulib.
Yes, I have read the relevant documentation:
https://www.freebsd.org/cgi/man.cgi?query=strlcpy&sektion=3
and the discussions:
https://www.sourceware.org/ml/libc-alpha/2000-08/msg00052.html
https://lwn.net/Articles/612244/
https://sourceware.org/ml/libc-alpha/2014-09/msg00350.html
https://sourceware.org/glibc/wiki/strlcpy
The major argument against strlcpy is that it is not fool-proof:
If the caller ignores the return value, silent truncation can occur.
To prevent this, the proposed patch declares strlcpy with
__attribute__((__warn_unused_result__)) on all platforms.
Bruno
>From dc4a8d880cb3be138b96b682ae57a10259a67ba4 Mon Sep 17 00:00:00 2001
From: Bruno Haible
Date: Thu, 28 Sep 2017 00:30:05 +0200
Subject: [PATCH 1/2] New module 'strlcpy'.
* lib/string.in.h (_GL_ATTRIBUTE_RETURN_CHECK): New macro.
(strlcpy): New declaration.
* lib/strlcpy.c: New file.
* m4/strlcpy.m4: New file.
* modules/strlcpy: New file.
* m4/string_h.m4 (gl_HEADER_STRING_H_BODY): Test whether strlcpy is
declared.
(gl_HEADER_STRING_H_DEFAULTS): Initialize GNULIB_STRLCPY, HAVE_STRLCPY,
REPLACE_STRLCPY.
* modules/string (Makefile.am): Substitite GNULIB_STRLCPY, HAVE_STRLCPY,
REPLACE_STRLCPY.
* doc/gnulib.texi (BSD Function Substitutes): New chapter.
* doc/bsd-functions/strlcpy.texi: New file.
---
ChangeLog | 17 +
doc/bsd-functions/strlcpy.texi | 18 +
doc/gnulib.texi| 27
lib/string.in.h| 46 ++
lib/strlcpy.c | 57 ++
m4/string_h.m4 | 17 +++--
m4/strlcpy.m4 | 22
modules/string | 15 ++-
modules/strlcpy| 26 +++
9 files changed, 232 insertions(+), 13 deletions(-)
create mode 100644 doc/bsd-functions/strlcpy.texi
create mode 100644 lib/strlcpy.c
create mode 100644 m4/strlcpy.m4
create mode 100644 modules/strlcpy
diff --git a/ChangeLog b/ChangeLog
index 8a9bbe6..c005f1c 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,20 @@
+2017-09-27 Bruno Haible
+
+ New module 'strlcpy'.
+ * lib/string.in.h (_GL_ATTRIBUTE_RETURN_CHECK): New macro.
+ (strlcpy): New declaration.
+ * lib/strlcpy.c: New file.
+ * m4/strlcpy.m4: New file.
+ * modules/strlcpy: New file.
+ * m4/string_h.m4 (gl_HEADER_STRING_H_BODY): Test whether strlcpy is
+ declared.
+ (gl_HEADER_STRING_H_DEFAULTS): Initialize GNULIB_STRLCPY, HAVE_STRLCPY,
+ REPLACE_STRLCPY.
+ * modules/string (Makefile.am): Substitite GNULIB_STRLCPY, HAVE_STRLCPY,
+ REPLACE_STRLCPY.
+ * doc/gnulib.texi (BSD Function Substitutes): New chapter.
+ * doc/bsd-functions/strlcpy.texi: New file.
+
2017-09-26 Bruno Haible
uniname/uniname-tests: Tighten code.
diff --git a/doc/bsd-functions/strlcpy.texi b/doc/bsd-functions/strlcpy.texi
new file mode 100644
index 000..5ce05dc
--- /dev/null
+++ b/doc/bsd-functions/strlcpy.texi
@@ -0,0 +1,18 @@
+@node strlcpy
+@subsection @code{strlcpy}
+@findex strlcpy
+
+Gnulib module: strlcpy
+
+Portability problems fixed by Gnulib:
+@itemize
+@item
+This function is missing on some platforms:
+glibc 2.24, AIX 5.1, HP-UX 11, OSF/1 5.1, mingw, MSVC 14, BeOS.
+@item
+Ignoring the return value of this function does not produce a warning.
+@end itemize
+
+Portability problems not fixed by Gnulib:
+@itemize
+@end itemize
diff --git a/doc/gnulib.texi b/doc/gnulib.texi
index 1468c14..6a52787 100644
--- a/doc/gnulib.texi
+++ b/doc/gnulib.texi
@@ -69,6 +69,7 @@ Documentation License''.
* Legacy Function Substitutes:: Replacing system functions.
* Glibc Header File Substitutes:: Overriding system headers.
* Glibc Function Substitutes:: Replacing system functions.
+*
Re: new module 'strlcpy'
I'd really rather not promote the use of strlcpy in GNU code, as it is
contrary to GNU style. Plus, I'm not a fan of __warn_unused_result__; in
my experience it's too often more trouble than it's worth, and I expect
this trouble would occur with strlcpy if we started using it.
That being said, I take your point that I should not have used strncpy
to pacify GCC in that test case: I plead guilty to being lazy because
the test-case code already has arbitrary limits.
Anyway, strlcpy is overkill here, as snprintf does the job just as well
here, and is already standard and supported by Gnulib. So I propose the
attached simpler patch instead.
>From 2513ff5a2d0cb11edbca47d5ad6b1b5ae0c690fa Mon Sep 17 00:00:00 2001
From: Paul Eggert
Date: Wed, 27 Sep 2017 16:16:43 -0700
Subject: [PATCH] duplocale-tests: use snprintf, not strncpy
* modules/duplocale-tests (Depends-on): Add snprintf.
* tests/test-duplocale.c (get_locale_dependent_values):
Use snprintf instead of strncpy to avoid overrunning
the fixed-size output buffer.
---
ChangeLog | 8
modules/duplocale-tests | 1 +
tests/test-duplocale.c | 4 ++--
3 files changed, 11 insertions(+), 2 deletions(-)
diff --git a/ChangeLog b/ChangeLog
index 8a9bbe625..7aa6c03d7 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,11 @@
+2017-09-27 Paul Eggert
+
+ duplocale-tests: use snprintf, not strncpy
+ * modules/duplocale-tests (Depends-on): Add snprintf.
+ * tests/test-duplocale.c (get_locale_dependent_values):
+ Use snprintf instead of strncpy to avoid overrunning
+ the fixed-size output buffer.
+
2017-09-26 Bruno Haible
uniname/uniname-tests: Tighten code.
diff --git a/modules/duplocale-tests b/modules/duplocale-tests
index baa9a6310..98f85b448 100644
--- a/modules/duplocale-tests
+++ b/modules/duplocale-tests
@@ -5,6 +5,7 @@ tests/macros.h
Depends-on:
langinfo
+snprintf
configure.ac:
AC_CHECK_FUNCS_ONCE([duplocale uselocale strfmon_l snprintf_l nl_langinfo_l])
diff --git a/tests/test-duplocale.c b/tests/test-duplocale.c
index f48fedf6a..c812f0b82 100644
--- a/tests/test-duplocale.c
+++ b/tests/test-duplocale.c
@@ -42,14 +42,14 @@ struct locale_dependent_values
static void
get_locale_dependent_values (struct locale_dependent_values *result)
{
+ size_t ntime = sizeof result->time;
strfmon (result->monetary, sizeof (result->monetary),
"%n", 123.75);
/* result->monetary is usually "$123.75" */
snprintf (result->numeric, sizeof (result->numeric),
"%g", 3.5);
/* result->numeric is usually "3,5" */
- strncpy (result->time, nl_langinfo (MON_1), sizeof result->time - 1);
- result->time[sizeof result->time - 1] = '\0';
+ ASSERT (snprintf (result->time, ntime, "%s", nl_langinfo (MON_1)) < ntime);
/* result->time is usually "janvier" */
}
--
2.13.5
Re: new module 'strlcpy'
Hi Paul, > Anyway, strlcpy is overkill here, as snprintf does the job just as well > here Not at all. snprintf is not at the right level of abstraction. There are three levels of abstractions to consider: (1) C code which composes a memory region by writing to bytes individually. (2) C code which works with strings. (3) C code which does memory allocations, formatted output etc. Here the task is to copy a string to a bounded memory area. The level (1) - which is embodied by the "strncpy and set NUL byte" approach - is too low-level, because - it is easy to make mistakes at this level (off-by-1), - it does not encourage the user to think about truncation and how to handle it. The level (3) - which is embodied by the "snprintf %s" approach = is too high-level, because - it is overkill to parse a format string when all you want to do is copy a single string, - snprintf can call malloc() and can fail with ENOMEM; these are undesired outcomes here. The level (2) is right for the task, but the standardized functions (memcpy, strcpy, strncpy) are not up to the job: - memcpy because it requires too much preparation code, and is still vulnerable to off-by-1 mistakes, - strcpy because it may produce buffer overruns, - strncpy because it may produce silent truncation. strlcpy with __warn_unused_result__ is the best solution for this task. > I'd really rather not promote the use of strlcpy in GNU code, as it is > contrary to GNU style. 1) I do not promote "strlcpy" with the flaw of ignoring the return value. I promote "strlcpy with __warn_unused_result__", which is an entirely different beast, because it will make the users aware of the return value and of the silent truncation issue. In some places the users will notice that strlcpy does not buy them much, compared to the "avoid arbitrary limits"[1] approach, and will switch over to what you call "GNU style". In other places, they will insert an abort() or assert() to handle the truncation case - which is *better* than the strncpy approach. 2) You need to distinguish application code and test code. The GNU standards [1] request to avoid arbitrary limits for programs. It is completely OK to assume that month names are less than 100 bytes long, *in unit tests*. If the same standards were set for test code than for application code, it would become even more tedious and boring to write unit tests than it already is. Probably I should configure the Coverity scan of Gnulib such that it puts the test code in a different area, so that we can apply different filters and prioritizations to the tests, and so that we won't be troubled by "sloppy" style in the tests. > Plus, I'm not a fan of __warn_unused_result__; in > my experience it's too often more trouble than it's worth We have a module 'ignore-value' that deals with it; so the trouble you are mentioning must be a trouble in the past, not in the future. Bruno [1] https://www.gnu.org/prep/standards/html_node/Semantics.html
Re: new module 'strlcpy'
I wrote: >In some places the users >will notice that strlcpy does not buy them much, compared to the >"avoid arbitrary limits"[1] approach, and will switch over to what >you call "GNU style". In other places, they will insert an abort() >or assert() to handle the truncation case - which is *better* than >the strncpy approach. For example, in gnulib's setlocale.c override. This file has fixed-size buffers and silent truncation - because it uses the "strncpy and set NUL byte" approach. As soon as someone (me) will use strlcpy with __warn_unused_result__ there, he will change the code to do - either dynamic allocation and no arbitrary limits, - or provide a good alternative to the silent truncation. Either result will be better than the current one. Bruno
Re: new module 'strlcpy'
On Wed, Sep 27, 2017 at 5:44 PM, Bruno Haible wrote: > I wrote: >>In some places the users >>will notice that strlcpy does not buy them much, compared to the >>"avoid arbitrary limits"[1] approach, and will switch over to what >>you call "GNU style". In other places, they will insert an abort() >>or assert() to handle the truncation case - which is *better* than >>the strncpy approach. > > For example, in gnulib's setlocale.c override. This file has fixed-size > buffers and silent truncation - because it uses the "strncpy and set NUL byte" > approach. As soon as someone (me) will use strlcpy with __warn_unused_result__ > there, he will change the code to do > - either dynamic allocation and no arbitrary limits, > - or provide a good alternative to the silent truncation. > Either result will be better than the current one. I have to agree that we need something, and it may even be spelled strlcpy. It seems reasonable to allow new uses of strlcpy, given a __warn_unused_result__ attribute -- that should prevent those new users from ignoring that return value. I developed a strong aversion to strncpy, and wrote this about it: https://meyering.net/crusade-to-eliminate-strncpy/
Re: new module 'strlcpy'
On 09/27/2017 05:35 PM, Bruno Haible wrote: strlcpy with __warn_unused_result__ is the best solution for this task. No it's not, because strlcpy always computes the string length of the source, and that is neither necessary nor desirable. Furthermore, in the bad style of programming that uses strlcpy, it's often acceptable to ignore the result since the programmer *wants* silent truncation. That is what strlcpy means in OpenBSD etc., and we shouldn't be trying to reuse their name for a function with different semantics. A better way to fix the test case is to remove its arbitrary limit on month name lengths. That way, it won't need snprintf or strlcpy or strncpy or anything like that. And it'll be following the GNU coding standards better. I can propose a patch along those lines, if you like. I do not want Gnulib to promote the use of strlcpy; that's a step in the wrong direction. If the same standards were set for test code than for application code, it would become even more tedious and boring to write unit tests than it already is. In that case, snprintf suffices: maybe snprintf is not good enough for production code, but it's plenty good for this test case. If you really want a function whose semantics are like strlcpy's but has __warn_unused_result__ semantics (and while we're at it, also does not count overlong sources, because that's silly), then I suppose we could invent one and use it for Gnulib. But we should not call it strlcpy, and we shouldn't use it in production code.
Re: new module 'strlcpy'
How about strscpy from the Linux kernel? https://www.kernel.org/doc/htmldocs/kernel-api/API-strscpy.html 28 сент. 2017 г. 4:23 ДП пользователь "Paul Eggert" написал: > On 09/27/2017 05:35 PM, Bruno Haible wrote: > >> strlcpy with __warn_unused_result__ is the best solution for this task. >> > > No it's not, because strlcpy always computes the string length of the > source, and that is neither necessary nor desirable. Furthermore, in the > bad style of programming that uses strlcpy, it's often acceptable to ignore > the result since the programmer *wants* silent truncation. That is what > strlcpy means in OpenBSD etc., and we shouldn't be trying to reuse their > name for a function with different semantics. > > A better way to fix the test case is to remove its arbitrary limit on > month name lengths. That way, it won't need snprintf or strlcpy or strncpy > or anything like that. And it'll be following the GNU coding standards > better. I can propose a patch along those lines, if you like. I do not want > Gnulib to promote the use of strlcpy; that's a step in the wrong direction. > > If the same standards were set for test code >> than for application code, it would become even more tedious and >> boring to write unit tests than it already is. >> > In that case, snprintf suffices: maybe snprintf is not good enough for > production code, but it's plenty good for this test case. > > If you really want a function whose semantics are like strlcpy's but has > __warn_unused_result__ semantics (and while we're at it, also does not > count overlong sources, because that's silly), then I suppose we could > invent one and use it for Gnulib. But we should not call it strlcpy, and we > shouldn't use it in production code. > > >
