[PATCH] _nss_nis_initgroups_dyn: Use struct scratch_buffer [BZ #18023]

[PATCH] _nss_nis_initgroups_dyn: Use struct scratch_buffer [BZ #18023]

[Florian Weimer]

Remove extend_alloca usage.  Also adjusts the internal function get_uid.

2018-06-25  Florian Weimer  <fweimer@redhat.com>

	[BZ #18023]
	* nis/nss_nis/nis-initgroups.c (get_uid, _nss_nis_initgroups_dyn):
	Use struct scratch_buffer instead of extend_alloca.

diff --git a/nis/nss_nis/nis-initgroups.c b/nis/nss_nis/nis-initgroups.c
index a47b4d7ada..ca043d4e5a 100644
--- a/nis/nss_nis/nis-initgroups.c
+++ b/nis/nss_nis/nis-initgroups.c
@@ -16,7 +16,6 @@
    License along with the GNU C Library; if not, see
    <http://www.gnu.org/licenses/>.  */
 
-#include <alloca.h>
 #include <ctype.h>
 #include <errno.h>
 #include <grp.h>
@@ -27,6 +26,7 @@
 #include <rpcsvc/yp.h>
 #include <rpcsvc/ypclnt.h>
 #include <sys/param.h>
+#include <scratch_buffer.h>
 
 #include "nss-nis.h"
 #include <libnsl.h>
@@ -120,27 +120,30 @@ internal_getgrent_r (struct group *grp, char *buffer, size_t buflen,
 static int
 get_uid (const char *user, uid_t *uidp)
 {
-  size_t buflen = sysconf (_SC_GETPW_R_SIZE_MAX);
-  char *buf = (char *) alloca (buflen);
+  struct scratch_buffer tmpbuf;
+  scratch_buffer_init (&tmpbuf);
 
   while (1)
     {
       struct passwd result;
       struct passwd *resp;
 
-      int r = getpwnam_r (user, &result, buf, buflen, &resp);
+      int r = getpwnam_r (user, &result, tmpbuf.data, tmpbuf.length, &resp);
       if (r == 0 && resp != NULL)
 	{
 	  *uidp = resp->pw_uid;
+	  scratch_buffer_free (&tmpbuf);
 	  return 0;
 	}
 
       if (r != ERANGE)
 	break;
 
-      buf = extend_alloca (buf, buflen, 2 * buflen);
+      if (!scratch_buffer_grow (&tmpbuf))
+	return 1;
     }
 
+  scratch_buffer_free (&tmpbuf);
   return 1;
 }
 
@@ -254,8 +257,6 @@ _nss_nis_initgroups_dyn (const char *user, gid_t group, long int *start,
     }
 
   struct group grpbuf, *g;
-  size_t buflen = sysconf (_SC_GETPW_R_SIZE_MAX);
-  char *tmpbuf;
   enum nss_status status;
   intern_t intern = { NULL, NULL, 0 };
   gid_t *groups = *groupsp;
@@ -264,15 +265,20 @@ _nss_nis_initgroups_dyn (const char *user, gid_t group, long int *start,
   if (status != NSS_STATUS_SUCCESS)
     return status;
 
-  tmpbuf = __alloca (buflen);
+  struct scratch_buffer tmpbuf;
+  scratch_buffer_init (&tmpbuf);
 
   while (1)
     {
       while ((status =
-	      internal_getgrent_r (&grpbuf, tmpbuf, buflen, errnop,
+	      internal_getgrent_r (&grpbuf, tmpbuf.data, tmpbuf.length, errnop,
 				   &intern)) == NSS_STATUS_TRYAGAIN
              && *errnop == ERANGE)
-	tmpbuf = extend_alloca (tmpbuf, buflen, 2 * buflen);
+	if (!scratch_buffer_grow (&tmpbuf))
+	  {
+	    status = NSS_STATUS_TRYAGAIN;
+	    goto done;
+	  }
 
       if (status != NSS_STATUS_SUCCESS)
 	{
@@ -331,6 +337,7 @@ done:
       intern.start = intern.start->next;
       free (intern.next);
     }
+  scratch_buffer_free (&tmpbuf);
 
   return status;
 }

Re: [PATCH] _nss_nis_initgroups_dyn: Use struct scratch_buffer [BZ #18023]

[DJ Delorie]

fweimer@redhat.com (Florian Weimer) writes:
> -      buf = extend_alloca (buf, buflen, 2 * buflen);
> +      if (!scratch_buffer_grow (&tmpbuf))
> +	return 1;

The docs say if _grow() fails, the buffer is still free'able.  Could we
not, or should we, use "break" here so that scratch_buffer_free() is
called when grow() fails?

The docs (scratch_buffer.h) mentions that the old buffer is freed, but
that the scratch_buffer is still "in a freeable state" so it's not clear
if the intention is that you *should* call scratch_buffer_free(), or
*must*, or *must not*, etc.

>      }
>  
> +  scratch_buffer_free (&tmpbuf);
>    return 1;
>  }


> +	if (!scratch_buffer_grow (&tmpbuf))
> +	  {
> +	    status = NSS_STATUS_TRYAGAIN;
> +	    goto done;
> +	  }

In this case, going to "done" *does* call scratch_buffer_free().  At
least we should be consistent within a given file ;-)

>        if (status != NSS_STATUS_SUCCESS)
>  	{
> @@ -331,6 +337,7 @@ done:
>        intern.start = intern.start->next;
>        free (intern.next);
>      }
> +  scratch_buffer_free (&tmpbuf);
>  
>    return status;
>  }

The code itself looks OK, the above are just consistency and
clarification things.

Re: [PATCH] _nss_nis_initgroups_dyn: Use struct scratch_buffer [BZ #18023]

[Florian Weimer]

On 06/25/2018 10:37 PM, DJ Delorie wrote:
> 
> fweimer@redhat.com (Florian Weimer) writes:
>> -      buf = extend_alloca (buf, buflen, 2 * buflen);
>> +      if (!scratch_buffer_grow (&tmpbuf))
>> +	return 1;
> 
> The docs say if _grow() fails, the buffer is still free'able.  Could we
> not, or should we, use "break" here so that scratch_buffer_free() is
> called when grow() fails?
> 
> The docs (scratch_buffer.h) mentions that the old buffer is freed, but
> that the scratch_buffer is still "in a freeable state" so it's not clear
> if the intention is that you *should* call scratch_buffer_free(), or
> *must*, or *must not*, etc.

You can free the buffer again, but you don't have to.  There is no 
memory leak if you don't.

> 
>>       }
>>   
>> +  scratch_buffer_free (&tmpbuf);
>>     return 1;
>>   }

The point here is not call free if we know it is a no-op.

So I think the code is okay as-is.

>> +	if (!scratch_buffer_grow (&tmpbuf))
>> +	  {
>> +	    status = NSS_STATUS_TRYAGAIN;
>> +	    goto done;
>> +	  }
> 
> In this case, going to "done" *does* call scratch_buffer_free().  At
> least we should be consistent within a given file ;-)

Here we call the entire cleanup sequence, which happens to include 
scratch_buffer_free as well, so the situation is different.

I hope this clarifies why things are the way they are.

Thanks,
Florian

Re: [PATCH] _nss_nis_initgroups_dyn: Use struct scratch_buffer [BZ #18023]

[DJ Delorie]

Florian Weimer <fweimer@redhat.com> writes:
> You can free the buffer again, but you don't have to.  There is no 
> memory leak if you don't.
>
> The point here is not call free if we know it is a no-op.
>
> So I think the code is okay as-is.

Agreed, just checking ;-)

>> In this case, going to "done" *does* call scratch_buffer_free().  At
>> least we should be consistent within a given file ;-)
>
> Here we call the entire cleanup sequence, which happens to include 
> scratch_buffer_free as well, so the situation is different.

Ok, as long as the inconsistency was intentional and reasoned, I'm OK
with it.