Bug 23410 - getpwent() can set errno to ENOENT which is not documented
Summary: getpwent() can set errno to ENOENT which is not documented
Status: RESOLVED INVALID
Alias: None
Product: glibc
Classification: Unclassified
Component: nss (show other bugs)
Version: 2.27
: P2 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-07-13 16:50 UTC by Laurent Bigonville
Modified: 2018-07-27 15:43 UTC (History)
3 users (show)

See Also:
Host:
Target:
Build:
Last reconfirmed:
fweimer: security-


Attachments
Test case (162 bytes, text/x-csrc)
2018-07-13 16:50 UTC, Laurent Bigonville
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Laurent Bigonville 2018-07-13 16:50:31 UTC
Created attachment 11127 [details]
Test case

Hi,

A recent discussion on the selinux mailing list showed that in some conditions getpwent() can set errno to ENOENT.

But this is not documented anywhere.

The manpage for getpwent(3), shows the following possible errors:

       EINTR  A signal was caught; see signal(7).

       EIO    I/O error.

       EMFILE The per-process limit on the number of open file descriptors has been reached.

       ENFILE The system-wide limit on the total number of open files has been reached.

       ENOMEM Insufficient memory to allocate passwd structure.

       ERANGE Insufficient buffer space supplied.

I attached a simple test case I can reproduce this with (you need to have the nss-systemd module loaded for the passwd database)

Apparently a similar bug report was opened in the past but was closed as INVALID, see bug #1969

[0] https://marc.info/?l=selinux&m=153149269702678&w=2
Comment 1 Nicolas Iooss 2018-07-14 15:38:52 UTC
Hi,
I have dug into systemd's code to understand where this ENOENT comes from. It appears it is intentional: _nss_systemd_getpwent_r() sets errno to ENOENT if there is no item to return (https://github.com/systemd/systemd/blob/v239/src/nss-systemd/nss-systemd.c#L781):

        if (!p) {
                *errnop = ENOENT;
                ret = NSS_STATUS_NOTFOUND;
                goto finalize;
        }

If returning with errno=ENOENT is acceptable, its semantic I agree its semantic should appear in the manpage of getpwent(3). Otherwise, this should be reported to systemd's developers.

Moreover "getent passwd" does not report any failure from getpwent (in https://sourceware.org/git/?p=glibc.git;a=blob;f=nss/getent.c;hb=glibc-2.27#l583), which makes this issue a little bit harder to debug than it should (it requires a C program which checks errno after getting NULL from getpwent()). Would a patch which displays the error from getpwent() in getent.c be accepted?

Thanks
Comment 2 Andreas Schwab 2018-07-14 16:32:38 UTC
NSS_STATUS_NOTFOUND is not an error condition and the nss module must not modify errno.  Please report that to systemd.
Comment 3 Nicolas Iooss 2018-07-15 10:01:39 UTC
Thanks for your feedback.  I reported this issue to systemd: https://github.com/systemd/systemd/issues/9585
Comment 4 Laurent Bigonville 2018-07-15 12:53:06 UTC
Well reading:

https://www.gnu.org/software/libc/manual/html_node/NSS-Modules-Interface.html

Returning NSS_STATUS_NOTFOUND and setting errno to ENOENT seems valid for a NSS module

OTOH, getpwent(3) setting ENOENT is NOT documented

So either the documentation needs to be fixed and/or clarified or getpwent() should reset errno
Comment 5 Carlos O'Donell 2018-07-16 16:24:40 UTC
(In reply to Laurent Bigonville from comment #4)
> Well reading:
> 
> https://www.gnu.org/software/libc/manual/html_node/NSS-Modules-Interface.html
> 
> Returning NSS_STATUS_NOTFOUND and setting errno to ENOENT seems valid for a
> NSS module

It's valid in the general sense, and the above documentation is a high-level overview of the NSS interface. However, the above is not a specific discussion of each interface being implemented. And the documentation calls that out:
~~~
These are proposed values. There can be other error codes and the described error codes can have different meaning.
~~~
 
> OTOH, getpwent(3) setting ENOENT is NOT documented

Correct, because semantically it doesn't make sense. You either provide an entry or if there are no more entries you return NULL.

The deeper question is: Is it valid for getpwent() to fail even if the implementation of NSS service module fails?

I'd argue it doesn't make sense for the NSS service module to expose this error because the caller can't recover from it except to abort.

> So either the documentation needs to be fixed and/or clarified or getpwent()
> should reset errno

Is the NSS_STATUS_NOTFOUND + ENOENT a *real* internal error condition for systemd's NSS service module?

Is systemd trying to indicate that retrieving the next entry resulted in an error that it could not recover from?

In the case of implementing getpwent it might be better for it to return NULL and no error.
Comment 6 Filipe Brandenburger 2018-07-16 17:24:39 UTC
Proposed patch to systemd here:

https://github.com/systemd/systemd/pull/9587/files

Your feedback would be welcome there, in particular there's a question of whether to set *errnop = 0; explicitly or just leave it to whatever the caller set it to... (Does that matter?)

Updating those docs would be appreciated, as they currently stand, it's unclear whether (NSS_STATUS_NOTFOUND + ENOENT) is even valid at all and, if it is, in which situations should it be used (over NSS_STATUS_NOTFOUND + 0 errno or keeping errno untouched.)

Cheers,
Filipe
Comment 7 Carlos O'Donell 2018-07-16 17:46:52 UTC
(In reply to Filipe Brandenburger from comment #6)
> Proposed patch to systemd here:
> 
> https://github.com/systemd/systemd/pull/9587/files
> 
> Your feedback would be welcome there, in particular there's a question of
> whether to set *errnop = 0; explicitly or just leave it to whatever the
> caller set it to... (Does that matter?)

It matters. Do not set it to zero. Please leave it if no real error is being reported.
 
> Updating those docs would be appreciated, as they currently stand, it's
> unclear whether (NSS_STATUS_NOTFOUND + ENOENT) is even valid at all and, if
> it is, in which situations should it be used (over NSS_STATUS_NOTFOUND + 0
> errno or keeping errno untouched.)

Thanks, we will keep this under consideration when updating the manual.
Comment 8 Andreas Schwab 2018-07-16 18:48:46 UTC
No library function should ever set errno to zero.