nss_wrapper support for musl-libc

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

nss_wrapper support for musl-libc

Dennis Schridde
Hello!

I made a few changes to nss_wrapper to get it to run on musl-libc 1.1 (Alpine
Linux 3.5). Please find the patch attached.

One test failed, without me being able to fix it: gethostent returns NULL on
musl-libc, but I don't know what might cause this or whether the test is
actually valid for musl-libc. If you could provide me with further information
on the purpose and expected behaviour of this function (the information on the
net was rather scarce) or why it might return NULL in this case, I would be
very glad.

If you want to test this, I suggest using my Dockerfile [1] as a basis. By
also installing "cmocka" and changing the build instructions at the bottom
appropriately, you can run the testsuite.

I would be glad if you could integrate this into your sources for the next
release. If you have concerns about my changes, related to coding style or
anything else, please do not hesitate to explain to me how I should improve
this code to allow inclusion into your sources.

Best regards,
Dennis Schridde

[1]: https://github.com/urzds/dynamicuser-docker/blob/master/Dockerfile
--
Heidelberg University Computing Centre
Service division: Future IT - Research & Education

Tel. +49 6221 54-4519, Fax +49 6221 54-5581
[hidden email]

http://www.urz.uni-heidelberg.de/

Ruprecht-Karls-Universität Heidelberg
Universitätsrechenzentrum
Im Neuenheimer Feld 293, 69120 Heidelberg, Germany

0001-Add-compatibility-for-musl-libc-1.1.patch (12K) Download Attachment
signature.asc (705 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: nss_wrapper support for musl-libc

Andreas Schneider-15
On Wednesday, 25 January 2017 23:47:23 CET Dennis Schridde wrote:
> Hello!

Hi Dennis,


thank you very much for your contribution!
 
> I made a few changes to nss_wrapper to get it to run on musl-libc 1.1
> (Alpine Linux 3.5). Please find the patch attached.
>
> One test failed, without me being able to fix it: gethostent returns NULL on
> musl-libc, but I don't know what might cause this or whether the test is
> actually valid for musl-libc. If you could provide me with further
> information on the purpose and expected behaviour of this function (the
> information on the net was rather scarce) or why it might return NULL in
> this case, I would be very glad.

The only documentation is the source. You need to dig there to see what is
going on.

>
> If you want to test this, I suggest using my Dockerfile [1] as a basis. By
> also installing "cmocka" and changing the build instructions at the bottom
> appropriately, you can run the testsuite.
>
> I would be glad if you could integrate this into your sources for the next
> release. If you have concerns about my changes, related to coding style or
> anything else, please do not hesitate to explain to me how I should improve
> this code to allow inclusion into your sources.

The changes to src/nss_wrapper.c are fine, if you comment #else and #endif,
like:

#ifdef HAVE_GETPWENT_R
...
#else /* HAVE_GETPWENT_R */
...
#endif /* HAVE_GETPWENT_R */

This makes it easier to understand which #ifdef it is is.


About tests/test_getaddrinfo.c:

I think you should report those to the developers of musl-libc. These tests
pass correctly on Linux, Solaris and BSD. All have different libc
implementations.

  rc = getaddrinfo(NULL, "echo", &hints, &res);
- assert_int_equal(rc, EAI_NONAME);
+ if (rc != EAI_SERVICE) { // musl-libc returns EAI_SERVICE
+ assert_int_equal(rc, EAI_NONAME);
+ }

I would argue that EAI_NONAME is the correct error code here as it is NULL.
EAI_SERICE is obviously the wrong return code.


The changes to tests/testsuite.c are fine if comments are added. Please make
seperate commits for each file.


Thanks!


        Andreas



--
Andreas Schneider                   GPG-ID: CC014E3D
Samba Team                             [hidden email]
www.samba.org

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: nss_wrapper support for musl-libc

Dennis Schridde
Hi Andreas!

On Thursday, 26 January 2017 10:19:26 CET Andreas Schneider wrote:

> On Wednesday, 25 January 2017 23:47:23 CET Dennis Schridde wrote:
> > I made a few changes to nss_wrapper to get it to run on musl-libc 1.1
> > (Alpine Linux 3.5). Please find the patch attached.
> >
> > One test failed, without me being able to fix it: gethostent returns NULL
> > on musl-libc, but I don't know what might cause this or whether the test
> > is actually valid for musl-libc. If you could provide me with further
> > information on the purpose and expected behaviour of this function (the
> > information on the net was rather scarce) or why it might return NULL in
> > this case, I would be very glad.
>
> The only documentation is the source. You need to dig there to see what is
> going on.
It turns out that reason is simple: The function is a stub.

https://git.musl-libc.org/cgit/musl/tree/src/network/ent.c

Thus I checked in CMakeLists.txt whether gethostent() generally returns non-
NULL pointers. Based on that I define HAVE_NONNULL_GETHOSTENT and use the
result in tests/test_nwrap_disabled.c

> > If you want to test this, I suggest using my Dockerfile [1] as a basis. By
> > also installing "cmocka" and changing the build instructions at the bottom
> > appropriately, you can run the testsuite.
> >
> > I would be glad if you could integrate this into your sources for the next
> > release. If you have concerns about my changes, related to coding style or
> > anything else, please do not hesitate to explain to me how I should
> > improve
> > this code to allow inclusion into your sources.
>
> The changes to src/nss_wrapper.c are fine, if you comment #else and #endif,
> like:
>
> #ifdef HAVE_GETPWENT_R
> ...
> #else /* HAVE_GETPWENT_R */
> ...
> #endif /* HAVE_GETPWENT_R */
>
> This makes it easier to understand which #ifdef it is is.
I fixed that in attached patches.

The checks for HAVE_SOLARIS_GETGRENT_R were not consistent and used the
inversion operator for the #else part. I fixed that, too, to conform to the
style guidelines you mentioned above.

> About tests/test_getaddrinfo.c:
>
> I think you should report those to the developers of musl-libc. These tests
> pass correctly on Linux, Solaris and BSD. All have different libc
> implementations.
>
>   rc = getaddrinfo(NULL, "echo", &hints, &res);
> - assert_int_equal(rc, EAI_NONAME);
> + if (rc != EAI_SERVICE) { // musl-libc returns EAI_SERVICE
> + assert_int_equal(rc, EAI_NONAME);
> + }
>
> I would argue that EAI_NONAME is the correct error code here as it is NULL.
> EAI_SERICE is obviously the wrong return code.
Is EAI_NONAME correct, because "echo" is specified in /etc/services as 7/tcp
or 7/udp? I.e. because a remote "node" address is necessary to communicate via
TCP or UDP and getaddrinfo is supposed to deduct that from /etc/services?

I fixed that in attached patches. The tests will fail, until upstream resolves
the issue. I will report it to them, as soon as I have a better understanding
of how the function is supposed to behave.

> The changes to tests/testsuite.c are fine if comments are added. Please make
> seperate commits for each file.

Please find the patch series, one patch for each file, attached.

--Dennis
--
Universität Heidelberg, Universitätsrechenzentrum (URZ)
Servicebereich Future IT - Research & Education (FIRE)

Tel. +49 6221 54-4519, Fax +49 6221 54-5581
[hidden email]

http://www.urz.uni-heidelberg.de/

Ruprecht-Karls-Universität Heidelberg
Universitätsrechenzentrum
Im Neuenheimer Feld 293, 69120 Heidelberg

0001-Add-compatibility-for-musl-libc-1.1.patch (6K) Download Attachment
0002-Add-compatibility-for-musl-libc-1.1.patch (3K) Download Attachment
0003-Add-compatibility-for-musl-libc-1.1.patch (2K) Download Attachment
0004-Add-compatibility-for-musl-libc-1.1.patch (1K) Download Attachment
0005-Add-compatibility-for-musl-libc-1.1.patch (1K) Download Attachment
signature.asc (705 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: nss_wrapper support for musl-libc

Dennis Schridde
Hi Andreas!

On Montag, 20. Februar 2017 17:08:33 CET Dennis Schridde wrote:

> > About tests/test_getaddrinfo.c:
> >
> > I think you should report those to the developers of musl-libc. These
> > tests
> > pass correctly on Linux, Solaris and BSD. All have different libc
> > implementations.
> >
> >   rc = getaddrinfo(NULL, "echo", &hints, &res);
> >
> > - assert_int_equal(rc, EAI_NONAME);
> > + if (rc != EAI_SERVICE) { // musl-libc returns EAI_SERVICE
> > + assert_int_equal(rc, EAI_NONAME);
> > + }
> >
> > I would argue that EAI_NONAME is the correct error code here as it is
> > NULL.
> > EAI_SERICE is obviously the wrong return code.
>
> Is EAI_NONAME correct, because "echo" is specified in /etc/services as 7/tcp
> or 7/udp? I.e. because a remote "node" address is necessary to communicate
> via TCP or UDP and getaddrinfo is supposed to deduct that from
> /etc/services?
From reading freeaddrinfo(3p) (redirect from getaddrinfo(3p)) it is not clear
to me what the expected behaviour is. It says about these two errors:

[EAI_NONAME]
     The name does not resolve for the supplied parameters.

     Neither nodename nor servname were supplied. At least one of these shall
be supplied.

[EAI_SERVICE]
     The service passed was not recognized for the specified socket type.

It seems that EAI_NONAME is about resolving the nodename only, while
EAI_SERVICE indicates whether the provided servname and hints represent a sane
combination.

The test specifies no nodename, only a servname. Thus musl's behaviour of
indicating that "the service passed was not recognized for the specified
socket type" seems to be spot on. Going by the specs, it seems that either GNU
libc's behaviour is wrong or the specification is too lax in this regard.

Another question would be what we are actually testing here: Conformance of
libc to POSIX, or whether nss_wrapper works as intended?

> > The changes to tests/testsuite.c are fine if comments are added. Please
> > make seperate commits for each file.
>
> Please find the patch series, one patch for each file, attached.

I also want to mention that I'm using these patches to build a container for
Alpine Linux:
* https://github.com/urzds/dynamicuser-docker/tree/alpine
* https://quay.io/repository/urzds/dynamicuser

I'm looking forward to your second review of these patches! I hope they are in
good shape to be accepted for integration.

--Dennis

signature.asc (696 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: nss_wrapper support for musl-libc

Andreas Schneider-15
On Tuesday, 28 February 2017 12:20:00 CET Dennis Schridde wrote:

> Hi Andreas!
>
> On Montag, 20. Februar 2017 17:08:33 CET Dennis Schridde wrote:
> > > About tests/test_getaddrinfo.c:
> > >
> > > I think you should report those to the developers of musl-libc. These
> > > tests
> > > pass correctly on Linux, Solaris and BSD. All have different libc
> > > implementations.
> > >
> > >   rc = getaddrinfo(NULL, "echo", &hints, &res);
> > >
> > > - assert_int_equal(rc, EAI_NONAME);
> > > + if (rc != EAI_SERVICE) { // musl-libc returns EAI_SERVICE
> > > + assert_int_equal(rc, EAI_NONAME);
> > > + }
> > >
> > > I would argue that EAI_NONAME is the correct error code here as it is
> > > NULL.
> > > EAI_SERICE is obviously the wrong return code.
> >
> > Is EAI_NONAME correct, because "echo" is specified in /etc/services as
> > 7/tcp or 7/udp? I.e. because a remote "node" address is necessary to
> > communicate via TCP or UDP and getaddrinfo is supposed to deduct that
> > from
> > /etc/services?
>
> From reading freeaddrinfo(3p) (redirect from getaddrinfo(3p)) it is not
> clear to me what the expected behaviour is. It says about these two errors:
>
> [EAI_NONAME]
>      The name does not resolve for the supplied parameters.
>
>      Neither nodename nor servname were supplied. At least one of these
> shall be supplied.
>
> [EAI_SERVICE]
>      The service passed was not recognized for the specified socket type.
>
> It seems that EAI_NONAME is about resolving the nodename only, while
> EAI_SERVICE indicates whether the provided servname and hints represent a
> sane combination.
>
> The test specifies no nodename, only a servname. Thus musl's behaviour of
> indicating that "the service passed was not recognized for the specified
> socket type" seems to be spot on. Going by the specs, it seems that either
> GNU libc's behaviour is wrong or the specification is too lax in this
> regard.
>
> Another question would be what we are actually testing here: Conformance of
> libc to POSIX, or whether nss_wrapper works as intended?

Normally I try to mimic the behaviour of glibc. Then I test on other platforms
BSD, OpenSolaris if they differ. They often did the same as glibc.

I'm currently checking why BSD and Solaris nightly build are not working.


        Andreas

--
Andreas Schneider                   GPG-ID: CC014E3D
Samba Team                             [hidden email]
www.samba.org

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: nss_wrapper support for musl-libc

Samba - samba-technical mailing list
Hi Andreas!

On Dienstag, 28. Februar 2017 15:22:14 CET Andreas Schneider wrote:

> On Tuesday, 28 February 2017 12:20:00 CET Dennis Schridde wrote:
> > It seems that EAI_NONAME is about resolving the nodename only, while
> > EAI_SERVICE indicates whether the provided servname and hints represent a
> > sane combination.
> >
> > The test specifies no nodename, only a servname. Thus musl's behaviour of
> > indicating that "the service passed was not recognized for the specified
> > socket type" seems to be spot on. Going by the specs, it seems that either
> > GNU libc's behaviour is wrong or the specification is too lax in this
> > regard.
> >
> > Another question would be what we are actually testing here: Conformance
> > of
> > libc to POSIX, or whether nss_wrapper works as intended?
>
> Normally I try to mimic the behaviour of glibc. Then I test on other
> platforms BSD, OpenSolaris if they differ. They often did the same as
> glibc.
I checked again with POSIX.1-2008 [1]. It has the exact same wording as the
man-pages I quoted.

So how do we proceed from here? Shall we try to change musl's behaviour,
because it does not conform to GNU libc? Is conformance to any valid
interpretation of POSIX.1-2008 acceptable? Or is just my understanding of the
specification of getaddrinfo and its errors wrong?

> I'm currently checking why BSD and Solaris nightly build are not working.

Were you able to figure out the build problems?

Best regards,
Dennis

[1]: http://pubs.opengroup.org/onlinepubs/9699919799/

signature.asc (696 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: nss_wrapper support for musl-libc

Samba - samba-technical mailing list
Hi Andreas!

On Mittwoch, 22. März 2017 10:00:31 CEST Dennis Schridde via samba-technical
wrote:

> On Dienstag, 28. Februar 2017 15:22:14 CET Andreas Schneider wrote:
> > On Tuesday, 28 February 2017 12:20:00 CET Dennis Schridde wrote:
> > > It seems that EAI_NONAME is about resolving the nodename only, while
> > > EAI_SERVICE indicates whether the provided servname and hints represent
> > > a
> > > sane combination.
> > >
> > > The test specifies no nodename, only a servname. Thus musl's behaviour
> > > of
> > > indicating that "the service passed was not recognized for the specified
> > > socket type" seems to be spot on. Going by the specs, it seems that
> > > either
> > > GNU libc's behaviour is wrong or the specification is too lax in this
> > > regard.
> > >
> > > Another question would be what we are actually testing here: Conformance
> > > of
> > > libc to POSIX, or whether nss_wrapper works as intended?
> >
> > Normally I try to mimic the behaviour of glibc. Then I test on other
> > platforms BSD, OpenSolaris if they differ. They often did the same as
> > glibc.
>
> I checked again with POSIX.1-2008 [1]. It has the exact same wording as the
> man-pages I quoted.
>
> So how do we proceed from here? Shall we try to change musl's behaviour,
> because it does not conform to GNU libc? Is conformance to any valid
> interpretation of POSIX.1-2008 acceptable? Or is just my understanding of
> the specification of getaddrinfo and its errors wrong?
>
> > I'm currently checking why BSD and Solaris nightly build are not working.
>
> Were you able to figure out the build problems?
Is there anything I can do to speed this up? Is something about my code still
in a known bad state? Or is all that is left the ambiguousness of POSIX.1-2008
regarding errors of getaddrinfo(), which makes glibc and musl-libc interpret
it differently? Or is just my reading of the spec wrong and musl-libc got it
wrong, too?

--Dennis

signature.asc (696 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: nss_wrapper support for musl-libc

Samba - samba-technical mailing list
In reply to this post by Dennis Schridde
On Monday, 20 February 2017 17:08:33 CEST Dennis Schridde wrote:
> Hi Andreas!

Hi Dennis,


sorry that it too so long. I'm fine with patch 0001 and 0002.


For 0003 I think we should detect if we build with MUSL LIBC during configure
time and have an option in config.h to have 'WITH_MUSL' or something like
that.

Then we can use that to enable or disable features and it should make it more
clean why a check has been added or removed.


For 0004 please add the code to ConfigureChecks.cmake and use NULL.

0005 seems to have hunk which belongs to 0004.



        Andreas

--
Andreas Schneider                   GPG-ID: CC014E3D
Samba Team                             [hidden email]
www.samba.org

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: nss_wrapper support for musl-libc

Samba - samba-technical mailing list
Hi Andreas!

I rebased the patches onto current master and applied your changes, hence the
numbering changed.

On Montag, 29. Mai 2017 15:25:15 CEST Andreas Schneider via samba-technical
wrote:
> On Monday, 20 February 2017 17:08:33 CEST Dennis Schridde wrote:
> sorry that it too so long. I'm fine with patch 0001 and 0002.

Thanks for committing them.


> For 0003 I think we should detect if we build with MUSL LIBC during
> configure time and have an option in config.h to have 'WITH_MUSL' or
> something like that.

There is no way to detect the presence of musl-libc directly.

The manpage (v4.04) for getaddrinfo says:

> If hints.ai_flags includes the AI_CANONNAME flag, then the ai_canonname
field of the first of the addrinfo structures in the returned list is set to
point to the official name of the host.

Notably it says "if" and not "iff".  musl-libc simply provides additional
information, which does not seem wrong.  Hence I would argue that the test
should accept both valid behaviours.  I thereforce added a test to
ConfigureChecks.cmake in patch 0001.

> Then we can use that to enable or disable features and it should make it
> more clean why a check has been added or removed.


> For 0004 please add the code to ConfigureChecks.cmake and use NULL.

Done.  The new patch is 0003.


> 0005 seems to have hunk which belongs to 0004.

On Thu, 26 Jan 2017 10:19:26 +0100 you requested that I should split the patch
on file boundaries instead of logical boundaries:

On Donnerstag, 26. Januar 2017 10:19:26 CEST Andreas Schneider wrote:
> Please make seperate commits for each file.

Did that policy change and I shall return to logical chunks for my patches?


Since I was so far unable to confirm that musl's behaviour regarding
getaddrinfo() and EAI_SERVICE is wrong.  Hence I created a patch to deal with
this difference in the same manner as in the other patches.  The new patch is
0002.


Please be aware that 2b80c4ce0be8a43fb6ea6abcd4f97105982e239a breaks musl.  
test_nwrap_vector will hang indefinitely in a call to futex() after loading
tests/libnss_nwrap.so.

Best regards,
Dennis

0003-tests-Add-musl-libc-1.1-compatibility-gethostent.patch (2K) Download Attachment
0001-tests-Add-musl-libc-1.1-compatibility-getaddrinfo-IP.patch (4K) Download Attachment
0002-tests-Add-musl-libc-1.1-compatibility-getaddrinfo-EA.patch (3K) Download Attachment
signature.asc (696 bytes) Download Attachment
Loading...