dir: fix malloc of root untracked_cache_dir

From: Jeff Hostetler <[email protected]>

Use FLEX_ALLOC_STR() to allocate the struct untracked_cache_dir for the root directory. Get rid of unsafe code that might fail to initialize the name field (if FLEX_ARRAY is not 1). This will make it clear that we intend to have a structure with an empty string following it.

From: Jeff Hostetler <[email protected]>

Use FLEX_ALLOC_STR() to allocate the struct untracked_cache_dir for the root directory. Get rid of unsafe code that might fail to initialize the name field (if FLEX_ARRAY is not 1). This will make it clear that we intend to have a structure with an empty string following it.

A problem was observed on Windows where the length of the memset() was too short, so the first byte of the name field was not zeroed. This resulted in the name field having garbage from a previous use of that area of memory.

The record for the root directory was then written to the untracked-cache extension in the index. This garbage would then be visible to future commands when they reloaded the untracked-cache extension.

Since the directory record for the root directory had garbage in the name field, the t/helper/test-tool dump-untracked-cache tool printed this garbage as the path prefix (rather than '/') for each directory in the untracked cache as it recursed.

Signed-off-by Jeff Hostetler <[email protected]>

---
dir: fix malloc of root untracked_cache_dir

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-884%2Fjeffhostetler%2Funtracked-cache-corruption-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-884/jeffhostetler/untracked-cache-corruption-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/884

 dir.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

dir.c +2 -5

index d153a63bbd14..fd8aa7c40faa 100644
@@ -2730,11 +2730,8 @@ static struct untracked_cache_dir *validate_untracked_cache(struct dir_struct *d
 		return NULL;
 	}
 
-	if (!dir->untracked->root) {
-		const int len = sizeof(*dir->untracked->root);
-		dir->untracked->root = xmalloc(len);
-		memset(dir->untracked->root, 0, len);
-	}
+	if (!dir->untracked->root)
+		FLEX_ALLOC_STR(dir->untracked->root, name, "");
 
 	/* Validate $GIT_DIR/info/exclude and core.excludesfile */
 	root = dir->untracked->root;

base-commit: 966e671106b2fd38301e7c344c754fd118d0bb07

Quoted 5 lines

Use FLEX_ALLOC_STR() to allocate the struct untracked_cache_dir for the root directory. Get rid of unsafe code that might fail to initialize the name field (if FLEX_ARRAY is not 1). This will make it clear that we intend to have a structure with an empty string following it.

[...]
The problematic code was introduced in 2015, a year before these FLEX_ALLOC_*() helpers were introduced. The result is of course correct and much easier to read, as the necessary "ask for a region of calloc'ed memory with an additional byte for terminating NUL beyond strlen()" is hidden in the helper.

Quoted 11 lines · Junio C Hamano

Quoted 5 lines

Use FLEX_ALLOC_STR() to allocate the struct untracked_cache_dir for the root directory. Get rid of unsafe code that might fail to initialize the name field (if FLEX_ARRAY is not 1). This will make it clear that we intend to have a structure with an empty string following it.

[...]
The problematic code was introduced in 2015, a year before these FLEX_ALLOC_*() helpers were introduced. The result is of course correct and much easier to read, as the necessary "ask for a region of calloc'ed memory with an additional byte for terminating NUL beyond strlen()" is hidden in the helper.

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).

Quoted 6 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.