Re: [PATCH 0/6] strbuf: simplify `strbuf_attach()` usage

From: Martin Ågren <martin.agren@gmail.com>
Date: Apr 19, 2020, 04:44 UTC
Message-ID: <CAN0heSrzVfb1=xUReZidBVhE427r7MRPtZ_RDOorXESq4r-wwQ@mail.gmail.com>
In-Reply-To: <cover.1587240635.git.martin.agren@gmail.com>

On Sat, 18 Apr 2020 at 22:18, Martin Ågren <martin.agren@gmail.com> wrote:

Show 13 quoted lines

>
> On Sat, 18 Apr 2020 at 21:56, Martin Ågren <martin.agren@gmail.com> wrote:
> > > -       strbuf_attach(line, out, strlen(out), strlen(out));
> > > +       strbuf_attach(line, out, out_len, out_len);
> >
> > This conversion is ok as such. I wondered why we pass in the same
> > value twice (before and after this patch). Turns out this usage is wrong
> > (as per the documentation in strbuf.h) but safe (as per my understanding
> > of the implementation in strbuf.c). I'll follow up with a series that
> > fell out of that investigation.
>
> Here's that series. It could go in parallel to Danh's, or one could go
> on top of the other. Any of those would be ok with me.

I realize now that some of the reasoning in this series is incorrect. `strbuf_grow()` will use `ALLOC_GROW` meaning we might reallocate cheaply rather than doing an entire allocate+copy+free cycle.

Still, there is some buggyness in the usage because we might reallocate (with a 50% overhead) even when we shouldn't really need to.

I'll resubmit this after tackling it from a different angle: I'll introduce `strbuf_attachstr()`, simplifying and robustifyng most users. Then I'll switch a few incorrect users of `strbuf_attach()` to `strbuf_attachstr()`. Then a small number of users will continue using `strbuf_attach()` with `alloc == len` for $reasons.

Show 11 quoted lines

> The summary is that this function takes `len` and `alloc`, where the
> latter must be greater than the former, yet several callers use the same
> value for both. I first thought this could cause quite hairy problems
> such as writing outside of allocated memory, but it doesn't look that
> way. See the patches for more information.
>
> An alternative to the approach taken here would be to introduce
> `strbuf_attachstr()` and convert most existing users, then convert the
> few remaining ones to use the new function or to move in another
> direction. But the new name is a downer -- what else would you attach to
> a strbuf if not a string?

So this is what I'll do instead. The reasoning is, you can attach a string (a NUL-terminated buffer) or a non-string (non-NUL-terminated buffer).

Martin