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

[enh]

On Sat, Jun 30, 2018 at 7:45 PM Rob Landley <rob at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Fix-bufgetgrgid-logic-and-grow-faster.patch
Type: text/x-patch
Size: 1286 bytes
Desc: not available
URL: <http://lists.landley.net/pipermail/toybox-landley.net/attachments/20180702/a9306e0d/attachment-0002.bin>