[Toybox] [PATCH] bufgetgrgid: fix for very large groups.

[Rob Landley]

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

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...

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.

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.

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