* [PATCH] _nss_nis_initgroups_dyn: Return error status @ 2015-02-26 14:47 Florian Weimer 2015-03-01 19:18 ` Mike Frysinger ` (2 more replies) 0 siblings, 3 replies; 17+ messages in thread From: Florian Weimer @ 2015-02-26 14:47 UTC (permalink / raw) To: GNU C Library This seems to have been an oversight. An error can be returned if the function is left with âgoto done;â. 2015-02-26 Florian Weimer <fweimer@redhat.com> * nis/nss_nis/nis-initgroups.c (_nss_nis_initgroups_dyn): Return error status. diff --git a/nis/nss_nis/nis-initgroups.c b/nis/nss_nis/nis-initgroups.c index d22b241..ed5c26b 100644 --- a/nis/nss_nis/nis-initgroups.c +++ b/nis/nss_nis/nis-initgroups.c @@ -326,5 +326,5 @@ done: free (intern.next); } - return NSS_STATUS_SUCCESS; + return status; } -- Florian Weimer / Red Hat Product Security ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] _nss_nis_initgroups_dyn: Return error status 2015-02-26 14:47 [PATCH] _nss_nis_initgroups_dyn: Return error status Florian Weimer @ 2015-03-01 19:18 ` Mike Frysinger 2016-06-16 9:31 ` Andreas Schwab 2016-06-16 14:06 ` [PATCH] Return proper status from _nss_nis_initgroups_dyn (bug 20262) Andreas Schwab 2 siblings, 0 replies; 17+ messages in thread From: Mike Frysinger @ 2015-03-01 19:18 UTC (permalink / raw) To: Florian Weimer; +Cc: GNU C Library [-- Attachment #1: Type: text/plain, Size: 177 bytes --] On 26 Feb 2015 15:47, Florian Weimer wrote: > This seems to have been an oversight. An error can be returned if the > function is left with “goto done;”. lgtm -mike [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] _nss_nis_initgroups_dyn: Return error status 2015-02-26 14:47 [PATCH] _nss_nis_initgroups_dyn: Return error status Florian Weimer 2015-03-01 19:18 ` Mike Frysinger @ 2016-06-16 9:31 ` Andreas Schwab 2016-06-16 9:35 ` Florian Weimer 2016-06-16 14:06 ` [PATCH] Return proper status from _nss_nis_initgroups_dyn (bug 20262) Andreas Schwab 2 siblings, 1 reply; 17+ messages in thread From: Andreas Schwab @ 2016-06-16 9:31 UTC (permalink / raw) To: Florian Weimer; +Cc: GNU C Library Florian Weimer <fweimer@redhat.com> writes: > This seems to have been an oversight. An error can be returned if the > function is left with âgoto done;â. > > 2015-02-26 Florian Weimer <fweimer@redhat.com> > > * nis/nss_nis/nis-initgroups.c (_nss_nis_initgroups_dyn): > Return error status. > > diff --git a/nis/nss_nis/nis-initgroups.c b/nis/nss_nis/nis-initgroups.c > index d22b241..ed5c26b 100644 > --- a/nis/nss_nis/nis-initgroups.c > +++ b/nis/nss_nis/nis-initgroups.c > @@ -326,5 +326,5 @@ done: > free (intern.next); > } > > - return NSS_STATUS_SUCCESS; > + return status; > } This causes _nss_nis_initgroups_dyn to always return NSS_STATUS_NOTFOUND. Andreas. -- Andreas Schwab, SUSE Labs, schwab@suse.de GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7 "And now for something completely different." ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] _nss_nis_initgroups_dyn: Return error status 2016-06-16 9:31 ` Andreas Schwab @ 2016-06-16 9:35 ` Florian Weimer 2016-06-16 11:24 ` Andreas Schwab 0 siblings, 1 reply; 17+ messages in thread From: Florian Weimer @ 2016-06-16 9:35 UTC (permalink / raw) To: Andreas Schwab; +Cc: GNU C Library On 06/16/2016 11:31 AM, Andreas Schwab wrote: > Florian Weimer <fweimer@redhat.com> writes: > >> This seems to have been an oversight. An error can be returned if the >> function is left with âgoto done;â. >> >> 2015-02-26 Florian Weimer <fweimer@redhat.com> >> >> * nis/nss_nis/nis-initgroups.c (_nss_nis_initgroups_dyn): >> Return error status. >> >> diff --git a/nis/nss_nis/nis-initgroups.c b/nis/nss_nis/nis-initgroups.c >> index d22b241..ed5c26b 100644 >> --- a/nis/nss_nis/nis-initgroups.c >> +++ b/nis/nss_nis/nis-initgroups.c >> @@ -326,5 +326,5 @@ done: >> free (intern.next); >> } >> >> - return NSS_STATUS_SUCCESS; >> + return status; >> } > > This causes _nss_nis_initgroups_dyn to always return > NSS_STATUS_NOTFOUND. What is the expectation behavior of this function? Always return NSS_STATUS_NOTFOUND, even in case of an error? Thanks, Florian ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] _nss_nis_initgroups_dyn: Return error status 2016-06-16 9:35 ` Florian Weimer @ 2016-06-16 11:24 ` Andreas Schwab 0 siblings, 0 replies; 17+ messages in thread From: Andreas Schwab @ 2016-06-16 11:24 UTC (permalink / raw) To: Florian Weimer; +Cc: GNU C Library Florian Weimer <fweimer@redhat.com> writes: > What is the expectation behavior of this function? Return NSS_STATUS_SUCCESS when done. Andreas. -- Andreas Schwab, SUSE Labs, schwab@suse.de GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7 "And now for something completely different." ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH] Return proper status from _nss_nis_initgroups_dyn (bug 20262) 2015-02-26 14:47 [PATCH] _nss_nis_initgroups_dyn: Return error status Florian Weimer 2015-03-01 19:18 ` Mike Frysinger 2016-06-16 9:31 ` Andreas Schwab @ 2016-06-16 14:06 ` Andreas Schwab 2016-06-23 16:51 ` Florian Weimer 2 siblings, 1 reply; 17+ messages in thread From: Andreas Schwab @ 2016-06-16 14:06 UTC (permalink / raw) To: Florian Weimer; +Cc: GNU C Library [BZ #20262] * nis/nss_nis/nis-initgroups.c (_nss_nis_initgroups_dyn): Return NSS_STATUS_SUCCESS when done. Return NSS_STATUS_TRYAGAIN when out of memory. --- nis/nss_nis/nis-initgroups.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/nis/nss_nis/nis-initgroups.c b/nis/nss_nis/nis-initgroups.c index dec385c..0368667 100644 --- a/nis/nss_nis/nis-initgroups.c +++ b/nis/nss_nis/nis-initgroups.c @@ -266,7 +266,7 @@ _nss_nis_initgroups_dyn (const char *user, gid_t group, long int *start, tmpbuf = __alloca (buflen); - do + while (1) { while ((status = internal_getgrent_r (&grpbuf, tmpbuf, buflen, errnop, @@ -275,8 +275,11 @@ _nss_nis_initgroups_dyn (const char *user, gid_t group, long int *start, tmpbuf = extend_alloca (tmpbuf, buflen, 2 * buflen); if (status != NSS_STATUS_SUCCESS) - goto done; - + { + if (status == NSS_STATUS_NOTFOUND) + status = NSS_STATUS_SUCCESS; + goto done; + } g = &grpbuf; if (g->gr_gid != group) @@ -304,7 +307,11 @@ _nss_nis_initgroups_dyn (const char *user, gid_t group, long int *start, newgroups = realloc (groups, newsize * sizeof (*groups)); if (newgroups == NULL) - goto done; + { + status = NSS_STATUS_TRYAGAIN; + *errnop = errno; + goto done; + } *groupsp = groups = newgroups; *size = newsize; } @@ -316,7 +323,6 @@ _nss_nis_initgroups_dyn (const char *user, gid_t group, long int *start, } } } - while (status == NSS_STATUS_SUCCESS); done: while (intern.start != NULL) -- 2.9.0 -- Andreas Schwab, SUSE Labs, schwab@suse.de GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7 "And now for something completely different." ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] Return proper status from _nss_nis_initgroups_dyn (bug 20262) 2016-06-16 14:06 ` [PATCH] Return proper status from _nss_nis_initgroups_dyn (bug 20262) Andreas Schwab @ 2016-06-23 16:51 ` Florian Weimer 2016-06-27 7:17 ` Andreas Schwab 0 siblings, 1 reply; 17+ messages in thread From: Florian Weimer @ 2016-06-23 16:51 UTC (permalink / raw) To: Andreas Schwab; +Cc: GNU C Library On 06/16/2016 04:06 PM, Andreas Schwab wrote: > [BZ #20262] > * nis/nss_nis/nis-initgroups.c (_nss_nis_initgroups_dyn): Return > NSS_STATUS_SUCCESS when done. Return NSS_STATUS_TRYAGAIN when out > of memory. Thanks for the patch. What about this loop exit? if (limit > 0 && *size == limit) /* We reached the maximum. */ goto done; Shouldn't the caller somehow learn about truncation? Is the internal initgroups_dyn interface used by anything else but nscd? Florian ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] Return proper status from _nss_nis_initgroups_dyn (bug 20262) 2016-06-23 16:51 ` Florian Weimer @ 2016-06-27 7:17 ` Andreas Schwab 2016-06-28 13:16 ` Florian Weimer 0 siblings, 1 reply; 17+ messages in thread From: Andreas Schwab @ 2016-06-27 7:17 UTC (permalink / raw) To: Florian Weimer; +Cc: GNU C Library Florian Weimer <fweimer@redhat.com> writes: > On 06/16/2016 04:06 PM, Andreas Schwab wrote: >> [BZ #20262] >> * nis/nss_nis/nis-initgroups.c (_nss_nis_initgroups_dyn): Return >> NSS_STATUS_SUCCESS when done. Return NSS_STATUS_TRYAGAIN when out >> of memory. > > Thanks for the patch. > > What about this loop exit? > > if (limit > 0 && *size == limit) > /* We reached the maximum. */ > goto done; > > Shouldn't the caller somehow learn about truncation? This is a system limit (NGROUPS_MAX), so I don't think the caller can do anything about it anyway. > Is the internal initgroups_dyn interface used by anything else but nscd? This is about NSS, nscd is just the cache. Andreas. -- Andreas Schwab, SUSE Labs, schwab@suse.de GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7 "And now for something completely different." ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] Return proper status from _nss_nis_initgroups_dyn (bug 20262) 2016-06-27 7:17 ` Andreas Schwab @ 2016-06-28 13:16 ` Florian Weimer 2016-06-28 14:21 ` Andreas Schwab 0 siblings, 1 reply; 17+ messages in thread From: Florian Weimer @ 2016-06-28 13:16 UTC (permalink / raw) To: Andreas Schwab; +Cc: GNU C Library On 06/27/2016 09:17 AM, Andreas Schwab wrote: > Florian Weimer <fweimer@redhat.com> writes: > >> On 06/16/2016 04:06 PM, Andreas Schwab wrote: >>> [BZ #20262] >>> * nis/nss_nis/nis-initgroups.c (_nss_nis_initgroups_dyn): Return >>> NSS_STATUS_SUCCESS when done. Return NSS_STATUS_TRYAGAIN when out >>> of memory. >> >> Thanks for the patch. >> >> What about this loop exit? >> >> if (limit > 0 && *size == limit) >> /* We reached the maximum. */ >> goto done; >> >> Shouldn't the caller somehow learn about truncation? > > This is a system limit (NGROUPS_MAX), so I don't think the caller can do > anything about it anyway. Silent truncation still seems to be wrong. But it's an unrelated issue. >> Is the internal initgroups_dyn interface used by anything else but nscd? > > This is about NSS, nscd is just the cache. I'm trying to figure out what the impact is. Is it restricted to incorrect merging of data with the next service module, even in situations where NIS should be the sole data source? Thanks, Florian ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] Return proper status from _nss_nis_initgroups_dyn (bug 20262) 2016-06-28 13:16 ` Florian Weimer @ 2016-06-28 14:21 ` Andreas Schwab 2016-06-28 14:30 ` Florian Weimer 0 siblings, 1 reply; 17+ messages in thread From: Andreas Schwab @ 2016-06-28 14:21 UTC (permalink / raw) To: Florian Weimer; +Cc: GNU C Library Florian Weimer <fweimer@redhat.com> writes: > I'm trying to figure out what the impact is. If initgroups_dyn returns NSS_STATUS_NOTFOUND then initgroups falls back to group enumeration. Andreas. -- Andreas Schwab, SUSE Labs, schwab@suse.de GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7 "And now for something completely different." ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] Return proper status from _nss_nis_initgroups_dyn (bug 20262) 2016-06-28 14:21 ` Andreas Schwab @ 2016-06-28 14:30 ` Florian Weimer 2016-06-28 14:40 ` Andreas Schwab 0 siblings, 1 reply; 17+ messages in thread From: Florian Weimer @ 2016-06-28 14:30 UTC (permalink / raw) To: Andreas Schwab; +Cc: GNU C Library On 06/28/2016 04:21 PM, Andreas Schwab wrote: > Florian Weimer <fweimer@redhat.com> writes: > >> I'm trying to figure out what the impact is. > > If initgroups_dyn returns NSS_STATUS_NOTFOUND then initgroups falls back > to group enumeration. Now I'm really confused. You are changing the group enumeration code, after the potential call to initgroups_netid. Florian ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] Return proper status from _nss_nis_initgroups_dyn (bug 20262) 2016-06-28 14:30 ` Florian Weimer @ 2016-06-28 14:40 ` Andreas Schwab 2016-06-30 10:26 ` Florian Weimer 0 siblings, 1 reply; 17+ messages in thread From: Andreas Schwab @ 2016-06-28 14:40 UTC (permalink / raw) To: Florian Weimer; +Cc: GNU C Library Florian Weimer <fweimer@redhat.com> writes: > Now I'm really confused. You are changing the group enumeration code, > after the potential call to initgroups_netid. I don't understand this sentence at all. Andreas. -- Andreas Schwab, SUSE Labs, schwab@suse.de GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7 "And now for something completely different." ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] Return proper status from _nss_nis_initgroups_dyn (bug 20262) 2016-06-28 14:40 ` Andreas Schwab @ 2016-06-30 10:26 ` Florian Weimer 2016-06-30 10:47 ` Andreas Schwab 0 siblings, 1 reply; 17+ messages in thread From: Florian Weimer @ 2016-06-30 10:26 UTC (permalink / raw) To: Andreas Schwab; +Cc: GNU C Library On 06/28/2016 04:40 PM, Andreas Schwab wrote: > Florian Weimer <fweimer@redhat.com> writes: > >> Now I'm really confused. You are changing the group enumeration code, >> after the potential call to initgroups_netid. > > I don't understand this sentence at all. For context, I've been trying to reproduce these issue and how it affects results returned by “id” and “getent initgroups”. Regarding the most recent discussion, I believe the direct query support for NIS is in the function initgroups_netid (starting around line 150 in nis/nss_nis/nis-initgroups.c). If this cannot be used, nss_nis falls back to enumeration of the group database (lines 256 onwards, in _nss_nis_initgroups_dyn), gathering group IDs of those groups which contain the user (check is at line 287). The broken status handling only affects this fallback code (group enumeration). Based on how this is called, I do not see any other forms of fallback. So I found your comment that the existing bug causes fallback to group enumeration most puzzling. Rather, it seems to me that the caller ignores the error code and proceeds to merge the results even if _nss_nis_initgroups_dyn returned NSS_STATUS_NOTFOUND. Thanks, Florian ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] Return proper status from _nss_nis_initgroups_dyn (bug 20262) 2016-06-30 10:26 ` Florian Weimer @ 2016-06-30 10:47 ` Andreas Schwab 2016-06-30 11:13 ` Florian Weimer 0 siblings, 1 reply; 17+ messages in thread From: Andreas Schwab @ 2016-06-30 10:47 UTC (permalink / raw) To: Florian Weimer; +Cc: GNU C Library Florian Weimer <fweimer@redhat.com> writes: > The broken status handling only affects this fallback code (group > enumeration). Based on how this is called, I do not see any other forms > of fallback. See nss_compat/compat-initgroups.c. Andreas. -- Andreas Schwab, SUSE Labs, schwab@suse.de GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7 "And now for something completely different." ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] Return proper status from _nss_nis_initgroups_dyn (bug 20262) 2016-06-30 10:47 ` Andreas Schwab @ 2016-06-30 11:13 ` Florian Weimer 2016-06-30 11:48 ` Andreas Schwab 0 siblings, 1 reply; 17+ messages in thread From: Florian Weimer @ 2016-06-30 11:13 UTC (permalink / raw) To: Andreas Schwab; +Cc: GNU C Library On 06/30/2016 12:47 PM, Andreas Schwab wrote: > Florian Weimer <fweimer@redhat.com> writes: > >> The broken status handling only affects this fallback code (group >> enumeration). Based on how this is called, I do not see any other forms >> of fallback. > > See nss_compat/compat-initgroups.c. Thanks. Is this just a performance issue, or can it alter results returned to the application (say if the NIS database and local files are inconsistent)? Florian ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] Return proper status from _nss_nis_initgroups_dyn (bug 20262) 2016-06-30 11:13 ` Florian Weimer @ 2016-06-30 11:48 ` Andreas Schwab 2016-06-30 11:51 ` Florian Weimer 0 siblings, 1 reply; 17+ messages in thread From: Andreas Schwab @ 2016-06-30 11:48 UTC (permalink / raw) To: Florian Weimer; +Cc: GNU C Library Florian Weimer <fweimer@redhat.com> writes: > On 06/30/2016 12:47 PM, Andreas Schwab wrote: >> Florian Weimer <fweimer@redhat.com> writes: >> >>> The broken status handling only affects this fallback code (group >>> enumeration). Based on how this is called, I do not see any other forms >>> of fallback. >> >> See nss_compat/compat-initgroups.c. > > Thanks. Is this just a performance issue, or can it alter results > returned to the application (say if the NIS database and local files are > inconsistent)? I'm seeing nscd failing to return the extra groups. Andreas. -- Andreas Schwab, SUSE Labs, schwab@suse.de GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7 "And now for something completely different." ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] Return proper status from _nss_nis_initgroups_dyn (bug 20262) 2016-06-30 11:48 ` Andreas Schwab @ 2016-06-30 11:51 ` Florian Weimer 0 siblings, 0 replies; 17+ messages in thread From: Florian Weimer @ 2016-06-30 11:51 UTC (permalink / raw) To: Andreas Schwab; +Cc: GNU C Library On 06/30/2016 01:48 PM, Andreas Schwab wrote: > Florian Weimer <fweimer@redhat.com> writes: > >> On 06/30/2016 12:47 PM, Andreas Schwab wrote: >>> Florian Weimer <fweimer@redhat.com> writes: >>> >>>> The broken status handling only affects this fallback code (group >>>> enumeration). Based on how this is called, I do not see any other forms >>>> of fallback. >>> >>> See nss_compat/compat-initgroups.c. >> >> Thanks. Is this just a performance issue, or can it alter results >> returned to the application (say if the NIS database and local files are >> inconsistent)? > > I'm seeing nscd failing to return the extra groups. Ah, yes, we have had such a bug report as well (which is why I asked about nscd before). I'm not sure if I said this before, but the patch looks good to me. (The truncation issue is separate and should not block this fix.) Thanks, Florian ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2016-06-30 11:51 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-02-26 14:47 [PATCH] _nss_nis_initgroups_dyn: Return error status Florian Weimer 2015-03-01 19:18 ` Mike Frysinger 2016-06-16 9:31 ` Andreas Schwab 2016-06-16 9:35 ` Florian Weimer 2016-06-16 11:24 ` Andreas Schwab 2016-06-16 14:06 ` [PATCH] Return proper status from _nss_nis_initgroups_dyn (bug 20262) Andreas Schwab 2016-06-23 16:51 ` Florian Weimer 2016-06-27 7:17 ` Andreas Schwab 2016-06-28 13:16 ` Florian Weimer 2016-06-28 14:21 ` Andreas Schwab 2016-06-28 14:30 ` Florian Weimer 2016-06-28 14:40 ` Andreas Schwab 2016-06-30 10:26 ` Florian Weimer 2016-06-30 10:47 ` Andreas Schwab 2016-06-30 11:13 ` Florian Weimer 2016-06-30 11:48 ` Andreas Schwab 2016-06-30 11:51 ` 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).