[PATCH] nss_compat: internal_end*ent may clobber errno, hiding ERANGE [BZ #25976]

[DJ Delorie]

Florian Weimer via Libc-alpha <libc-alpha@sourceware.org> writes:
> During cleanup, before returning from get*_r functions, the end*ent
> calls must not change errno.  Otherwise, an ERANGE error from the
> underlying implementation can be hidden, causing unexpected lookup
> failures.  This commit introduces an internal_end*ent_noerror
> function which saves and restore errno, and marks the original
> internal_end*ent function as warn_unused_result, so that it is used
> only in contexts were errors from it can be handled explicitly.

Just to be clear - this should ONLY affect the cases where we've already
failed (or completed, where that step is the relevent one) some other
step, and we're just cleaning up, so the "hidden" call shouldn't affect
anything?

Patch is LGTM as-is but I have some suggestions for minor tweaks below.

Reviewed-by: DJ Delorie <dj@redhat.com>

> -static enum nss_status
> +static enum nss_status __attribute__ ((warn_unused_result))

Ok.  Some parts of the source still use __attribute_warn_unused_result__ though.

> +/* Like internal_endgrent, but preserve errno in all cases.  */

Should mention something about why we're not returning a status, but I'm
not sure how best to state that.  Maybe ", but only for cleanups where
we don't need the return status and errno must be preserved" ?

> +static void
> +internal_endgrent_noerror (ent_t *ent)
> +{
> +  int saved_errno = errno;
> +  enum nss_status unused __attribute__ ((unused)) = internal_endgrent (ent);

Can we suppress this warning with the typical "(void) internal_endgrent
(ent);" ?

(otherwise ok)

> @@ -485,7 +494,7 @@ _nss_compat_getgrnam_r (const char *name, struct group *grp,
>    if (result == NSS_STATUS_SUCCESS)
>      result = internal_getgrnam_r (name, grp, &ent, buffer, buflen, errnop);
>  
> -  internal_endgrent (&ent);
> +  internal_endgrent_noerror (&ent);

Cleanup, ok.

> @@ -614,7 +623,7 @@ _nss_compat_getgrgid_r (gid_t gid, struct group *grp,
>    if (result == NSS_STATUS_SUCCESS)
>      result = internal_getgrgid_r (gid, grp, &ent, buffer, buflen, errnop);
>  
> -  internal_endgrent (&ent);
> +  internal_endgrent_noerror (&ent);

cleanup, ok.

> -static enum nss_status
> +static enum nss_status __attribute__ ((warn_unused_result))
>  internal_endgrent (ent_t *ent)

Ok.

> +/* Like internal_endgrent, but preserve errno in all cases.  */
> +static void
> +internal_endgrent_noerror (ent_t *ent)
> +{
> +  int saved_errno = errno;
> +  enum nss_status unused __attribute__ ((unused)) = internal_endgrent (ent);
> +  __set_errno (saved_errno);
> +}

Same comments as above

> @@ -502,7 +511,7 @@ _nss_compat_initgroups_dyn (const char *user, gid_t group, long int *start,
>   done:
>    scratch_buffer_free (&tmpbuf);
>  
> -  internal_endgrent (&intern);
> +  internal_endgrent_noerror (&intern);

Ok.

Rest of the patch is rinse-repeat...