accept4 and SOCK_NONBLOCK

accept4 and SOCK_NONBLOCK

[Richard W.M. Jones]

First of all I'm using Linux 5.0.17 & glibc-2.29-15 so as far as I'm
aware accept4 exists and fully works and gnulib shouldn't be replacing
it at all.  I don't understand why it's being replaced.

But given that, in libguestfs we call:

  r = accept4 (console_sock, NULL, NULL, SOCK_NONBLOCK|SOCK_CLOEXEC);

  https://github.com/libguestfs/libguestfs/blob/master/lib/launch-libvirt.c#L639

This is a valid set of flags according to the Linux man page for
accept4(2).  But it fails with EINVAL because of the following test in
gnulib:

  if ((flags & ~(SOCK_CLOEXEC | O_TEXT | O_BINARY)) != 0)
    {
      errno = EINVAL;
      return -1;
    }

  https://git.savannah.gnu.org/cgit/gnulib.git/tree/lib/accept4.c#n61

So I think the set of flags should be broadened to include
SOCK_NONBLOCK + add a call to set_nonblocking_flag.

As for why accept4 is being replaced at all, the only reference to it
in config.log is:

  configure:25630: checking whether accept4 is declared
  configure:25630: gcc -c -O2 -g -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -Wp,-D_GLIBCXX_ASSERTIONS -fexceptions -fstack-protector-strong -grecord-gcc-switches -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -m64 -mtune=generic -fasynchronous-unwind-tables -fstack-clash-protection -fcf-protection  conftest.c >&5
  configure:25630: $? = 0
  configure:25630: result: yes

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html

Re: accept4 and SOCK_NONBLOCK

[Florian Weimer]

* Richard W. M. Jones:

> As for why accept4 is being replaced at all, the only reference to it
> in config.log is:
>
>   configure:25630: checking whether accept4 is declared
>   configure:25630: gcc -c -O2 -g -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -Wp,-D_GLIBCXX_ASSERTIONS -fexceptions -fstack-protector-strong -grecord-gcc-switches -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -m64 -mtune=generic -fasynchronous-unwind-tables -fstack-clash-protection -fcf-protection  conftest.c >&5
>   configure:25630: $? = 0
>   configure:25630: result: yes

What does the test program look like?  Does it include <config.h>?
accept4 requires _GNU_SOURCE.

Thanks,
Florian

Re: accept4 and SOCK_NONBLOCK

[Richard W.M. Jones]

On Tue, Aug 20, 2019 at 05:37:58PM +0200, Florian Weimer wrote:
> * Richard W. M. Jones:
> 
> > As for why accept4 is being replaced at all, the only reference to it
> > in config.log is:
> >
> >   configure:25630: checking whether accept4 is declared
> >   configure:25630: gcc -c -O2 -g -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -Wp,-D_GLIBCXX_ASSERTIONS -fexceptions -fstack-protector-strong -grecord-gcc-switches -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -m64 -mtune=generic -fasynchronous-unwind-tables -fstack-clash-protection -fcf-protection  conftest.c >&5
> >   configure:25630: $? = 0
> >   configure:25630: result: yes
> 
> What does the test program look like?  Does it include <config.h>?
> accept4 requires _GNU_SOURCE.

I'm not sure how to find out.  However m4/accept.m4 (supplied
by gnulib itself) does contain:

  dnl Persuade glibc <sys/socket.h> to declare accept4().
  AC_REQUIRE([AC_USE_SYSTEM_EXTENSIONS])

so I guess the test *ought* to be using _GNU_SOURCE.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

Re: accept4 and SOCK_NONBLOCK

[Florian Weimer]

* Richard W. M. Jones:

> On Tue, Aug 20, 2019 at 05:37:58PM +0200, Florian Weimer wrote:
>> * Richard W. M. Jones:
>> 
>> > As for why accept4 is being replaced at all, the only reference to it
>> > in config.log is:
>> >
>> >   configure:25630: checking whether accept4 is declared
>> >   configure:25630: gcc -c -O2 -g -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -Wp,-D_GLIBCXX_ASSERTIONS -fexceptions -fstack-protector-strong -grecord-gcc-switches -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -m64 -mtune=generic -fasynchronous-unwind-tables -fstack-clash-protection -fcf-protection  conftest.c >&5
>> >   configure:25630: $? = 0
>> >   configure:25630: result: yes
>> 
>> What does the test program look like?  Does it include <config.h>?
>> accept4 requires _GNU_SOURCE.
>
> I'm not sure how to find out.  However m4/accept.m4 (supplied
> by gnulib itself) does contain:
>
>   dnl Persuade glibc <sys/socket.h> to declare accept4().
>   AC_REQUIRE([AC_USE_SYSTEM_EXTENSIONS])
>
> so I guess the test *ought* to be using _GNU_SOURCE.

And the configure test acctually succeeded, which I missed.  So it has
to be something else.  You'll have to investigate the generated files to
see what's going wrong there.

Thanks,
Florian

Re: accept4 and SOCK_NONBLOCK

[Eric Blake]

[-- Attachment #1.1: Type: text/plain, Size: 2840 bytes --]

On 8/20/19 10:27 AM, Richard W.M. Jones wrote:
> First of all I'm using Linux 5.0.17 & glibc-2.29-15 so as far as I'm
> aware accept4 exists and fully works and gnulib shouldn't be replacing
> it at all.  I don't understand why it's being replaced.
> 

I think I'm reproducing your setup (Fedora 30), and running:

./gnulib-tool --test accept4

shows:
...
/usr/bin/mkdir -p sys
rm -f sys/socket.h-t sys/socket.h && \
{ echo '/* DO NOT EDIT! GENERATED AUTOMATICALLY! */'; \
  sed -e 's|@''GUARD_PREFIX''@|GL|g' \
...
      -e 's/@''GNULIB_ACCEPT4''@/1/g' \
...
      -e 's|@''HAVE_ACCEPT4''@|1|g' \
...
make[4]: Entering directory '/home/eblake/gnulib/testdir935/build/gllib'
gcc -DHAVE_CONFIG_H -I. -I../../gllib -I..  -DGNULIB_STRICT_CHECKING=1
-g -O2 -MT accept4.o -MD -MP -MF .deps/accept4.Tpo -c -o accept4.o
../../gllib/accept4.c

so it is indeed locating the system accept4(), but also compiling the
gnulib file.  In turn, the gnulib file starts out:

#if HAVE_ACCEPT4

Aha - there's the bug.

Commit e597d4b8 changed from AC_CHECK_FUNCS_ONCE (defines HAVE_ACCEPT4)
to AC_CHECK_DECLS (defines HAVE_DECL_ACCEPT4), but failed to update the
.c file.  I'll push the obvious fix.



> But given that, in libguestfs we call:
> 
>   r = accept4 (console_sock, NULL, NULL, SOCK_NONBLOCK|SOCK_CLOEXEC);
> 
>   https://github.com/libguestfs/libguestfs/blob/master/lib/launch-libvirt.c#L639
> 
> This is a valid set of flags according to the Linux man page for
> accept4(2).  But it fails with EINVAL because of the following test in
> gnulib:
> 
>   if ((flags & ~(SOCK_CLOEXEC | O_TEXT | O_BINARY)) != 0)
>     {
>       errno = EINVAL;
>       return -1;
>     }
> 
>   https://git.savannah.gnu.org/cgit/gnulib.git/tree/lib/accept4.c#n61

Yes, that code is unnecessarily strict; gnulib should be taught to
handle SOCK_NONBLOCK, but doing it everywhere correctly is a bigger
task, as it also affects the socket() function.

> 
> So I think the set of flags should be broadened to include
> SOCK_NONBLOCK + add a call to set_nonblocking_flag.
> 
> As for why accept4 is being replaced at all, the only reference to it
> in config.log is:
> 
>   configure:25630: checking whether accept4 is declared
>   configure:25630: gcc -c -O2 -g -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -Wp,-D_GLIBCXX_ASSERTIONS -fexceptions -fstack-protector-strong -grecord-gcc-switches -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -m64 -mtune=generic -fasynchronous-unwind-tables -fstack-clash-protection -fcf-protection  conftest.c >&5
>   configure:25630: $? = 0
>   configure:25630: result: yes

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: accept4 and SOCK_NONBLOCK

[Bruno Haible]

Eric Blake wrote:
> #if HAVE_ACCEPT4
> 
> Aha - there's the bug.
> 
> Commit e597d4b8 changed from AC_CHECK_FUNCS_ONCE (defines HAVE_ACCEPT4)
> to AC_CHECK_DECLS (defines HAVE_DECL_ACCEPT4), but failed to update the
> .c file.  I'll push the obvious fix.

Indeed. My mistake, sorry.

Bruno