Re: [PATCH] dir: fix malloc of root untracked_cache_dir

From: Jeff King <peff@peff.net>
Date: Feb 24, 2021, 23:51 UTC
Message-ID: <YDbmgXVKVsog6K+r@coredump.intra.peff.net>
In-Reply-To: <581ece66-6a93-eda6-67bf-3bb49f553dfc@jeffhostetler.com>

On Wed, Feb 24, 2021 at 04:15:11PM -0500, Jeff Hostetler wrote:

Show 13 quoted lines

> > When I added the FLEX_ALLOC_* helpers, I audited for existing callers to
> > convert. But I did so by looking for places where we were doing manual
> > size computations. The bug here was that it was not doing any
> > computation at all (when it need to be doing "+1"). So that's my guess
> > why it got overlooked (which is not super important, but may give a hint
> > about how to look for similar bugs).
> 
> There's another call to xmalloc() at [1] that does the st_add3()
> thing that I didn't change.  It was correctly computing the size
> so I didn't want to change it for no reason.  That and the 2 memcpy()s
> made it feel like we should have a different FLEX_ macro for this
> case -- more of a FLEX_DUP... or something.  But I didn't want to
> distract from my bug fix here.

Thanks for pointing it out. Even if we change it, I agree it's better to keep it out of your existing bugfix patch.

But I do think that spot gets weird. We are copying all of the elements of the struct _except_ the name field, which comes from somewhere else. So it's tempting to do:

diff --git a/dir.c b/dir.c
index d153a63bbd..ab16db3f76 100644
--- a/dir.c
+++ b/dir.c
@@ -3287,9 +3287,9 @@ static int read_one_dir(struct untracked_cache_dir **untracked_,
 	if (!eos || eos == end)
 		return -1;
 
-	*untracked_ = untracked = xmalloc(st_add3(sizeof(*untracked), eos - data, 1));
+	FLEX_ALLOC_MEM(untracked, name, data, eos - data);
 	memcpy(untracked, &ud, sizeof(ud));
-	memcpy(untracked->name, data, eos - data + 1);
+	*untracked_ = untracked;
 	data = eos + 1;
 
 	for (i = 0; i < untracked->untracked_nr; i++) {

But that's wrong! The remaining memcpy using sizeof(ud) might or might
not touch the first byte of the name field, depending on struct padding
and the value of FLEX_ARRAY. And if it does, then we'd be overwriting
the early bytes of that field with whatever was in "ud" (which I guess
is uninitialized cruft, because that is a stack variable).

So it would have to be:

  memcpy(untracked, &ud, offsetof(struct untracked_cache_dir, name));

Quite subtle. Even with a comment, I think I prefer the existing code.
If we had a macro as you suggest for "allocate a flex struct, dup most of
the contents from this other struct, and then copy in the flex bytes
from this other spot", that would make it more readable. But I'm not
sure we'd find more than one spot to use such a macro. :)

-Peff