Message ID | 20190404192047.GA108564@google.com |
---|---|
State | New |
Headers | show |
Series | [1/1] stdlib/tst-secure-getenv: handle >64 groups | expand |
* 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
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.
* 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
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; }