On Sat, 18 Apr 2020 at 22:18, Martin Ågren <martin.agren@xxxxxxxxx> wrote: > > On Sat, 18 Apr 2020 at 21:56, Martin Ågren <martin.agren@xxxxxxxxx> 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. > 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
