* [PATCH] nss_compat: internal_end*ent may clobber errno, hiding ERANGE [BZ #25976]
@ 2020-05-14 14:24 Florian Weimer
2020-05-18 5:11 ` Florian Weimer
2020-05-18 20:31 ` DJ Delorie
0 siblings, 2 replies; 6+ messages in thread
From: Florian Weimer @ 2020-05-14 14:24 UTC (permalink / raw)
To: libc-alpha
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.
---
nss/nss_compat/compat-grp.c | 15 ++++++++++++---
nss/nss_compat/compat-initgroups.c | 13 +++++++++++--
nss/nss_compat/compat-pwd.c | 15 ++++++++++++---
nss/nss_compat/compat-spwd.c | 14 +++++++++++---
4 files changed, 46 insertions(+), 11 deletions(-)
diff --git a/nss/nss_compat/compat-grp.c b/nss/nss_compat/compat-grp.c
index 14aadc6f01..f52d764273 100644
--- a/nss/nss_compat/compat-grp.c
+++ b/nss/nss_compat/compat-grp.c
@@ -144,7 +144,7 @@ _nss_compat_setgrent (int stayopen)
}
-static enum nss_status
+static enum nss_status __attribute__ ((warn_unused_result))
internal_endgrent (ent_t *ent)
{
if (ent->stream != NULL)
@@ -165,6 +165,15 @@ internal_endgrent (ent_t *ent)
return NSS_STATUS_SUCCESS;
}
+/* 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);
+}
+
enum nss_status
_nss_compat_endgrent (void)
{
@@ -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);
return result;
}
@@ -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);
return result;
}
diff --git a/nss/nss_compat/compat-initgroups.c b/nss/nss_compat/compat-initgroups.c
index 67a4c100f6..4032be3308 100644
--- a/nss/nss_compat/compat-initgroups.c
+++ b/nss/nss_compat/compat-initgroups.c
@@ -134,7 +134,7 @@ internal_setgrent (ent_t *ent)
}
-static enum nss_status
+static enum nss_status __attribute__ ((warn_unused_result))
internal_endgrent (ent_t *ent)
{
if (ent->stream != NULL)
@@ -158,6 +158,15 @@ internal_endgrent (ent_t *ent)
return NSS_STATUS_SUCCESS;
}
+/* 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);
+}
+
/* Add new group record. */
static void
add_group (long int *start, long int *size, gid_t **groupsp, long int limit,
@@ -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);
return status;
}
diff --git a/nss/nss_compat/compat-pwd.c b/nss/nss_compat/compat-pwd.c
index dfb454f777..69f2ab37db 100644
--- a/nss/nss_compat/compat-pwd.c
+++ b/nss/nss_compat/compat-pwd.c
@@ -261,7 +261,7 @@ _nss_compat_setpwent (int stayopen)
}
-static enum nss_status
+static enum nss_status __attribute__ ((warn_unused_result))
internal_endpwent (ent_t *ent)
{
if (ent->stream != NULL)
@@ -289,6 +289,15 @@ internal_endpwent (ent_t *ent)
return NSS_STATUS_SUCCESS;
}
+/* Like internal_endpwent, but preserve errno in all cases. */
+static void
+internal_endpwent_noerror (ent_t *ent)
+{
+ int saved_errno = errno;
+ enum nss_status unused __attribute__ ((unused)) = internal_endpwent (ent);
+ __set_errno (saved_errno);
+}
+
enum nss_status
_nss_compat_endpwent (void)
{
@@ -824,7 +833,7 @@ _nss_compat_getpwnam_r (const char *name, struct passwd *pwd,
if (result == NSS_STATUS_SUCCESS)
result = internal_getpwnam_r (name, pwd, &ent, buffer, buflen, errnop);
- internal_endpwent (&ent);
+ internal_endpwent_noerror (&ent);
return result;
}
@@ -1063,7 +1072,7 @@ _nss_compat_getpwuid_r (uid_t uid, struct passwd *pwd,
if (result == NSS_STATUS_SUCCESS)
result = internal_getpwuid_r (uid, pwd, &ent, buffer, buflen, errnop);
- internal_endpwent (&ent);
+ internal_endpwent_noerror (&ent);
return result;
}
diff --git a/nss/nss_compat/compat-spwd.c b/nss/nss_compat/compat-spwd.c
index 0a1fde1ea4..908746840d 100644
--- a/nss/nss_compat/compat-spwd.c
+++ b/nss/nss_compat/compat-spwd.c
@@ -217,7 +217,7 @@ _nss_compat_setspent (int stayopen)
}
-static enum nss_status
+static enum nss_status __attribute__ ((warn_unused_result))
internal_endspent (ent_t *ent)
{
if (ent->stream != NULL)
@@ -246,6 +246,15 @@ internal_endspent (ent_t *ent)
return NSS_STATUS_SUCCESS;
}
+/* Like internal_endspent, but preserve errno in all cases. */
+static void
+internal_endspent_noerror (ent_t *ent)
+{
+ int saved_errno = errno;
+ enum nss_status unused __attribute__ ((unused)) = internal_endspent (ent);
+ __set_errno (saved_errno);
+}
+
enum nss_status
_nss_compat_endspent (void)
{
@@ -263,7 +272,6 @@ _nss_compat_endspent (void)
return result;
}
-
static enum nss_status
getspent_next_nss_netgr (const char *name, struct spwd *result, ent_t *ent,
char *group, char *buffer, size_t buflen,
@@ -788,7 +796,7 @@ _nss_compat_getspnam_r (const char *name, struct spwd *pwd,
if (result == NSS_STATUS_SUCCESS)
result = internal_getspnam_r (name, pwd, &ent, buffer, buflen, errnop);
- internal_endspent (&ent);
+ internal_endspent_noerror (&ent);
return result;
}
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] nss_compat: internal_end*ent may clobber errno, hiding ERANGE [BZ #25976]
2020-05-14 14:24 [PATCH] nss_compat: internal_end*ent may clobber errno, hiding ERANGE [BZ #25976] Florian Weimer
@ 2020-05-18 5:11 ` Florian Weimer
2020-05-18 20:31 ` DJ Delorie
1 sibling, 0 replies; 6+ messages in thread
From: Florian Weimer @ 2020-05-18 5:11 UTC (permalink / raw)
To: Florian Weimer via Libc-alpha
* Florian Weimer via Libc-alpha:
> 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.
Ping. This patch needs review:
<https://sourceware.org/pipermail/libc-alpha/2020-May/113950.html>
Thanks,
Florian
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] nss_compat: internal_end*ent may clobber errno, hiding ERANGE [BZ #25976]
2020-05-14 14:24 [PATCH] nss_compat: internal_end*ent may clobber errno, hiding ERANGE [BZ #25976] Florian Weimer
2020-05-18 5:11 ` Florian Weimer
@ 2020-05-18 20:31 ` DJ Delorie
2020-05-19 12:11 ` Florian Weimer
1 sibling, 1 reply; 6+ messages in thread
From: DJ Delorie @ 2020-05-18 20:31 UTC (permalink / raw)
To: Florian Weimer; +Cc: libc-alpha
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...
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] nss_compat: internal_end*ent may clobber errno, hiding ERANGE [BZ #25976]
2020-05-18 20:31 ` DJ Delorie
@ 2020-05-19 12:11 ` Florian Weimer
0 siblings, 0 replies; 6+ messages in thread
From: Florian Weimer @ 2020-05-19 12:11 UTC (permalink / raw)
To: DJ Delorie; +Cc: libc-alpha
* 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?
Yes, the hidden call is just needed to deallocate resources.
(As explained elsewhere, these deallocation functions really should not
fail because it is often impossible to recover from errors on
deallocation paths. But POSIX suggests that end*ent actually can report
failure …)
> Patch is LGTM as-is but I have some suggestions for minor tweaks below.
>
> Reviewed-by: DJ Delorie <dj@redhat.com>
Thanks.
>> -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.
__attribute_warn_unused_result__ is used in public headers. But I wrote
most internal uses of of __attribute__ ((warn_unused_result)), so maybe
that was wrong. I'm going to change it to
__attribute_warn_unused_result__ before pushing.
__wur is yet different again, it's conditional on _FORTIFY_SOURCE, so
it's not appropriate here.
>> +/* 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" ?
I'm going with the shorter description. I'm not entirely sure how this
code will evolve if/when we fix the other errno handling issue.
>> +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);" ?
This idiom is the recommended way, I think. See
<https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66425>.
Thanks,
Florian
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] nss_compat: internal_end*ent may clobber errno, hiding ERANGE [BZ #25976]
2020-05-12 12:52 Florian Weimer
@ 2020-05-14 12:10 ` Florian Weimer
0 siblings, 0 replies; 6+ messages in thread
From: Florian Weimer @ 2020-05-14 12:10 UTC (permalink / raw)
To: Florian Weimer via Libc-alpha
* Florian Weimer via Libc-alpha:
> Change the internal_end*ent functions to preserve errno because they
> are called on error-returning paths, where the value of errno matters.
>
> For the external-facing _nss_compat_end*ent functions, this is not
> required because they always report success, so they may clobber
> errno.
Internal discussion at Red Hat convinced me that the patch does not go
into the right direction, so I'm withdrawing it for now.
Thanks,
Florian
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] nss_compat: internal_end*ent may clobber errno, hiding ERANGE [BZ #25976]
@ 2020-05-12 12:52 Florian Weimer
2020-05-14 12:10 ` Florian Weimer
0 siblings, 1 reply; 6+ messages in thread
From: Florian Weimer @ 2020-05-12 12:52 UTC (permalink / raw)
To: libc-alpha
Change the internal_end*ent functions to preserve errno because they
are called on error-returning paths, where the value of errno matters.
For the external-facing _nss_compat_end*ent functions, this is not
required because they always report success, so they may clobber
errno.
-----
nss/nss_compat/compat-grp.c | 3 +++
nss/nss_compat/compat-initgroups.c | 3 +++
nss/nss_compat/compat-pwd.c | 3 +++
nss/nss_compat/compat-spwd.c | 3 +++
4 files changed, 12 insertions(+)
diff --git a/nss/nss_compat/compat-grp.c b/nss/nss_compat/compat-grp.c
index 14aadc6f01..173c71abd5 100644
--- a/nss/nss_compat/compat-grp.c
+++ b/nss/nss_compat/compat-grp.c
@@ -147,6 +147,8 @@ _nss_compat_setgrent (int stayopen)
static enum nss_status
internal_endgrent (ent_t *ent)
{
+ int saved_errno = errno;
+
if (ent->stream != NULL)
{
fclose (ent->stream);
@@ -162,6 +164,7 @@ internal_endgrent (ent_t *ent)
else
ent->blacklist.current = 0;
+ __set_errno (saved_errno);
return NSS_STATUS_SUCCESS;
}
diff --git a/nss/nss_compat/compat-initgroups.c b/nss/nss_compat/compat-initgroups.c
index 67a4c100f6..60c8af64a3 100644
--- a/nss/nss_compat/compat-initgroups.c
+++ b/nss/nss_compat/compat-initgroups.c
@@ -137,6 +137,8 @@ internal_setgrent (ent_t *ent)
static enum nss_status
internal_endgrent (ent_t *ent)
{
+ int saved_errno = errno;
+
if (ent->stream != NULL)
{
fclose (ent->stream);
@@ -155,6 +157,7 @@ internal_endgrent (ent_t *ent)
if (ent->need_endgrent && endgrent_impl != NULL)
endgrent_impl ();
+ __set_errno (saved_errno);
return NSS_STATUS_SUCCESS;
}
diff --git a/nss/nss_compat/compat-pwd.c b/nss/nss_compat/compat-pwd.c
index dfb454f777..7fbce88e7e 100644
--- a/nss/nss_compat/compat-pwd.c
+++ b/nss/nss_compat/compat-pwd.c
@@ -264,6 +264,8 @@ _nss_compat_setpwent (int stayopen)
static enum nss_status
internal_endpwent (ent_t *ent)
{
+ int saved_errno = errno;
+
if (ent->stream != NULL)
{
fclose (ent->stream);
@@ -286,6 +288,7 @@ internal_endpwent (ent_t *ent)
give_pwd_free (&ent->pwd);
+ __set_errno (saved_errno);
return NSS_STATUS_SUCCESS;
}
diff --git a/nss/nss_compat/compat-spwd.c b/nss/nss_compat/compat-spwd.c
index 0a1fde1ea4..39eed87de8 100644
--- a/nss/nss_compat/compat-spwd.c
+++ b/nss/nss_compat/compat-spwd.c
@@ -220,6 +220,8 @@ _nss_compat_setspent (int stayopen)
static enum nss_status
internal_endspent (ent_t *ent)
{
+ int saved_errno = errno;
+
if (ent->stream != NULL)
{
fclose (ent->stream);
@@ -243,6 +245,7 @@ internal_endspent (ent_t *ent)
give_spwd_free (&ent->pwd);
+ __set_errno (saved_errno);
return NSS_STATUS_SUCCESS;
}
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-05-19 12:11 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-14 14:24 [PATCH] nss_compat: internal_end*ent may clobber errno, hiding ERANGE [BZ #25976] Florian Weimer
2020-05-18 5:11 ` Florian Weimer
2020-05-18 20:31 ` DJ Delorie
2020-05-19 12:11 ` Florian Weimer
-- strict thread matches above, loose matches on Subject: below --
2020-05-12 12:52 Florian Weimer
2020-05-14 12:10 ` Florian Weimer
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).