Hi Stuart,
Stuart Henderson wrote on Sun, Feb 23, 2014 at 02:22:47PM +0000:
> btw, here are results from a search for getpw(nam|uid)_r in unpacked
> ports source, after removing a bunch of autoconf checks etc.
Wow, these are nearly a hundred ports. It is not feasible to audit
all that. Besides, my patches also imply slight changes to the
behaviour of getgr{nam,gid}_r() (when called with errno already set)
and to getpw{nam,uid}() (which may now set errno even when finding
a match and will no longer set errno with the third patch in that
case). So your list is too long for a complete audit even though
it is incomplete.
Anyway, i looked at seven examples from your list, choosing ports
from different categories. The results are, if i read the code
correctly:
- I found no cases where my patches might break something.
- One of the seven ports would be significantly improved (postfix).
- Three ports get minor improvements (gdm, glib2, ruby).
- For three ports, it makes no difference (gmake, postgres, gnutls).
- Nearly all changes in behaviour are due to the first patch -
change of the return values - and among these, nearly all
are due to changing the return value from 1 to 0 when not
finding a match, but when there is no error.
- The errno variable and the exact return value (as opposed to
merely whether it's 0 or non-0) are rarely looked at.
- For detailed results, look at the very end of this mail.
Even after unlock, i don't think it would be reasonable to try to
evaluate all the consequences of the changes. After unlock, we
should just commit all the changes such that libc behaves reasonably,
then watch out for any fallout.
What do we commit now?
After this review, i'd say:
* PLEASE somebody give me an OK just for the first patch.
It actually improves various ports.
It doesn't look like it's going to break stuff.
(Theo said he is not opposed to fixing part of this,
but cannot look at the details right now.)
* Let's not commit the second patch
(avoid errno clobbering as suggested by guenther@).
It doesn't seem to matter much, so let's avoid any risk.
* Let's not commit the third patch
(let getpw*_r() not touch errno as suggested by kettenis@).
Even though i didn't find any port where this might matter,
i still consider it more risky than the first two patches.
Here is the first patch, again.
Yours,
Ingo
Index: getpwent.c
===================================================================
RCS file: /cvs/src/lib/libc/gen/getpwent.c,v
retrieving revision 1.48
diff -u -p -r1.48 getpwent.c
--- getpwent.c 15 Nov 2013 22:32:55 -0000 1.48
+++ getpwent.c 19 Feb 2014 16:36:48 -0000
@@ -708,8 +708,12 @@ getpwnam_r(const char *name, struct pass
{
struct passwd *pwret = NULL;
int flags = 0, *flagsp;
+ int my_errno = 0;
+ int saved_errno;
_THREAD_PRIVATE_MUTEX_LOCK(pw);
+ saved_errno = errno;
+ errno = 0;
if (!_pw_db && !__initdb())
goto fail;
@@ -733,8 +737,12 @@ getpwnam_r(const char *name, struct pass
fail:
if (pwretp)
*pwretp = pwret;
+ if (pwret == NULL)
+ my_errno = errno;
+ if (!errno)
+ errno = saved_errno;
_THREAD_PRIVATE_MUTEX_UNLOCK(pw);
- return (pwret ? 0 : 1);
+ return (my_errno);
}
struct passwd *
@@ -753,8 +761,12 @@ getpwuid_r(uid_t uid, struct passwd *pw,
{
struct passwd *pwret = NULL;
int flags = 0, *flagsp;
+ int my_errno = 0;
+ int saved_errno;
_THREAD_PRIVATE_MUTEX_LOCK(pw);
+ saved_errno = errno;
+ errno = 0;
if (!_pw_db && !__initdb())
goto fail;
@@ -778,8 +790,12 @@ getpwuid_r(uid_t uid, struct passwd *pw,
fail:
if (pwretp)
*pwretp = pwret;
+ if (pwret == NULL)
+ my_errno = errno;
+ if (!errno)
+ errno = saved_errno;
_THREAD_PRIVATE_MUTEX_UNLOCK(pw);
- return (pwret ? 0 : 1);
+ return (my_errno);
}
struct passwd *
Index: getgrent.c
===================================================================
RCS file: /cvs/src/lib/libc/gen/getgrent.c,v
retrieving revision 1.38
diff -u -p -r1.38 getgrent.c
--- getgrent.c 17 Apr 2013 17:40:35 -0000 1.38
+++ getgrent.c 19 Feb 2014 16:36:49 -0000
@@ -134,6 +134,7 @@ getgrnam_r(const char *name, struct grou
if (bufsize < GETGR_R_SIZE_MAX)
return ERANGE;
errnosave = errno;
+ errno = 0;
*result = getgrnam_gs(name, grp, (struct group_storage *)buffer);
if (*result == NULL)
ret = errno;
@@ -180,6 +181,7 @@ getgrgid_r(gid_t gid, struct group *grp,
if (bufsize < GETGR_R_SIZE_MAX)
return ERANGE;
errnosave = errno;
+ errno = 0;
*result = getgrgid_gs(gid, grp, (struct group_storage *)buffer);
if (*result == NULL)
ret = errno;
----- 8< ----- schnipp ----- >8 ----- 8< ----- schnapp ----- >8 -----
AUDIT RESULTS:
+ means improved behaviour; [1] means due to the first patch
= means unchanged behaviour
- would mean my patches would break something (nothing found!)
postfix
-------
+ non-existant users are now deferred, will be bounced right away [1]
+ When a user having a UID that doesn't exist in the password database
calls the postfix sendmail(1), the process stalls in an endless loop.
After the change, the user will be treated as "unknown" [1]
gdm
---
+ no longer calls bogus g_warning when username does not exist [1]
+ calls g_warning with the right error string in case of errors [1]
glib2
-----
= mostly ignores return values and errno, anyway
+ but at least improves some g_warning() calls [1]
ruby
----
= tests return value for != 0 only and ignores errno
+ for "no match", will call rb_raise(can't find user), not rb_sys_fail [1]
gmake
-----
= no effect if i read the code correctly
postgres, gnutls
----------------
= completely ignore return values and errno, anyway
----- 8< ----- schnipp ----- >8 ----- 8< ----- schnapp ----- >8 -----
THIS IS WHAT I AUDITED FOR:
FIRST PATCH
-----------
* not found without error: -> some major positive effects
OLD: return 1
NEW: return 0
* not found with error: -> some minor positive effects
OLD: return 1
NEW: return errno
- also affects return value of getgrnam_r() and getgrgid_r()
when called with errno set -> not searched for by sthen@
SECOND PATCH -> no effects found
------------
* close() failure
OLD: sets errno, no effect on return value
NEW: does not set errno, no effect on return value
* syslog() failure in __initdb()
OLD: overwrites errno from dbopen(), return 1
NEW: preserves errno from dbopen and returns it
also affects __intdb() + syslog() failure in getpwent()
* called with insufficient buffer:
OLD: return 1, errno unchanged
NEW: return ERANGE, errno = ERANGE unless third patch
THIRD PATCH -> no effects found
-----------
* getpw{nam,uid}_r():
OLD: errno set on failure
NEW: errno never set
- getpw{nam,uid}() found a result:
OLD: errno may be set
NEW: errno never set
AUDIT TASK SUMMARY
------------------
* return 1 -> 0: not found without error [1] -> important
* return 1 -> errno: not found with error [1] -> minor effects
* return 1 -> ERANGE: insufficient buffer [2]
* errno not -> ERANGE: insufficient buffer [2]
* errno set -> not: any failure [3]
* errno set -> not: close() failure [2]
* errno syslog() -> dbopen(): syslog() failure [2]
