Patchwork [1/1] stdlib/tst-secure-getenv: handle >64 groups

login
register
mail settings
Submitter Mike Gerow
Date April 4, 2019, 7:20 p.m.
Message ID <20190404192047.GA108564@google.com>
Download mbox | patch
Permalink /patch/32169/
State New
Headers show

Comments

Mike Gerow - April 4, 2019, 7:20 p.m.
This test would fail unnecessarily if the user running it had more than
64 groups since getgroups returns EINVAL if the size provided is less
than the number of supplementary group IDs. Instead dynamically
determine the number of supplementary groups the user has.
---
 stdlib/tst-secure-getenv.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)
Florian Weimer - April 5, 2019, 9:56 a.m.
* Mike Gerow:

> This test would fail unnecessarily if the user running it had more than
> 64 groups since getgroups returns EINVAL if the size provided is less
> than the number of supplementary group IDs. Instead dynamically
> determine the number of supplementary groups the user has.

Yes, that's indeed a test bug.

> ---
>  stdlib/tst-secure-getenv.c | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/stdlib/tst-secure-getenv.c b/stdlib/tst-secure-getenv.c
> index 74580b889a..178dfa0439 100644
> --- a/stdlib/tst-secure-getenv.c
> +++ b/stdlib/tst-secure-getenv.c
> @@ -41,8 +41,14 @@ static char MAGIC_ARGUMENT[] = "run-actual-test";
>  static gid_t
>  choose_gid (void)
>  {
> -  const int count = 64;
> -  gid_t groups[count];
> +  int count = getgroups (0, NULL);
> +  if (count < 0)
> +    {
> +      printf ("getgroups: %m\n");
> +      exit (1);
> +    }
> +  gid_t *groups;
> +  groups = malloc (count * sizeof (*groups));

This should be xcalloc (or perhaps xreallocarray, if we had support for
that).

>    int ret = getgroups (count, groups);

Technically, you would also need to add a loop to retry because the
probing for size could have returned an outdated value.

Thanks,
Florian
Mike Gerow - April 5, 2019, 7:46 p.m.
On Fri, Apr 05, 2019 at 11:56:28AM +0200, Florian Weimer wrote:
> * Mike Gerow:
> > ---
> >  stdlib/tst-secure-getenv.c | 19 +++++++++++++++----
> >  1 file changed, 15 insertions(+), 4 deletions(-)
> >
> > diff --git a/stdlib/tst-secure-getenv.c b/stdlib/tst-secure-getenv.c
> > index 74580b889a..178dfa0439 100644
> > --- a/stdlib/tst-secure-getenv.c
> > +++ b/stdlib/tst-secure-getenv.c
> > @@ -41,8 +41,14 @@ static char MAGIC_ARGUMENT[] = "run-actual-test";
> >  static gid_t
> >  choose_gid (void)
> >  {
> > -  const int count = 64;
> > -  gid_t groups[count];
> > +  int count = getgroups (0, NULL);
> > +  if (count < 0)
> > +    {
> > +      printf ("getgroups: %m\n");
> > +      exit (1);
> > +    }
> > +  gid_t *groups;
> > +  groups = malloc (count * sizeof (*groups));
> 
> This should be xcalloc (or perhaps xreallocarray, if we had support for
> that).
Sounds good, I'll change it to xaclloc.
> 
> >    int ret = getgroups (count, groups);
> 
> Technically, you would also need to add a loop to retry because the
> probing for size could have returned an outdated value.
Considering (at least on linux) the groups are a property of the process
it seems pretty unlikely that this is something we need to worry about,
at least in the context of a test. If you insist, though, I'm happy to
handle this case.
Florian Weimer - April 5, 2019, 7:50 p.m.
* Mike Gerow:

>> Technically, you would also need to add a loop to retry because the
>> probing for size could have returned an outdated value.

> Considering (at least on linux) the groups are a property of the process
> it seems pretty unlikely that this is something we need to worry about,
> at least in the context of a test. If you insist, though, I'm happy to
> handle this case.

You are right.  For a single-threaded test, this is completely
unnecessary.  I confused it with the NSS APIs.

Thanks,
Florian

Patch

diff --git a/stdlib/tst-secure-getenv.c b/stdlib/tst-secure-getenv.c
index 74580b889a..178dfa0439 100644
--- a/stdlib/tst-secure-getenv.c
+++ b/stdlib/tst-secure-getenv.c
@@ -41,8 +41,14 @@  static char MAGIC_ARGUMENT[] = "run-actual-test";
 static gid_t
 choose_gid (void)
 {
-  const int count = 64;
-  gid_t groups[count];
+  int count = getgroups (0, NULL);
+  if (count < 0)
+    {
+      printf ("getgroups: %m\n");
+      exit (1);
+    }
+  gid_t *groups;
+  groups = malloc (count * sizeof (*groups));
   int ret = getgroups (count, groups);
   if (ret < 0)
     {
@@ -50,12 +56,17 @@  choose_gid (void)
       exit (1);
     }
   gid_t current = getgid ();
+  gid_t not_current = 0;
   for (int i = 0; i < ret; ++i)
     {
       if (groups[i] != current)
-	return groups[i];
+        {
+          not_current = groups[i];
+          break;
+        }
     }
-  return 0;
+  free (groups);
+  return not_current;
 }