Re: [libvirt] gnulib tests in libvirt broken by newer glibc 2.26

[Paul Eggert]

That patch essentially negates the point of the test, which is that 
getopt should be visible from unistd.h. I'd rather fix the problem than 
nuke the test.


Could you explain what the Gnulib problem is here? I can't really see it 
in your email. A self-contained example would help.


For what it's worth, I could not reproduce the problem on Fedora 26 by 
doing this in Gnulib (this tells 'configure' to use Gnulib-supplied 
getopt.h and getopt.c):


./gnulib-tool --create-testdir --dir foo getopt-posix
cd foo
./configure gl_cv_func_getopt_posix=no ac_cv_header_getopt_h=no
make
make check

Re: alternatives to 'strlcpy'

[Paul Eggert]

On 09/28/2017 12:36 PM, Dmitry Selyutin wrote:

ptrdiff_t is not a good choice here,  > because it represents not length, but 
difference between pointers
They're the same concept in C. In 7th Edition Unix strlen returned int, 
and this was better than having it return an unsigned type due to common 
problems with wraparound arithmetic and comparisons with unsigned types. 
In GNU Emacs source code, the general style has been to prefer signed 
integers for indexes and sizes and the like, and this has helped us gain 
reliability because we can catch integer overflows better by compiling 
with -fsanitize=undefined, bugs that we couldn't catch so easily if we 
used unsigned integers.


This is why I suggest ptrdiff_t for new low-level APIs. Although it 
limits object size to PTRDIFF_MAX, that limitation is present already 
because in practice behavior is undefined when you compute p1-p2 when 
((char *) p1 - (char *) p2) does not fit into ptrdiff_t, which means 
that objects larger than PTRDIFF_MAX are dangerous and should be avoided 
anyway.


ssize_t is a worse choice for portable software, as it is not in 
stddef.h and on a few old platforms ssize_t is narrower than size_t 
(POSIX allows this).


All this is moot of course if any new function returns bool or void, as 
I suspect it should.

Re: alternatives to 'strlcpy'

[Paul Eggert]

On 09/28/2017 11:39 AM, Bruno Haible wrote:

in BSD, it is common practice to try
a call with a fixed-size stack-allocated or statically allocated buffer,
and try dynamic memory only when this first attempt fails.


This doesn't match my experience with BSD application code, where the 
most common practice is to call strlcpy and ignore the return value. I 
looked at the source for OpenSSH 7.5 (the current version). Of its 56 
calls to strlcpy, only 15 check the return value, and none of these 
calls try dynamic allocation later (they all simply fail in the caller 
somehow). Of the 41 calls that ignore the return value, sometimes this 
is a bug and sometimes not, and often when it is not a bug plain strcpy 
would work as well. So the strlcpy API appears to be a poor fit for 
OpenSSH's needs.



/* Copies the string SRC, possibly truncated, into the bounded memory area
[DST,...,DST+DSTSIZE-1].
If the result fits, it returns 0.
If DSTSIZE is 0, it returns -1 and sets errno to EINVAL.
If DSTSIZE is nonzero and the result does not fit, it produces a NUL-
terminated truncated result, and returns -1 and sets errno to E2BIG.  */
extern int strgcpy (char *__dst, size_t __dstsize, const char *__src)
   __attribute__ ((__nonnull__ (1, 3)))
   __attribute__ ((__warn_unused_result__));


Some comments:

* I suggest a name that does not begin with "str", as that's reserved to 
the implementation.


* I too am bothered by this function setting errno (and using two 
different errno values to boot); it'd be simpler for it to just to 
return a value, or to abort if the destination is too small.


* If the intent is that this function be used for test-case code that 
aborts if the source is too long, then I suggest that the function 
simply return 'void' and abort if the source is too long, as that would 
be more convenient for the callers. If the intent is something else, I'd 
like to know what it is so that I can think about it more.

Re: alternatives to 'strlcpy'

[Dmitry Selyutin]

>> Though I would change its ssize_t 
>> to ptrdiff_t, so that it depends only on stddef.h.
>   - The mixed use of ssize_t vs. size_t.
What's wrong with ssize_t? As for me, ptrdiff_t is not a good choice here, 
because it represents not length, but difference between pointers. String 
length IS a difference between pointer in some way, but well, I think that's 
not the first allegory that comes to mind when one talks about strings. As for 
me, the only reason to avoid ssize_t is a dependency to stddef.h, but well, I 
think it is going to be used in most projects anyway, especially if these 
projects are somewhat complex. I thought size_t also comes from stddef, but I 
may be wrong. :-)

>   - The argument order.
That's also I think largely a historical issue. BTW, on x86_64 it makes some 
sense due to use of $rdi/$rsi. The only register that must be copied is $rcx. 
After then, one can implement memcpy/strcpy in just a few lines of assembly. 
Anyway, I agree that the argument order should better be changed, to avoid 
confusing strgcpy with its relatives (strncpy, strlcpy, strscpy).

>   If DSTSIZE is 0, it returns -1 and sets errno to EINVAL.
>   If DSTSIZE is nonzero and the result does not fit, it produces a NUL-
>   terminated truncated result, and returns -1 and sets errno to E2BIG.
I really dislike the idea of setting errno, since it involves thread-local 
storage. I know it's a "cold" path, but nevertheless, I really favor returning 
error codes if it is possible. And it is possible with ssize_t. Moreover, more 
modern POSIX functions do actually favor returning error code directly.


Anyway, I really think it is a good idea to provide yet another function, for 
me it seems the most logical choice; I cannot guess what "g" in strgcpy means 
though. Guarded? GNU? Generic? Godlike? :-)
The only possible reason to avoid doing such change is that some people may 
think that these Open Source guys went crazy and invented at least three 
functions to copy strings (four, including strscpy from the Linux kernel); 
moreover, since some people do not actually know the difference between strcpy 
and strcat, the situation is even worse. :-)

-- 
With best regards,
Dmitry Selyutin

signature.asc
Description: This is a digitally signed message part.

Re: alternatives to 'strlcpy'

[Bruno Haible]

Paul Eggert wrote:
> If you really want a function whose semantics are like strlcpy's but has 
> __warn_unused_result__ semantics ..., then I suppose we could 
> invent one and use it for Gnulib. But we should not call it strlcpy

Yes, I do want such a function for copying a string to a bounded memory area.

What you don't like about strlcpy:
  - The name.
  - The return value: why compute strlen(src) when the function does not
otherwise need it?

The reason for this design is that in BSD, it is common practice to try
a call with a fixed-size stack-allocated or statically allocated buffer,
and try dynamic memory only when this first attempt fails. The return value
is thus useful for the second step. For system calls this is a reasonable
design, but not here: the caller can call strlen(src) by himself. It is
strange design to do the first part of the second step in the function
which is supposed to do the first step. My take on this is that such
an algorithm should be abstracted in a facility of its own, like
 or .

Other things I don't like about strlcpy:
  - It's hard to remember whether the return value should be tested
as >= dstsize, > dstsize, <= dstsize, < dstsize. As Jim writes [1]
"Some of us forget to read documentation. Or read it, and then forget
 details."
  - The argument order: while snprintf, strftime, strfmon, and others
receive the destination buffer size immediately following the
destination buffer address, strlcpy wants it as third argument.
Which is against the principle "keep together what belongs together".

Dmitry suggested to look at strscpy [2]. About this one I don't like
  - The return value convention: It's suitable for in-kernel APIs only.
  - The mixed use of ssize_t vs. size_t.
  - The argument order.

Here's my take:

/* Copies the string SRC, possibly truncated, into the bounded memory area
   [DST,...,DST+DSTSIZE-1].
   If the result fits, it returns 0.
   If DSTSIZE is 0, it returns -1 and sets errno to EINVAL.
   If DSTSIZE is nonzero and the result does not fit, it produces a NUL-
   terminated truncated result, and returns -1 and sets errno to E2BIG.  */
extern int strgcpy (char *__dst, size_t __dstsize, const char *__src)
  __attribute__ ((__nonnull__ (1, 3)))
  __attribute__ ((__warn_unused_result__));

Bruno

[1] https://meyering.net/crusade-to-eliminate-strncpy/
[2] https://www.kernel.org/doc/htmldocs/kernel-api/API-strscpy.html

Re: new module 'strlcpy'

[Paul Eggert]

On 09/28/2017 10:54 AM, Bruno Haible wrote:

Here's doc I propose to add to the gnulib documentation (and similar one
to wcscpy and wcsncpy):


Thanks, that looks good. I too share an aversion to strncpy, and wish 
that I had not stirred up this hornet's nest by using it in a moment of 
weakness.

Re: new module 'strlcpy'

[Paul Eggert]

On 09/27/2017 11:29 PM, Dmitry Selyutin wrote:

How about strscpy from the Linux kernel?


Yes, that's a better API than strlcpy. Though I would change its ssize_t 
to ptrdiff_t, so that it depends only on stddef.h. Plus, I would define 
its behavior even if the buffers overlap - that's safer, and the 
function is for when safety is more important than performance or 
correctness.

Re: new module 'strlcpy'

[Bruno Haible]

Hi Jim,

> I developed a strong aversion to strncpy, and wrote this about it:
> https://meyering.net/crusade-to-eliminate-strncpy/

Thanks for your voice and past effort here.

Here's doc I propose to add to the gnulib documentation (and similar one
to wcscpy and wcsncpy):

diff --git a/doc/posix-functions/strcpy.texi b/doc/posix-functions/strcpy.texi
index 3289362..89c6cd3 100644
--- a/doc/posix-functions/strcpy.texi
+++ b/doc/posix-functions/strcpy.texi
@@ -17,3 +17,6 @@ OS X 10.8.
 Portability problems not fixed by Gnulib:
 @itemize
 @end itemize
+
+Note: @code{strcpy (dst, src)} is only safe to use when you can guarantee that
+there are at least @code{strlen (src) + 1} bytes allocated at @code{dst}.
diff --git a/doc/posix-functions/strncpy.texi b/doc/posix-functions/strncpy.texi
index 3cc6b45..087acaf 100644
--- a/doc/posix-functions/strncpy.texi
+++ b/doc/posix-functions/strncpy.texi
@@ -17,3 +17,12 @@ OS X 10.8.
 Portability problems not fixed by Gnulib:
 @itemize
 @end itemize
+
+Note: This function was designed for the use-case of filling a fixed-size
+record with a string, before writing it to a file.  This function is
+@strong{not} appropriate for copying a string into a bounded memory area,
+because you have no guarantee that the result will be NUL-terminated.
+Even if you add the NUL byte at the end yourself, this function is
+inefficient (as it spends time clearing unused memory) and will allow
+silent truncation occur, which is not a good behavior for GNU programs.
+For more details, see @see{https://meyering.net/crusade-to-eliminate-strncpy/}.

Re: stdint.h: #if with no expression

[Bruno Haible]

Tim Rühsen wrote:
> The gettext (0.19.8.1-4) package contains pretty old m4 files.
> 
> With 'autoreconf -fi' all that old stuff is copied into m4/ though there
> are newer versions from gnulib in gl/m4 and lib/gl/m4/.
> 
> Removing m4/ and later on using autoreconf without -fi does the job
> (resp. doesn't copy unwanted stuff into m4/).

And what if you keep using autoreconf, but set the environment variable
AUTOPOINT to ':', to prevent autoreconf from invoking autopoint? (And
rely on the gnulib module 'gettext-h' instead.)

Or if you stop using autoreconf entirely, and instead set up your
invocations of 'aclocal', 'autoconf', 'autoheader', 'automake' (with
appropriate options) by yourself?

Bruno

Re: stdint.h: #if with no expression

[Tim Rühsen]

The gettext (0.19.8.1-4) package contains pretty old m4 files.

With 'autoreconf -fi' all that old stuff is copied into m4/ though there
are newer versions from gnulib in gl/m4 and lib/gl/m4/.

Removing m4/ and later on using autoreconf without -fi does the job
(resp. doesn't copy unwanted stuff into m4/).


With Best Regards, Tim



On 09/26/2017 05:01 PM, Tim Rühsen wrote:
> On 09/26/2017 04:49 PM, Bruno Haible wrote:
>> Hi,
>>
>> Tim Rühsen wrote:
>>> With the latest gnulib I experience the mentioned error.
>>>
>>> Command sequence (configure.ac, Makefile.am, ... exists):
>>>
>>> ../gnulib/gnulib-tool --import --local-dir=gl/override --lib=libgnu
>>> --source-base=gl --m4-base=gl/m4 --doc-base=doc --tests-base=gltests
>>> --aux-dir=build-aux --with-tests --avoid=fcntl-h-tests
>>> --avoid=stdlib-tests --avoid=string-tests --avoid=sys_stat-tests
>>> --avoid=time-tests --avoid=unistd-tests --avoid=update-copyright-tests
>>> --avoid=wchar-tests --no-conditional-dependencies --libtool
>>> --macro-prefix=gl --no-vc-files autobuild csharpcomp-script
>>> csharpexec-script error fdl-1.3 gendocs getline getopt-gnu gnupload
>>> maintainer-makefile manywarnings pmccabe2html progname update-copyright
>>> useless-if-before-free valgrind-tests vc-list-files version-etc warnings
>>>
>>> autoreconf -fi
>>>
>>> ./configure
>>
>> One possible cause of the problem: You did not do "make distclean" before
>> "./configure".
> 
> The directory was fresh and clean. Just Makefile.am, configure.ac and
> zero-byte files for NEWS, AUTHORS, ...
> 
>> The other possible cause: You have an old wint_t.m4 hanging around somewhere,
>> like the reporter in [1].
> 
> After gnulib-tool / autoreconf -fi:
> $ ls -la `find . -name wint_t.m4`
> 
> -rw-r--r-- 1 oms users 2339 Sep 26 15:36 ./gl/m4/wint_t.m4
> 
> 
> -rw-r--r-- 1 oms users 1053 Sep 26 15:13 ./m4/wint_t.m4
> 
> 
> So these should be the same, I guess:
> $ head -2 `find . -name wint_t.m4`
> ==> ./m4/wint_t.m4 <==
> # wint_t.m4 serial 5 (gettext-0.18.2)
> dnl Copyright (C) 2003, 2007-2014 Free Software Foundation, Inc.
> 
> ==> ./gl/m4/wint_t.m4 <==
> # wint_t.m4 serial 7
> dnl Copyright (C) 2003, 2007-2017 Free Software Foundation, Inc.
> 
> $ ls -la /usr/share/aclocal/wint_t.m4 /usr/share/gnulib/m4/wint_t.m4
> -rw-r--r-- 1 root root 1053 22-08-17 03:20:24 /usr/share/aclocal/wint_t.m4
> -rw-r--r-- 1 root root 1053 03-02-14 01:48:46 /usr/share/gnulib/m4/wint_t.m4
> 
> The first is from Debian package 'gettext', the second from 'gnulib':
> ii  gettext 0.19.8.1-4 amd64
>  GNU Internationalization utilities
> ii  gnulib  20140202+stable-2  all
>  GNU Portability Library
> 
> OK, wow... quite old stuff here :-(
> I'll remove that and try again tomorrow (no time today) and report back.
> 
> Thanks for your help !
> 
> With Best Regards, Tim
> 



signature.asc
Description: OpenPGP digital signature

Re: [libvirt] gnulib tests in libvirt broken by newer glibc 2.26

[Christian Ehrhardt]

On Thu, Sep 28, 2017 at 2:05 PM, Christian Ehrhardt <
christian.ehrha...@canonical.com> wrote:

>
>
> On Thu, Sep 28, 2017 at 12:25 AM, Eric Blake  wrote:
>
>> [adding gnulib]
>>
>
> [...]

>
>>
> then libvirt needs to pick up the
>> updated gnulib.
>
>
> I copied in current gnulib from git and it didn't resolve yet. But maybe
> it is just something that still is work in progress.
>

Until there is a final and better solution is available the following patch
avoids the issue without fully skipping the test for now.
Since the issue seems only to apply to getopt, but not getopt_long and the
code doesn't use that in anything except examples it might be a valid
interim helper until glibc/gnulib sorted out how that is handled more
correctly.


-- 
Christian Ehrhardt
Software Engineer, Ubuntu Server
Canonical Ltd
Description: fix gnulib unittests with glibc 2.26

Due to glibc and gnulib exchanging and syncing getopt implementations
it now uses the implementation form glibc but expects the former gnulib
behavior.

Until fully resolved override the test include to use gnulib function as it
used to be before.

It is somewhat ok to do so as the only real user of getopt so far is an example
at examples/admin/logging.c. All others use getopt_long which does not have
this function name collision between unistd.h and getopt.h.

Note: to be replaced by the final fix as soon as that is available.

Forwarded: yes https://www.redhat.com/archives/libvir-list/2017-September/msg01039.html
Author: Christian Ehrhardt 
Bug-Ubuntu: https://bugs.launchpad.net/bugs/1718668
Last-Update: 2017-09-28

 test-getopt-posix.c |7 +++
  1 file changed, 7 insertions(+)

--- a/gnulib/tests/test-getopt-posix.c
+++ b/gnulib/tests/test-getopt-posix.c
@@ -22,6 +22,13 @@
ftell link warning if we are not using the gnulib ftell module.  */
 #define _GL_NO_LARGE_FILES
 
+/*
+ * Glibc 2.26 does hard include bits/getopt_posix.h which causes the system
+ * to use glibc's getopt but the tests expect gnulib behavior. Until a better
+ * fix is available this avoids that mis-resolution.
+ */
+#include 
+
 /* POSIX and glibc provide the getopt() function in , see
http://pubs.opengroup.org/onlinepubs/9699919799/functions/getopt.html
https://www.gnu.org/software/libc/manual/html_node/Using-Getopt.html

Re: [libvirt] gnulib tests in libvirt broken by newer glibc 2.26

[Christian Ehrhardt]

On Thu, Sep 28, 2017 at 12:25 AM, Eric Blake  wrote:

> [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),


Yeah that seems to be part of the effort I linked.


> then libvirt needs to pick up the
> updated gnulib.


I copied in current gnulib from git and it didn't resolve yet. But maybe it
is just something that still is work in progress.


> 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?


It was Ubuntu 17.10 (Artful) which is on 2.26

>
> > 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
>
>


-- 
Christian Ehrhardt
Software Engineer, Ubuntu Server
Canonical Ltd

Re: new module 'strlcpy'

[Tim Rühsen]

On 09/28/2017 08:29 AM, Dmitry Selyutin wrote:
> How about strscpy from the Linux kernel?
> 
> https://www.kernel.org/doc/htmldocs/kernel-api/API-strscpy.html

As an application library & programmer, I need this (thanks, Dmitry)
*and* strlcpy. A gnulib module would reduce configure.ac code and
several checks - and thus would be very welcome (thanks, Bruno).

In production code strlcpy replaces snprintf with format string "%s" -
the return value (length of source string) often comes in handy for
malloc fallbacks.

But I already stumbled into the side-effects of strlcpy in the past.
Determining the length of the source is sometimes dangerous and may lead
to unexpected CPU cycle waste. So this strscpy function seems to be a
good choice if you are not interested in the source length.

With Best Regards, Tim



signature.asc
Description: OpenPGP digital signature

Re: new module 'strlcpy'

[Dmitry Selyutin]

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.
>
>
>