{"thread":{"id":"61643","subject":"[PATCH] attr: fix msan issue in read_attr_from_index","startedAt":"2024-06-17T20:00:28Z","lastAt":"2024-06-18T23:39:36Z","messageCount":6,"participants":["Kyle Lippincott via GitGitGadget","Junio C Hamano","Kyle Lippincott","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"497256","messageId":"pull.1747.git.1718654424683.gitgitgadget@gmail.com","threadId":"61643","inReplyTo":null,"subject":"[PATCH] attr: fix msan issue in read_attr_from_index","fromName":"Kyle Lippincott via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-06-17T20:00:24Z","receivedAt":"2024-06-17T20:00:28Z","isPatch":true,"sender":{"key":"spectral@google.com","avatar":"https://avatars.githubusercontent.com/u/6371650?v=4"},"body":"From: Kyle Lippincott \n\nMemory sanitizer (msan) is detecting a use of an uninitialized variable\n(`size`) in `read_attr_from_index`:\n\n ==2268==WARNING: MemorySanitizer: use-of-uninitialized-value\n #0 0x5651f3416504 in read_attr_from_index git/attr.c:868:11\n #1 0x5651f3415530 in read_attr git/attr.c\n #2 0x5651f3413d74 in bootstrap_attr_stack git/attr.c:968:6\n #3 0x5651f3413d74 in prepare_attr_stack git/attr.c:1004:2\n #4 0x5651f3413d74 in collect_some_attrs git/attr.c:1199:2\n #5 0x5651f3413144 in git_check_attr git/attr.c:1345:2\n #6 0x5651f34728da in convert_attrs git/convert.c:1320:2\n #7 0x5651f3473425 in would_convert_to_git_filter_fd git/convert.c:1373:2\n #8 0x5651f357a35e in index_fd git/object-file.c:2630:34\n #9 0x5651f357aa15 in index_path git/object-file.c:2657:7\n #10 0x5651f35db9d9 in add_to_index git/read-cache.c:766:7\n #11 0x5651f35dc170 in add_file_to_index git/read-cache.c:799:9\n #12 0x5651f321f9b2 in add_files git/builtin/add.c:346:7\n #13 0x5651f321f9b2 in cmd_add git/builtin/add.c:565:18\n #14 0x5651f321d327 in run_builtin git/git.c:474:11\n #15 0x5651f321bc9e in handle_builtin git/git.c:729:3\n #16 0x5651f321a792 in run_argv git/git.c:793:4\n #17 0x5651f321a792 in cmd_main git/git.c:928:19\n #18 0x5651f33dde1f in main git/common-main.c:62:11\n\nThe issue exists because `size` is an output parameter from\n`read_blob_data_from_index`, but it's only modified if\n`read_blob_data_from_index` returns non-NULL. The read of `size` when\ncalling `read_attr_from_buf` unconditionally may read from an\nuninitialized value. `read_attr_from_buf` checks that `buf` is non-NULL\nbefore reading from `size`, but by then it's already too late: the\nuninitialized read will have happened already. Furthermore, there's no\nguarantee that the compiler won't reorder things so that it checks\n`size` before checking `!buf`.\n\nMake the call to `read_attr_from_buf` conditional on `buf` being\nnon-NULL, ensuring that `size` is not read if it's never set.\n\nSigned-off-by: Kyle Lippincott \n---\n attr: fix msan issue in read_attr_from_index\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1747%2Fspectral54%2Fmsan-attr-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1747/spectral54/msan-attr-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1747\n\n attr.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/attr.c b/attr.c\nindex 300f994ba6e..a2e0775f7e5 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -865,7 +865,8 @@ static struct attr_stack *read_attr_from_index(struct index_state *istate,\n \t\tstack = read_attr_from_blob(istate, &istate->cache[sparse_dir_pos]->oid, relative_path, flags);\n \t} else {\n \t\tbuf = read_blob_data_from_index(istate, path, &size);\n-\t\tstack = read_attr_from_buf(buf, size, path, flags);\n+\t\tif (buf)\n+\t\t\tstack = read_attr_from_buf(buf, size, path, flags);\n \t}\n \treturn stack;\n }\n\nbase-commit: 8d94cfb54504f2ec9edc7ca3eb5c29a3dd3675ae\n-- \ngitgitgadget\n"},{"id":"497257","messageId":"xmqqmsnj5o3r.fsf@gitster.g","threadId":"61643","inReplyTo":"pull.1747.git.1718654424683.gitgitgadget@gmail.com","subject":"Re: [PATCH] attr: fix msan issue in read_attr_from_index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-17T20:08:24Z","receivedAt":"2024-06-17T20:08:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Kyle Lippincott via GitGitGadget\" writes:\n\n> Make the call to `read_attr_from_buf` conditional on `buf` being\n> non-NULL, ensuring that `size` is not read if it's never set.\n\nMakes good sense.\n\n\n> Signed-off-by: Kyle Lippincott \n> ---\n> attr: fix msan issue in read_attr_from_index\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1747%2Fspectral54%2Fmsan-attr-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1747/spectral54/msan-attr-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1747\n>\n> attr.c | 3 ++-\n> 1 file changed, 2 insertions(+), 1 deletion(-)\n>\n> diff --git a/attr.c b/attr.c\n> index 300f994ba6e..a2e0775f7e5 100644\n> --- a/attr.c\n> +++ b/attr.c\n> @@ -865,7 +865,8 @@ static struct attr_stack *read_attr_from_index(struct index_state *istate,\n> \t\tstack = read_attr_from_blob(istate, &istate->cache[sparse_dir_pos]->oid, relative_path, flags);\n> \t} else {\n> \t\tbuf = read_blob_data_from_index(istate, path, &size);\n> -\t\tstack = read_attr_from_buf(buf, size, path, flags);\n> +\t\tif (buf)\n> +\t\t\tstack = read_attr_from_buf(buf, size, path, flags);\n> \t}\n> \treturn stack;\n> }\n\nNot directly related to the issue this patch addresses, but I notice\nthat both buf and size variables have unnecesarily wide scope. As a\nclean-up we may want to move their declaration into this \"} else {\"\nblock. But that is totally outside the scope (no pun intended) of\nthis patch.\n\nWill queue.\nThanks. \n"},{"id":"497258","messageId":"xmqqcyof5n2t.fsf@gitster.g","threadId":"61643","inReplyTo":"pull.1747.git.1718654424683.gitgitgadget@gmail.com","subject":"Re: [PATCH] attr: fix msan issue in read_attr_from_index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-17T20:30:34Z","receivedAt":"2024-06-17T20:30:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Kyle Lippincott via GitGitGadget\" writes:\n\n> The issue exists because `size` is an output parameter from\n> `read_blob_data_from_index`, but it's only modified if\n> `read_blob_data_from_index` returns non-NULL.\n\nCorrect.\n\n> The read of `size` when\n> calling `read_attr_from_buf` unconditionally may read from an\n> uninitialized value. `read_attr_from_buf` checks that `buf` is non-NULL\n> before reading from `size`, but by then it's already too late: the\n> uninitialized read will have happened already.\n\nYes, but it is dubious that reading an uninitialized value that we\nknow will not be used is a problem, so I am inclined to say that\nMSAN is giving a false positive here.\n\n> Furthermore, there's no\n> guarantee that the compiler won't reorder things so that it checks\n> `size` before checking `!buf`.\n\nThis I do not understand. Are you talking about buf vs length here\nin the callee?\n\n static struct attr_stack *read_attr_from_buf(char *buf, size_t length,\n const char *path, unsigned flags)\n {\n struct attr_stack *res;\n char *sp;\n int lineno = 0;\n\n if (!buf)\n return NULL;\n if (length >= ATTR_MAX_FILE_SIZE) {\n warning(_(\"ignoring overly large gitattributes blob '%s'\"), path);\n free(buf);\n return NULL;\n }\n\nAt the machine level, a prefetch may happen from both buf and\nlength, but the program ought to behave the same way as the code is\nexecuted serially as written. If the compiler allows the outside\nworld to observe that resulting code checks length even when buf is\nNULL, such a compiler is broken. So I do not think that is what you\nare referring to, but then I do not know what problem you are\ndescribing.\n\nHaving said all that ...\n\n> Make the call to `read_attr_from_buf` conditional on `buf` being\n> non-NULL, ensuring that `size` is not read if it's never set.\n\n... this makes the logic at the caller crystal clear, so even if\nthere are suboptimal checker that bothers us with false positives,\nthe change itself justifies itself, I would say.\n\n> \t} else {\n> \t\tbuf = read_blob_data_from_index(istate, path, &size);\n> -\t\tstack = read_attr_from_buf(buf, size, path, flags);\n> +\t\tif (buf)\n> +\t\t\tstack = read_attr_from_buf(buf, size, path, flags);\n> \t}\n> \treturn stack;\n\nThanks.\n"},{"id":"497261","messageId":"xmqqwmmn46ge.fsf@gitster.g","threadId":"61643","inReplyTo":"xmqqcyof5n2t.fsf@gitster.g","subject":"Re: [PATCH] attr: fix msan issue in read_attr_from_index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-17T21:14:57Z","receivedAt":"2024-06-17T21:15:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Having said all that ...\n>\n>> Make the call to `read_attr_from_buf` conditional on `buf` being\n>> non-NULL, ensuring that `size` is not read if it's never set.\n>\n> ... this makes the logic at the caller crystal clear, so even if\n> there are suboptimal checker that bothers us with false positives,\n> the change itself justifies itself, I would say.\n\nWell, \"even if there were *no* MSAN or other issues wrt usage of size\"\nwas what I wanted to say. Sorry for a noise.\n\n>> \t} else {\n>> \t\tbuf = read_blob_data_from_index(istate, path, &size);\n>> -\t\tstack = read_attr_from_buf(buf, size, path, flags);\n>> +\t\tif (buf)\n>> +\t\t\tstack = read_attr_from_buf(buf, size, path, flags);\n>> \t}\n>> \treturn stack;\n>\n> Thanks.\n"},{"id":"497263","messageId":"CAO_smVg1GuyreBw7Dw0RjLPjD1KhH-NmFMkkC-D4hB7kfPqjCQ@mail.gmail.com","threadId":"61643","inReplyTo":"xmqqcyof5n2t.fsf@gitster.g","subject":"Re: [PATCH] attr: fix msan issue in read_attr_from_index","fromName":"Kyle Lippincott","fromEmail":"spectral@google.com","sentAt":"2024-06-17T21:17:28Z","receivedAt":"2024-06-17T21:17:56Z","isPatch":true,"sender":{"key":"spectral@google.com","avatar":"https://avatars.githubusercontent.com/u/6371650?v=4"},"body":"On Mon, Jun 17, 2024 at 1:30 PM Junio C Hamano wrote:\n>\n> \"Kyle Lippincott via GitGitGadget\" writes:\n>\n> > The issue exists because `size` is an output parameter from\n> > `read_blob_data_from_index`, but it's only modified if\n> > `read_blob_data_from_index` returns non-NULL.\n>\n> Correct.\n>\n> > The read of `size` when\n> > calling `read_attr_from_buf` unconditionally may read from an\n> > uninitialized value. `read_attr_from_buf` checks that `buf` is non-NULL\n> > before reading from `size`, but by then it's already too late: the\n> > uninitialized read will have happened already.\n>\n> Yes, but it is dubious that reading an uninitialized value that we\n> know will not be used is a problem, so I am inclined to say that\n> MSAN is giving a false positive here.\n>\n> > Furthermore, there's no\n> > guarantee that the compiler won't reorder things so that it checks\n> > `size` before checking `!buf`.\n>\n> This I do not understand. Are you talking about buf vs length here\n> in the callee?\n>\n> static struct attr_stack *read_attr_from_buf(char *buf, size_t length,\n> const char *path, unsigned flags)\n> {\n> struct attr_stack *res;\n> char *sp;\n> int lineno = 0;\n>\n> if (!buf)\n> return NULL;\n> if (length >= ATTR_MAX_FILE_SIZE) {\n> warning(_(\"ignoring overly large gitattributes blob '%s'\"), path);\n> free(buf);\n> return NULL;\n> }\n>\n> At the machine level, a prefetch may happen from both buf and\n> length, but the program ought to behave the same way as the code is\n> executed serially as written. If the compiler allows the outside\n> world to observe that resulting code checks length even when buf is\n> NULL, such a compiler is broken. So I do not think that is what you\n> are referring to, but then I do not know what problem you are\n> describing.\n\nOnce there's an uninitialized read, we're in undefined behavior\nterritory. There's no requirement that the compiler keep the code\noperating the way we'd logically expect once there's undefined\nbehavior, especially with the optimizer involved.\n\nI think that you're right though: when I wrote this I'd convinced\nmyself that this wasn't guaranteed to work, but taking a look now I\ncan't think of a way for this to go wrong, because afaik size_t is\nsuch a simple type, conceptually. When compiling `read_attr_from_buf`,\nit can't actually assume any relationship between `buf` and `length`.\nMaybe I was thinking that with link-time optimizations (whole-program\noptimizations) it can go wrong? I'm not remembering what I was\nthinking about when I wrote that, sorry.\n\n>\n> Having said all that ...\n>\n> > Make the call to `read_attr_from_buf` conditional on `buf` being\n> > non-NULL, ensuring that `size` is not read if it's never set.\n>\n> ... this makes the logic at the caller crystal clear, so even if\n> there are suboptimal checker that bothers us with false positives,\n> the change itself justifies itself, I would say.\n>\n> > } else {\n> > buf = read_blob_data_from_index(istate, path, &size);\n> > - stack = read_attr_from_buf(buf, size, path, flags);\n> > + if (buf)\n> > + stack = read_attr_from_buf(buf, size, path, flags);\n> > }\n> > return stack;\n>\n> Thanks.\n"},{"id":"497305","messageId":"20240618233935.GB188880@coredump.intra.peff.net","threadId":"61643","inReplyTo":"pull.1747.git.1718654424683.gitgitgadget@gmail.com","subject":"Re: [PATCH] attr: fix msan issue in read_attr_from_index","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-06-18T23:39:35Z","receivedAt":"2024-06-18T23:39:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 17, 2024 at 08:00:24PM +0000, Kyle Lippincott via GitGitGadget wrote:\n\n> The issue exists because `size` is an output parameter from\n> `read_blob_data_from_index`, but it's only modified if\n> `read_blob_data_from_index` returns non-NULL. The read of `size` when\n> calling `read_attr_from_buf` unconditionally may read from an\n> uninitialized value. `read_attr_from_buf` checks that `buf` is non-NULL\n> before reading from `size`, but by then it's already too late: the\n> uninitialized read will have happened already. Furthermore, there's no\n> guarantee that the compiler won't reorder things so that it checks\n> `size` before checking `!buf`.\n> \n> Make the call to `read_attr_from_buf` conditional on `buf` being\n> non-NULL, ensuring that `size` is not read if it's never set.\n\nYeah, this is the same one I mentioned when bisecting in the other\nthread[1]. But I got confused by applying my fixup patch at various\npoints in the bisection, and thought it _used_ to be a problem, and\nisn't anymore. It's the other way around. It was introduced by\nc793f9cb08, which moved the NULL check into the helper.\n\nThat patch is from Taylor, but I'm listed as a co-author, and I'm almost\ncertain moving that NULL check was my suggestion. So it's doubly bad\nthat I didn't figure out what was going on earlier. ;)\n\nPossible UB aside, I doubt this can trigger bad behavior in practice.\nBut I also wouldn't call it a false positive in MSan. We really are\nreading the uninitialized value and passing it. Your fix here is the\nobviously correct thing to do.\n\n-Peff\n\n[1] https://lore.kernel.org/git/20240608081855.GA2390433@coredump.intra.peff.net/\n"}]}