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

From: Martin Ågren <martin.agren@gmail.com>
Date: Apr 18, 2020, 20:18 UTC
Message-ID: <cover.1587240635.git.martin.agren@gmail.com>
In-Reply-To: <CAN0heSppn6BBX4V1T1qgKc4XP+8i6qbcEqd1_3NqWQtZJLaJww@mail.gmail.com>

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

Show 8 quoted lines

> > -       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 think `strbuf_attach()` is sufficiently hard to use that we might as well simplify it for the majority of the users that don't care about the distinction between the string's length and the size of the allocated memory, and avoid it for the few remaining users that are just as well off using `strbuf_add()`.

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?

Another alternative would be to first convert certain users away from the function, then simplify it for the remainder. I preferred to first spend a few patches converting a few existing callers to make it clear that those are ok.

Martin

Martin Ågren (6):
  am: use `strbuf_attach()` correctly
  strbuf_attach: correctly pass in `strlen() + 1` for `alloc`
  strbuf: use `strbuf_attach()` correctly
  fast-import: avoid awkward use of `strbuf_attach()`
  rerere: avoid awkward use of `strbuf_attach()`
  strbuf: simplify `strbuf_attach()` usage

 strbuf.h             | 16 ++++++++--------
 apply.c              |  2 +-
 archive.c            |  2 +-
 blame.c              |  2 +-
 builtin/am.c         |  2 +-
 convert.c            |  4 ++--
 fast-import.c        |  7 ++++---
 imap-send.c          |  2 +-
 mailinfo.c           |  2 +-
 merge-recursive.c    |  2 +-
 path.c               |  3 +--
 pretty.c             |  4 ++--
 refs/files-backend.c |  2 +-
 rerere.c             |  3 ++-
 strbuf.c             | 11 +++++++----
 trailer.c            |  2 +-
 16 files changed, 35 insertions(+), 31 deletions(-)

-- 
2.26.1