{"thread":{"id":"55204","subject":"[PATCH] dir: fix malloc of root untracked_cache_dir","startedAt":"2021-02-24T15:51:38Z","lastAt":"2021-02-24T23:52:11Z","messageCount":6,"participants":["Jeff Hostetler via GitGitGadget","Taylor Blau","Junio C Hamano","Jeff King","Jeff Hostetler"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"417676","messageId":"pull.884.git.1614177117508.gitgitgadget@gmail.com","threadId":"55204","inReplyTo":null,"subject":"[PATCH] dir: fix malloc of root untracked_cache_dir","fromName":"Jeff Hostetler via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-02-24T14:31:57Z","receivedAt":"2021-02-24T15:51:38Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"From: Jeff Hostetler <jeffhost@microsoft.com>\n\nUse FLEX_ALLOC_STR() to allocate the `struct untracked_cache_dir`\nfor the root directory.  Get rid of unsafe code that might fail to\ninitialize the `name` field (if FLEX_ARRAY is not 1).  This will\nmake it clear that we intend to have a structure with an empty\nstring following it.\n\nA problem was observed on Windows where the length of the memset() was\ntoo short, so the first byte of the name field was not zeroed.  This\nresulted in the name field having garbage from a previous use of that\narea of memory.\n\nThe record for the root directory was then written to the untracked-cache\nextension in the index.  This garbage would then be visible to future\ncommands when they reloaded the untracked-cache extension.\n\nSince the directory record for the root directory had garbage in the\n`name` field, the `t/helper/test-tool dump-untracked-cache` tool\nprinted this garbage as the path prefix (rather than '/') for each\ndirectory in the untracked cache as it recursed.\n\nSigned-off-by: Jeff Hostetler <jeffhost@microsoft.com>\n---\n    dir: fix malloc of root untracked_cache_dir\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-884%2Fjeffhostetler%2Funtracked-cache-corruption-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-884/jeffhostetler/untracked-cache-corruption-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/884\n\n dir.c | 7 ++-----\n 1 file changed, 2 insertions(+), 5 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex d153a63bbd14..fd8aa7c40faa 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -2730,11 +2730,8 @@ static struct untracked_cache_dir *validate_untracked_cache(struct dir_struct *d\n \t\treturn NULL;\n \t}\n \n-\tif (!dir->untracked->root) {\n-\t\tconst int len = sizeof(*dir->untracked->root);\n-\t\tdir->untracked->root = xmalloc(len);\n-\t\tmemset(dir->untracked->root, 0, len);\n-\t}\n+\tif (!dir->untracked->root)\n+\t\tFLEX_ALLOC_STR(dir->untracked->root, name, \"\");\n \n \t/* Validate $GIT_DIR/info/exclude and core.excludesfile */\n \troot = dir->untracked->root;\n\nbase-commit: 966e671106b2fd38301e7c344c754fd118d0bb07\n-- \ngitgitgadget\n"},{"id":"417682","messageId":"YDaFKSLawYyWhg5x@nand.local","threadId":"55204","inReplyTo":"pull.884.git.1614177117508.gitgitgadget@gmail.com","subject":"Re: [PATCH] dir: fix malloc of root untracked_cache_dir","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-02-24T16:56:09Z","receivedAt":"2021-02-24T16:57:27Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Feb 24, 2021 at 02:31:57PM +0000, Jeff Hostetler via GitGitGadget wrote:\n> From: Jeff Hostetler <jeffhost@microsoft.com>\n>\n> Use FLEX_ALLOC_STR() to allocate the `struct untracked_cache_dir`\n> for the root directory.  Get rid of unsafe code that might fail to\n> initialize the `name` field (if FLEX_ARRAY is not 1).  This will\n> make it clear that we intend to have a structure with an empty\n> string following it.\n\nI had to read this patch message a few times over to make sure that I\nwasn't missing something in the patch itself, but it looks all good to\nme.\n\n  Reviewed-by: Taylor Blau <me@ttaylorr.com>\n\nThanks,\nTaylor\n"},{"id":"417753","messageId":"xmqqk0qxz0s5.fsf@gitster.g","threadId":"55204","inReplyTo":"pull.884.git.1614177117508.gitgitgadget@gmail.com","subject":"Re: [PATCH] dir: fix malloc of root untracked_cache_dir","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-02-24T20:08:42Z","receivedAt":"2021-02-24T20:10:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Jeff Hostetler via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Jeff Hostetler <jeffhost@microsoft.com>\n>\n> Use FLEX_ALLOC_STR() to allocate the `struct untracked_cache_dir`\n> for the root directory.  Get rid of unsafe code that might fail to\n> initialize the `name` field (if FLEX_ARRAY is not 1).  This will\n> make it clear that we intend to have a structure with an empty\n> string following it.\n>\n> A problem was observed on Windows where the length of the memset() was\n> too short, so the first byte of the name field was not zeroed.  This\n> resulted in the name field having garbage from a previous use of that\n> area of memory.\n>\n> The record for the root directory was then written to the untracked-cache\n> extension in the index.  This garbage would then be visible to future\n> commands when they reloaded the untracked-cache extension.\n>\n> Since the directory record for the root directory had garbage in the\n> `name` field, the `t/helper/test-tool dump-untracked-cache` tool\n> printed this garbage as the path prefix (rather than '/') for each\n> directory in the untracked cache as it recursed.\n>\n> Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>\n> ---\n>     dir: fix malloc of root untracked_cache_dir\n\nNicely spotted.\n\nThe problematic code was introduced in 2015, a year before these\nFLEX_ALLOC_*() helpers were introduced.  The result is of course\ncorrect and much easier to read, as the necessary \"ask for a region\nof calloc'ed memory with an additional byte for terminating NUL\nbeyond strlen()\" is hidden in the helper.\n\nWill queue; thanks.\n\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-884%2Fjeffhostetler%2Funtracked-cache-corruption-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-884/jeffhostetler/untracked-cache-corruption-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/884\n>\n>  dir.c | 7 ++-----\n>  1 file changed, 2 insertions(+), 5 deletions(-)\n>\n> diff --git a/dir.c b/dir.c\n> index d153a63bbd14..fd8aa7c40faa 100644\n> --- a/dir.c\n> +++ b/dir.c\n> @@ -2730,11 +2730,8 @@ static struct untracked_cache_dir *validate_untracked_cache(struct dir_struct *d\n>  \t\treturn NULL;\n>  \t}\n>  \n> -\tif (!dir->untracked->root) {\n> -\t\tconst int len = sizeof(*dir->untracked->root);\n> -\t\tdir->untracked->root = xmalloc(len);\n> -\t\tmemset(dir->untracked->root, 0, len);\n> -\t}\n> +\tif (!dir->untracked->root)\n> +\t\tFLEX_ALLOC_STR(dir->untracked->root, name, \"\");\n>  \n>  \t/* Validate $GIT_DIR/info/exclude and core.excludesfile */\n>  \troot = dir->untracked->root;\n>\n> base-commit: 966e671106b2fd38301e7c344c754fd118d0bb07\n"},{"id":"417762","messageId":"YDa/hT8g8FvmpXYT@coredump.intra.peff.net","threadId":"55204","inReplyTo":"xmqqk0qxz0s5.fsf@gitster.g","subject":"Re: [PATCH] dir: fix malloc of root untracked_cache_dir","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-02-24T21:05:09Z","receivedAt":"2021-02-24T21:05:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 24, 2021 at 12:08:42PM -0800, Junio C Hamano wrote:\n\n> > Use FLEX_ALLOC_STR() to allocate the `struct untracked_cache_dir`\n> > for the root directory.  Get rid of unsafe code that might fail to\n> > initialize the `name` field (if FLEX_ARRAY is not 1).  This will\n> > make it clear that we intend to have a structure with an empty\n> > string following it.\n> [...]\n> The problematic code was introduced in 2015, a year before these\n> FLEX_ALLOC_*() helpers were introduced.  The result is of course\n> correct and much easier to read, as the necessary \"ask for a region\n> of calloc'ed memory with an additional byte for terminating NUL\n> beyond strlen()\" is hidden in the helper.\n\nWhen I added the FLEX_ALLOC_* helpers, I audited for existing callers to\nconvert. But I did so by looking for places where we were doing manual\nsize computations. The bug here was that it was not doing any\ncomputation at all (when it need to be doing \"+1\"). So that's my guess\nwhy it got overlooked (which is not super important, but may give a hint\nabout how to look for similar bugs).\n\n-Peff\n"},{"id":"417763","messageId":"581ece66-6a93-eda6-67bf-3bb49f553dfc@jeffhostetler.com","threadId":"55204","inReplyTo":"YDa/hT8g8FvmpXYT@coredump.intra.peff.net","subject":"Re: [PATCH] dir: fix malloc of root untracked_cache_dir","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2021-02-24T21:15:11Z","receivedAt":"2021-02-24T21:16:13Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 2/24/21 4:05 PM, Jeff King wrote:\n> On Wed, Feb 24, 2021 at 12:08:42PM -0800, Junio C Hamano wrote:\n> \n>>> Use FLEX_ALLOC_STR() to allocate the `struct untracked_cache_dir`\n>>> for the root directory.  Get rid of unsafe code that might fail to\n>>> initialize the `name` field (if FLEX_ARRAY is not 1).  This will\n>>> make it clear that we intend to have a structure with an empty\n>>> string following it.\n>> [...]\n>> The problematic code was introduced in 2015, a year before these\n>> FLEX_ALLOC_*() helpers were introduced.  The result is of course\n>> correct and much easier to read, as the necessary \"ask for a region\n>> of calloc'ed memory with an additional byte for terminating NUL\n>> beyond strlen()\" is hidden in the helper.\n> \n> When I added the FLEX_ALLOC_* helpers, I audited for existing callers to\n> convert. But I did so by looking for places where we were doing manual\n> size computations. The bug here was that it was not doing any\n> computation at all (when it need to be doing \"+1\"). So that's my guess\n> why it got overlooked (which is not super important, but may give a hint\n> about how to look for similar bugs).\n\n\nThere's another call to xmalloc() at [1] that does the st_add3()\nthing that I didn't change.  It was correctly computing the size\nso I didn't want to change it for no reason.  That and the 2 memcpy()s\nmade it feel like we should have a different FLEX_ macro for this\ncase -- more of a FLEX_DUP... or something.  But I didn't want to\ndistract from my bug fix here.\n\n[1] https://github.com/git/git/blob/master/dir.c#L3290\n\nJeff\n"},{"id":"417767","messageId":"YDbmgXVKVsog6K+r@coredump.intra.peff.net","threadId":"55204","inReplyTo":"581ece66-6a93-eda6-67bf-3bb49f553dfc@jeffhostetler.com","subject":"Re: [PATCH] dir: fix malloc of root untracked_cache_dir","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-02-24T23:51:29Z","receivedAt":"2021-02-24T23:52:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 24, 2021 at 04:15:11PM -0500, Jeff Hostetler wrote:\n\n> > When I added the FLEX_ALLOC_* helpers, I audited for existing callers to\n> > convert. But I did so by looking for places where we were doing manual\n> > size computations. The bug here was that it was not doing any\n> > computation at all (when it need to be doing \"+1\"). So that's my guess\n> > why it got overlooked (which is not super important, but may give a hint\n> > about how to look for similar bugs).\n> \n> There's another call to xmalloc() at [1] that does the st_add3()\n> thing that I didn't change.  It was correctly computing the size\n> so I didn't want to change it for no reason.  That and the 2 memcpy()s\n> made it feel like we should have a different FLEX_ macro for this\n> case -- more of a FLEX_DUP... or something.  But I didn't want to\n> distract from my bug fix here.\n\nThanks for pointing it out. Even if we change it, I agree it's better to\nkeep it out of your existing bugfix patch.\n\nBut I do think that spot gets weird. We are copying all of the elements\nof the struct _except_ the name field, which comes from somewhere else.\nSo it's tempting to do:\n\ndiff --git a/dir.c b/dir.c\nindex d153a63bbd..ab16db3f76 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -3287,9 +3287,9 @@ static int read_one_dir(struct untracked_cache_dir **untracked_,\n \tif (!eos || eos == end)\n \t\treturn -1;\n \n-\t*untracked_ = untracked = xmalloc(st_add3(sizeof(*untracked), eos - data, 1));\n+\tFLEX_ALLOC_MEM(untracked, name, data, eos - data);\n \tmemcpy(untracked, &ud, sizeof(ud));\n-\tmemcpy(untracked->name, data, eos - data + 1);\n+\t*untracked_ = untracked;\n \tdata = eos + 1;\n \n \tfor (i = 0; i < untracked->untracked_nr; i++) {\n\nBut that's wrong! The remaining memcpy using sizeof(ud) might or might\nnot touch the first byte of the name field, depending on struct padding\nand the value of FLEX_ARRAY. And if it does, then we'd be overwriting\nthe early bytes of that field with whatever was in \"ud\" (which I guess\nis uninitialized cruft, because that is a stack variable).\n\nSo it would have to be:\n\n  memcpy(untracked, &ud, offsetof(struct untracked_cache_dir, name));\n\nQuite subtle. Even with a comment, I think I prefer the existing code.\nIf we had a macro as you suggest for \"allocate a flex struct, dup most of\nthe contents from this other struct, and then copy in the flex bytes\nfrom this other spot\", that would make it more readable. But I'm not\nsure we'd find more than one spot to use such a macro. :)\n\n-Peff\n"}]}