On Sat, Jun 30, 2018 at 7:45 PM Rob Landley <r...@landley.net> wrote:
>
> On 06/28/2018 06:41 PM, enh wrote:
> > In theory, both getpwuid_r and getgrgid_r need to loop until the buffer
> > is large enough. In practice, that's true for me with getgrgid_r.
>
> A fixed size 512 byte allocation is pretty small. It should probably have been
> page size, but since the list never flushes I was worried a big find / might
> wind up with hundreds of them... but then any given system should only have so
> many total
>
> (I'd have used libbuf there and then copied the data out, but it's hard to
> strdup() a struct group *. No obvious size indicator.)
>
> > Even
> > though the old hard-coded pw buffer size was enough for me, I've fixed
> > that too on the assumption that there's someone out there that needs
> > the general code.
>
> Sigh. If I had used a 4096 byte allocation instead of 512 I doubt you'd ever
> have hit this. :P

oh, you'd be surprised. i need many *megabytes* of space.

> These twio functions irritate me because it's the same code with trivially
> different data types. It's even using the same structure offsets and putting 
> the
> same stuff on the stack, just using a different static list pointer and 
> calling
> a different function pointer. Grumble grumble duplication...

templates, dude. templates.

> Hmmm... a group can have an arbitrary number of users belong to it, so the
> gr_mem field is a list of unbounded length, so yes getgrgid_r can get
> arbitrarily long. But I'm not sure the same is true of getpwuid? The list of
> groups this user's a member of is also arbitrarily long, but isn't in there.
> That's getgrouplist(), which is using toybuf in id.c rather than looping
> expansion logic, which implies either 4k is enough or that needs to be fixed 
> too.

(checks.) yeah, that's just under 1KiB for me right now.

at least, if/when the time comes, getgrouplist(3) actually tells you
how much space you need.

> So the only way for the struct passwd size to blow up is for the string 
> members
> to be arbitrarily long, which is possible but I've never seen it. (And in 
> theory
> they'd cap at PATH_MAX anyway? A 4k allocation would almost certainly cover 
> it...)
>
> Sigh, but I might as well keep the two functions matching in case I do work 
> out
> a way to collapse them together later.

you didn't change enough in the groups code. i'm surprised it doesn't
SEGV for you too.

fix for the crashes attached. i've also restored the doubling behavior
for groups on the assumption that there's a bimodal distribution out
there: folks on single-user machines only need a tiny buffer, while
folks on large corporate networks need ridiculously large buffers.



> Also, this "loop until big enough" thing seems like there should be some 
> generic
> way to do it (it comes up for readlink() too), but the call to use the data's
> awkwardly placed.
>
> Rob
From 4de04d64b4f85f3cd319865f773886aee651ea08 Mon Sep 17 00:00:00 2001
From: Elliott Hughes <e...@google.com>
Date: Mon, 2 Jul 2018 11:06:57 -0700
Subject: [PATCH] Fix bufgetgrgid logic, and grow faster.

---
 lib/lib.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/lib/lib.c b/lib/lib.c
index 473163b..f635e18 100644
--- a/lib/lib.c
+++ b/lib/lib.c
@@ -1244,7 +1244,7 @@ struct passwd *bufgetpwuid(uid_t uid)
   return &list->pw;
 }
 
-// Return cached passwd entries.
+// Return cached group entries.
 struct group *bufgetgrgid(gid_t gid)
 {
   struct grgidbuf_list {
@@ -1253,15 +1253,16 @@ struct group *bufgetgrgid(gid_t gid)
   } *list = 0;
   struct group *temp;
   static struct grgidbuf_list *grgidbuf;
-  int size = 0;
+  int size = 256;
 
   for (list = grgidbuf; list; list = list->next)
     if (list->gr.gr_gid == gid) return &(list->gr);
 
   for (;;) {
-    list = xrealloc(temp, size += 512);
+    // Grow faster for group entries because they can be many megabytes in size.
+    list = xrealloc(list, size *= 2);
     errno = getgrgid_r(gid, &list->gr, sizeof(*list)+(char *)list,
-      4096-sizeof(*list), &temp);
+      size-sizeof(*list), &temp);
     if (errno != ERANGE) break;
   }
   if (!temp) {
-- 
2.18.0.399.gad0ab374a1-goog

_______________________________________________
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net

Reply via email to