diff mbox

Return proper status from _nss_nis_initgroups_dyn (bug 20262)

Message ID mvmr3bxw7m8.fsf@hawking.suse.de
State Committed
Headers show

Commit Message

Andreas Schwab June 16, 2016, 2:06 p.m. UTC
[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(-)

Comments

Florian Weimer June 23, 2016, 4:50 p.m. UTC | #1
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
Andreas Schwab June 27, 2016, 7:17 a.m. UTC | #2
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.
Florian Weimer June 28, 2016, 1:16 p.m. UTC | #3
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
Andreas Schwab June 28, 2016, 2:21 p.m. UTC | #4
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.
Florian Weimer June 28, 2016, 2:30 p.m. UTC | #5
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
Andreas Schwab June 28, 2016, 2:40 p.m. UTC | #6
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.
Florian Weimer June 30, 2016, 10:26 a.m. UTC | #7
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
Andreas Schwab June 30, 2016, 10:47 a.m. UTC | #8
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.
Florian Weimer June 30, 2016, 11:13 a.m. UTC | #9
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
Andreas Schwab June 30, 2016, 11:48 a.m. UTC | #10
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.
Florian Weimer June 30, 2016, 11:51 a.m. UTC | #11
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
diff mbox

Patch

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)