> Date: Mon, 24 Feb 2014 16:51:42 +0100
> From: Ingo Schwarze <schwa...@usta.de>
>
> 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.
While I agree this is a step in the right direction, and probably not
going to break stuff, I'm not sure this is the time to change this.
It does change the behaviour, and some ports will end up in codepaths
that have not been tested on OpenBSD.
> 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]
