Hi,
sorry for being late to the party, i just started to look at this issue.
Obviously, there are multiple bugs here, so as Todd suggested, i think
the fixes will be done in multiple steps. Here, i'm preliminarily
addressing the non-YP part.
Philip Guenther wrote on Wed, Feb 05, 2014 at 04:31:16PM -0800:
> There's more needed. There are actually three cases:
> 1) found a match:
> getpwnam(): return non-NULL (Linux and Solaris set errno to 0)
> getpwnam_r(): set *pwretp to non-NULL and return 0
> 2) no match, but no error:
> getpwnam(): set errno to zero and return NULL
> getpwnam_r(): set *pwretp to NULL and return 0
> 3) something failed:
> getpwnam(): set errno to non-zero-errno-value and return NULL
> getpwnam_r(): set *pwretp to NULL and return non-zero-errno-value
>
> On Solaris, getpwnam_r() doesn't change errno; on Linux, getpwnam_r() sets
> errno to its return value.
>
> So: we need to
> - clear errno in the success and "no match" cases,
This is incorrect and shouldn't be done.
When no error occurs, errno has to stay as it is.
POSIX explicitly says: If the user code wants to distinguish "no match"
and "error", the *user code* must clear errno before calling getpwnam().
If user code calls getpwnam() without caring whether or not errno
is set and then assumes getpwnam() failed if it is (still?) set
after getpwnam() returns, that user code is buggy, and the bug
should be reported to upstream and fixed upstream.
I do not consider "Linux does this" or "Solaris does that" a good
reason to implement incorrect behaviour in our C library.
> - need to set errno in the ERANGE case,
> - need to return errno from getpw{uid,nam}_r() on failure, and
> - need to preserve errno around calls whose failure doesn't affect the
> success or failure of the lookup (e.g., the DB close)
Yes.
> Diff below does that, though I haven't checked the YP lookup closely to
> verify whether it might return the errno of the wrong system call on
> failure.
As Todd says, that can be done in a later step.
Some specific comments inline:
> Index: gen/getpwent.c
> ===================================================================
> RCS file: /cvs/src/lib/libc/gen/getpwent.c,v
> retrieving revision 1.48
> diff -u -p -r1.48 getpwent.c
> --- gen/getpwent.c 15 Nov 2013 22:32:55 -0000 1.48
> +++ gen/getpwent.c 6 Feb 2014 00:15:46 -0000
> @@ -672,8 +672,11 @@ _pwhashbyname(const char *name, char *bu
> int r;
>
> len = strlen(name);
> - if (len > _PW_NAME_LEN)
> + if (len > _PW_NAME_LEN) {
> + /* can't be present, so just treat as "no match" */
> + errno = 0;
> return (NULL);
> + }
> bf[0] = _PW_KEYBYNAME;
> bcopy(name, &bf[1], MIN(len, _PW_NAME_LEN));
> key.data = (u_char *)bf;
That code is correct, your change makes it incorrect.
> @@ -707,7 +710,7 @@ getpwnam_r(const char *name, struct pass
> struct passwd **pwretp)
> {
> struct passwd *pwret = NULL;
> - int flags = 0, *flagsp;
> + int flags = 0, *flagsp, saved_errno;
>
> _THREAD_PRIVATE_MUTEX_LOCK(pw);
> if (!_pw_db && !__initdb())
> @@ -727,14 +730,16 @@ getpwnam_r(const char *name, struct pass
> pwret = _pwhashbyname(name, buf, buflen, pw, flagsp);
>
> if (!_pw_stayopen) {
> + saved_errno = errno;
> (void)(_pw_db->close)(_pw_db);
> _pw_db = NULL;
> + errno = saved_errno;
> }
> fail:
> if (pwretp)
> *pwretp = pwret;
> _THREAD_PRIVATE_MUTEX_UNLOCK(pw);
> - return (pwret ? 0 : 1);
> + return (pwret ? 0 : errno);
> }
>
> struct passwd *
These chunks make sense to me; i will read to code to check
completeness and do some testing in order to provide a real OK.
> @@ -742,8 +747,11 @@ getpwnam(const char *name)
> {
> struct passwd *pw = NULL;
>
> + /* getpwnam_r sets errno on failure */
> if (getpwnam_r(name, &_pw_passwd, _pw_string, sizeof _pw_string, &pw))
> pw = NULL;
> + else
> + errno = 0;
> return (pw);
> }
>
That code is correct and your change makes it incorrect.
Even if we really wanted to clear errno, which i don't think we should,
it should only be done inside getpwnam_r(), not all over the place.
Besides, this chunk contradicts what you said above:
> The one thing this diff does *not* do is set errno to zero when
> a match is found. Both Linux and Solaris do that, so maybe we
> should too...
This chunk *would* do just that.
> @@ -752,7 +760,7 @@ getpwuid_r(uid_t uid, struct passwd *pw,
> struct passwd **pwretp)
> {
> struct passwd *pwret = NULL;
> - int flags = 0, *flagsp;
> + int flags = 0, *flagsp, saved_errno;
>
> _THREAD_PRIVATE_MUTEX_LOCK(pw);
> if (!_pw_db && !__initdb())
> @@ -772,14 +780,16 @@ getpwuid_r(uid_t uid, struct passwd *pw,
> pwret = _pwhashbyuid(uid, buf, buflen, pw, flagsp);
>
> if (!_pw_stayopen) {
> + saved_errno = errno;
> (void)(_pw_db->close)(_pw_db);
> _pw_db = NULL;
> + errno = saved_errno;
> }
> fail:
> if (pwretp)
> *pwretp = pwret;
> _THREAD_PRIVATE_MUTEX_UNLOCK(pw);
> - return (pwret ? 0 : 1);
> + return (pwret ? 0 : errno);
> }
>
> struct passwd *
Again, makes sense.
> @@ -787,8 +797,11 @@ getpwuid(uid_t uid)
> {
> struct passwd *pw = NULL;
>
> + /* getpwuid_r sets errno on failure */
> if (getpwuid_r(uid, &_pw_passwd, _pw_string, sizeof _pw_string, &pw))
> pw = NULL;
> + else
> + errno = 0;
> return (pw);
> }
>
Again, please don't.
> @@ -851,8 +864,11 @@ __initdb(void)
> errno = saved_errno;
> return (1);
> }
> - if (!warned)
> + if (!warned) {
> + saved_errno = errno;
> syslog(LOG_ERR, "%s: %m", _PATH_MP_DB);
> + errno = saved_errno;
> + }
> warned = 1;
> return (0);
> }
Makes sense.
> @@ -863,12 +879,20 @@ __hashpw(DBT *key, char *buf, size_t buf
> {
> char *p, *t;
> DBT data;
> + int ret;
>
> - if ((_pw_db->get)(_pw_db, key, &data, 0))
> + if ((ret = (_pw_db->get)(_pw_db, key, &data, 0))) {
> + if (ret == 1) {
> + /* not found, but not an error */
> + errno = 0;
> + }
> return (0);
> + }
No, please don't.
> p = (char *)data.data;
> - if (data.size > buflen)
> + if (data.size > buflen) {
> + errno = ERANGE;
> return (0);
> + }
Makes sense.
In a further step, we also have to document the possible
errnos in a new ERRORS section in the manual page.
Yours,
Ingo
