{"thread":{"id":"53980","subject":"[PATCH 0/3] some compiler/asan/ubsan fixes","startedAt":"2020-08-04T07:41:48Z","lastAt":"2020-08-05T19:23:52Z","messageCount":10,"participants":["Jeff King","Derrick Stolee","Junio C Hamano","Taylor Blau"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"402799","messageId":"20200804074146.GA190027@coredump.intra.peff.net","threadId":"53980","inReplyTo":null,"subject":"[PATCH 0/3] some compiler/asan/ubsan fixes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-04T07:41:46Z","receivedAt":"2020-08-04T07:41:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Here are fixes for a few problems I came across today while running\n\"make SANITIZE=address,undefined test\" on the tip of master.\n\n [1/3]: config: work around gcc-10 -Wstringop-overflow warning\n [2/3]: revision: avoid out-of-bounds read/write on empty pathspec\n [3/3]: revision: avoid leak when preparing bloom filter for \"/\"\n\n config.c | 2 +-\n revision.c | 8 +++-----\n 2 files changed, 4 insertions(+), 6 deletions(-)\n\n-Peff\n"},{"id":"402800","messageId":"20200804074353.GA284046@coredump.intra.peff.net","threadId":"53980","inReplyTo":"20200804074146.GA190027@coredump.intra.peff.net","subject":"[PATCH 1/3] config: work around gcc-10 -Wstringop-overflow warning","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-04T07:43:53Z","receivedAt":"2020-08-04T07:43:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Compiling with gcc-10, -O2, and -fsanitize=undefined results in a\ncompiler warning:\n\n config.c: In function ‘git_config_copy_or_rename_section_in_file’:\n config.c:3170:17: error: writing 1 byte into a region of size 0 [-Werror=stringop-overflow=]\n 3170 | output[0] = '\\t';\n | ~~~~~~~~~~^~~~~~\n config.c:3076:7: note: at offset -1 to object ‘buf’ with size 1024 declared here\n 3076 | char buf[1024];\n | ^~~\n\nThis is a false positive. The interesting lines of code are:\n\n int i;\n char *output = buf;\n ...\n for (i = 0; buf[i] && isspace(buf[i]); i++)\n ; /* do nothing */\n ...\n int offset;\n offset = section_name_match(&buf[i], old_name);\n if (offset > 0) {\n ...\n output += offset + i;\n if (strlen(output) > 0) {\n\t\t /*\n\t\t * More content means there's\n\t\t * a declaration to put on the\n\t\t * next line; indent with a\n\t\t * tab\n\t\t */\n\t\t output -= 1;\n\t\t output[0] = '\\t';\n\t }\n }\n\nSo we do assign output to buf initially. Later we increment it based on\n\"offset\" and \"i\" and then subtract \"1\" from it. That latter step is what\nthe compiler is complaining about; it could lead to going off the left\nside of the array if \"output == buf\" at the moment of the subtraction.\nFor that to be the case, then \"offset + i\" would have to be 0. But that\ncan't happen:\n\n - we know that \"offset\" is at least 1, since we're in a conditional\n block that checks that\n\n - we know that \"i\" is not negative, since it started at 0 and only\n incremented over whitespace\n\nSo the sum must be at least 1, and therefore it's OK to subtract one\nfrom \"output\".\n\nBut that's not quite the whole story. Since \"i\" is an int, it could in\ntheory be possible to overflow to negative (when counting whitespace on\na very large string). But we know that's impossible because we're\ncounting the 1024-byte buffer we just fed to fgets(), so it can never be\nlarger than that.\n\nSwitching the type of \"i\" to \"unsigned\" makes the warning go away, so\nlet's do that.\n\nArguably size_t is an even better type (for this and for the other\nlength fields), but switching to it produces a similar but distinct\nwarning:\n\n config.c: In function ‘git_config_copy_or_rename_section_in_file’:\n config.c:3170:13: error: array subscript -1 is outside array bounds of ‘char[1024]’ [-Werror=array-bounds]\n 3170 | output[0] = '\\t';\n | ~~~~~~^~~\n config.c:3076:7: note: while referencing ‘buf’\n 3076 | char buf[1024];\n | ^~~\n\nIf we were to ever switch off of fgets() to strbuf_getline() or similar,\nwe'd probably need to use size_t to avoid other overflow problems. But\nfor now we know we're safe because of the small fixed size of our\nbuffer.\n\nSigned-off-by: Jeff King \n---\n config.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/config.c b/config.c\nindex 8db9c77098..2b79fe76ad 100644\n--- a/config.c\n+++ b/config.c\n@@ -3115,7 +3115,7 @@ static int git_config_copy_or_rename_section_in_file(const char *config_filename\n \t}\n \n \twhile (fgets(buf, sizeof(buf), config_file)) {\n-\t\tint i;\n+\t\tunsigned i;\n \t\tint length;\n \t\tint is_section = 0;\n \t\tchar *output = buf;\n-- \n2.28.0.536.ga4d8134877\n\n"},{"id":"402801","messageId":"20200804074652.GB284046@coredump.intra.peff.net","threadId":"53980","inReplyTo":"20200804074146.GA190027@coredump.intra.peff.net","subject":"[PATCH 2/3] revision: avoid out-of-bounds read/write on empty pathspec","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-04T07:46:52Z","receivedAt":"2020-08-04T07:46:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Running t4216 with ASan results in it complaining of an out-of-bounds\nread in prepare_to_use_bloom_filter(). The issue is this code to strip a\ntrailing slash:\n\n last_index = pi->len - 1;\n if (pi->match[last_index] == '/') {\n\nbecause we have no guarantee that pi->len isn't zero. This can happen if\nthe pathspec is \".\", as we translate that to an empty string. And if\nthat read of random memory does trigger the conditional, we'd then do an\nout-of-bounds write:\n\n path_alloc = xstrdup(pi->match);\n path_alloc[last_index] = '\\0';\n\nLet's make sure to check the length before subtracting. Note that for an\nempty pathspec, we'd end up bailing from the function a few lines later,\nwhich makes it tempting to just:\n\n if (!pi->len)\n return;\n\nearly here. But our code here is stripping a trailing slash, and we need\nto check for emptiness after stripping that slash, too. So we'd have two\nblocks, which would require repeating some cleanup code.\n\nInstead, just skip the trailing-slash for an empty string. Setting\nlast_index at all in the case is awkward since it will have a nonsense\nvalue (and it uses an \"int\", which is a too-small type for a string\nanyway). So while we're here, let's:\n\n - drop last_index entirely; it's only used in two spots right next to\n each other and writing out \"pi->len - 1\" in both is actually easier\n to follow\n\n - use xmemdupz() to duplicate the string. This is slightly more\n efficient, but more importantly makes the intent more clear by\n allocating the correct-sized substring in the first place. It also\n eliminates any question of whether path_alloc is as long as\n pi->match (which it would not be if pi->match has any embedded NULs,\n though in practice this is probably impossible).\n\nSigned-off-by: Jeff King \n---\nAnother variant is to actually stop assigning revs->bloom_filter_settings\nso early, so that we don't have to clean it up. And then once we're sure\nwe're going to use it and have passed all of our early-return checks,\nthen assign it. But that conflicts with the get_bloom_filter_settings()\npatch in:\n\n https://lore.kernel.org/git/08479793c1274d5ee0f063578bb0f4d93c910fa9.1596480582.git.me@ttaylorr.com/\n\nso I didn't go that way.\n\n revision.c | 7 ++-----\n 1 file changed, 2 insertions(+), 5 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex 6de29cdf7a..5ed86e4524 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -669,7 +669,6 @@ static void prepare_to_use_bloom_filter(struct rev_info *revs)\n \tstruct pathspec_item *pi;\n \tchar *path_alloc = NULL;\n \tconst char *path, *p;\n-\tint last_index;\n \tsize_t len;\n \tint path_component_nr = 1;\n \n@@ -692,12 +691,10 @@ static void prepare_to_use_bloom_filter(struct rev_info *revs)\n \t\treturn;\n \n \tpi = &revs->pruning.pathspec.items[0];\n-\tlast_index = pi->len - 1;\n \n \t/* remove single trailing slash from path, if needed */\n-\tif (pi->match[last_index] == '/') {\n-\t\tpath_alloc = xstrdup(pi->match);\n-\t\tpath_alloc[last_index] = '\\0';\n+\tif (pi->len > 0 && pi->match[pi->len - 1] == '/') {\n+\t\tpath_alloc = xmemdupz(pi->match, pi->len - 1);\n \t\tpath = path_alloc;\n \t} else\n \t\tpath = pi->match;\n-- \n2.28.0.536.ga4d8134877\n\n"},{"id":"402802","messageId":"20200804075017.GC284046@coredump.intra.peff.net","threadId":"53980","inReplyTo":"20200804074146.GA190027@coredump.intra.peff.net","subject":"[PATCH 3/3] revision: avoid leak when preparing bloom filter for \"/\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-04T07:50:17Z","receivedAt":"2020-08-04T07:50:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If we're given an empty pathspec, we refuse to set up bloom filters, as\ndescribed in f3c2a36810 (revision: empty pathspecs should not use Bloom\nfilters, 2020-07-01).\n\nBut before the empty string check, we drop any trailing slash by\nallocating a new string without it. So a pathspec consisting only of \"/\"\nwill allocate that string, but then still cause us to bail, leaking the\nnew string. Let's make sure to free it.\n\nSigned-off-by: Jeff King \n---\nJust noticed while reading the function to fix the previous patch.\n\nI'm not even sure if it's possible to get here with a pathspec of \"/\",\nsince we'd probably give a \"/ is outside repository\" error before then.\n\nSo maybe this case doesn't even matter. If it doesn't, then it might\nsimplify the function a bit to do the empty-pathspec check before\nhandling trailing slashes. But handling it does help make it more clear\nthis function is doing the right thing no matter what input it is given,\nso that's what I went with here.\n\n revision.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/revision.c b/revision.c\nindex 5ed86e4524..b80868556b 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -702,6 +702,7 @@ static void prepare_to_use_bloom_filter(struct rev_info *revs)\n \tlen = strlen(path);\n \tif (!len) {\n \t\trevs->bloom_filter_settings = NULL;\n+\t\tfree(path_alloc);\n \t\treturn;\n \t}\n \n-- \n2.28.0.536.ga4d8134877\n"},{"id":"402810","messageId":"f05b2a13-d8c6-62c4-4efd-90cba216b263@gmail.com","threadId":"53980","inReplyTo":"20200804074652.GB284046@coredump.intra.peff.net","subject":"Re: [PATCH 2/3] revision: avoid out-of-bounds read/write on empty pathspec","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-08-04T13:08:32Z","receivedAt":"2020-08-04T13:08:37Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/4/2020 3:46 AM, Jeff King wrote:\n> Running t4216 with ASan results in it complaining of an out-of-bounds\n> read in prepare_to_use_bloom_filter(). The issue is this code to strip a\n> trailing slash:\n> \n> last_index = pi->len - 1;\n> if (pi->match[last_index] == '/') {\n> \n> because we have no guarantee that pi->len isn't zero. This can happen if\n> the pathspec is \".\", as we translate that to an empty string. And if\n> that read of random memory does trigger the conditional, we'd then do an\n> out-of-bounds write:\n> \n> path_alloc = xstrdup(pi->match);\n> path_alloc[last_index] = '\\0';\n> \n> Let's make sure to check the length before subtracting. Note that for an\n> empty pathspec, we'd end up bailing from the function a few lines later,\n> which makes it tempting to just:\n> \n> if (!pi->len)\n> return;\n> \n> early here. But our code here is stripping a trailing slash, and we need\n> to check for emptiness after stripping that slash, too. So we'd have two\n> blocks, which would require repeating some cleanup code.\n> \n> Instead, just skip the trailing-slash for an empty string. Setting\n> last_index at all in the case is awkward since it will have a nonsense\n> value (and it uses an \"int\", which is a too-small type for a string\n> anyway). So while we're here, let's:\n> \n> - drop last_index entirely; it's only used in two spots right next to\n> each other and writing out \"pi->len - 1\" in both is actually easier\n> to follow\n> \n> - use xmemdupz() to duplicate the string. This is slightly more\n> efficient, but more importantly makes the intent more clear by\n> allocating the correct-sized substring in the first place. It also\n> eliminates any question of whether path_alloc is as long as\n> pi->match (which it would not be if pi->match has any embedded NULs,\n> though in practice this is probably impossible).\n> \n> Signed-off-by: Jeff King \n> ---\n> Another variant is to actually stop assigning revs->bloom_filter_settings\n> so early, so that we don't have to clean it up. And then once we're sure\n> we're going to use it and have passed all of our early-return checks,\n> then assign it. But that conflicts with the get_bloom_filter_settings()\n> patch in:\n> \n> https://lore.kernel.org/git/08479793c1274d5ee0f063578bb0f4d93c910fa9.1596480582.git.me@ttaylorr.com/\n> \n> so I didn't go that way.\n> \n> revision.c | 7 ++-----\n> 1 file changed, 2 insertions(+), 5 deletions(-)\n> \n> diff --git a/revision.c b/revision.c\n> index 6de29cdf7a..5ed86e4524 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -669,7 +669,6 @@ static void prepare_to_use_bloom_filter(struct rev_info *revs)\n> \tstruct pathspec_item *pi;\n> \tchar *path_alloc = NULL;\n> \tconst char *path, *p;\n> -\tint last_index;\n> \tsize_t len;\n> \tint path_component_nr = 1;\n> \n> @@ -692,12 +691,10 @@ static void prepare_to_use_bloom_filter(struct rev_info *revs)\n> \t\treturn;\n> \n> \tpi = &revs->pruning.pathspec.items[0];\n> -\tlast_index = pi->len - 1;\n> \n> \t/* remove single trailing slash from path, if needed */\n> -\tif (pi->match[last_index] == '/') {\n> -\t\tpath_alloc = xstrdup(pi->match);\n> -\t\tpath_alloc[last_index] = '\\0';\n> +\tif (pi->len > 0 && pi->match[pi->len - 1] == '/') {\n> +\t\tpath_alloc = xmemdupz(pi->match, pi->len - 1);\n> \t\tpath = path_alloc;\n> \t} else\n> \t\tpath = pi->match;\n\nThis change has the advantage of looking simpler than the previous\nimplementation. Thanks.\n\n-Stolee\n"},{"id":"402811","messageId":"5159cfbb-55a4-8010-66a3-f6b4e4b05d7e@gmail.com","threadId":"53980","inReplyTo":"20200804075017.GC284046@coredump.intra.peff.net","subject":"Re: [PATCH 3/3] revision: avoid leak when preparing bloom filter for \"/\"","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-08-04T13:09:21Z","receivedAt":"2020-08-04T13:09:27Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/4/2020 3:50 AM, Jeff King wrote:\n> If we're given an empty pathspec, we refuse to set up bloom filters, as\n> described in f3c2a36810 (revision: empty pathspecs should not use Bloom\n> filters, 2020-07-01).\n> \n> But before the empty string check, we drop any trailing slash by\n> allocating a new string without it. So a pathspec consisting only of \"/\"\n> will allocate that string, but then still cause us to bail, leaking the\n> new string. Let's make sure to free it.\n> \n> Signed-off-by: Jeff King \n> ---\n> Just noticed while reading the function to fix the previous patch.\n> \n> I'm not even sure if it's possible to get here with a pathspec of \"/\",\n> since we'd probably give a \"/ is outside repository\" error before then.\n> \n> So maybe this case doesn't even matter. If it doesn't, then it might\n> simplify the function a bit to do the empty-pathspec check before\n> handling trailing slashes. But handling it does help make it more clear\n> this function is doing the right thing no matter what input it is given,\n> so that's what I went with here.\n\nWorks for me. Thanks for your careful attention here.\n\n-Stolee\n\n> revision.c | 1 +\n> 1 file changed, 1 insertion(+)\n> \n> diff --git a/revision.c b/revision.c\n> index 5ed86e4524..b80868556b 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -702,6 +702,7 @@ static void prepare_to_use_bloom_filter(struct rev_info *revs)\n> \tlen = strlen(path);\n> \tif (!len) {\n> \t\trevs->bloom_filter_settings = NULL;\n> +\t\tfree(path_alloc);\n> \t\treturn;\n> \t}\n> \n> \n\n"},{"id":"402820","messageId":"xmqqv9hys7ag.fsf@gitster.c.googlers.com","threadId":"53980","inReplyTo":"20200804074353.GA284046@coredump.intra.peff.net","subject":"Re: [PATCH 1/3] config: work around gcc-10 -Wstringop-overflow warning","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-04T16:30:15Z","receivedAt":"2020-08-04T16:30:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Compiling with gcc-10, -O2, and -fsanitize=undefined results in a\n> compiler warning:\n>\n> config.c: In function ‘git_config_copy_or_rename_section_in_file’:\n> config.c:3170:17: error: writing 1 byte into a region of size 0 [-Werror=stringop-overflow=]\n> 3170 | output[0] = '\\t';\n> | ~~~~~~~~~~^~~~~~\n> config.c:3076:7: note: at offset -1 to object ‘buf’ with size 1024 declared here\n> 3076 | char buf[1024];\n> | ^~~\n>\n> This is a false positive. The interesting lines of code are:\n>\n> int i;\n> char *output = buf;\n> ...\n> for (i = 0; buf[i] && isspace(buf[i]); i++)\n> ; /* do nothing */\n> ...\n> int offset;\n> offset = section_name_match(&buf[i], old_name);\n> if (offset > 0) {\n> ...\n> output += offset + i;\n> if (strlen(output) > 0) {\n> \t\t /*\n> \t\t * More content means there's\n> \t\t * a declaration to put on the\n> \t\t * next line; indent with a\n> \t\t * tab\n> \t\t */\n> \t\t output -= 1;\n> \t\t output[0] = '\\t';\n> \t }\n> }\n>\n> So we do assign output to buf initially. Later we increment it based on\n> \"offset\" and \"i\" and then subtract \"1\" from it. That latter step is what\n> the compiler is complaining about; it could lead to going off the left\n> side of the array if \"output == buf\" at the moment of the subtraction.\n> For that to be the case, then \"offset + i\" would have to be 0. But that\n> can't happen:\n>\n> - we know that \"offset\" is at least 1, since we're in a conditional\n> block that checks that\n>\n> - we know that \"i\" is not negative, since it started at 0 and only\n> incremented over whitespace\n>\n> So the sum must be at least 1, and therefore it's OK to subtract one\n> from \"output\".\n>\n> But that's not quite the whole story. Since \"i\" is an int, it could in\n> theory be possible to overflow to negative (when counting whitespace on\n> a very large string). But we know that's impossible because we're\n> counting the 1024-byte buffer we just fed to fgets(), so it can never be\n> larger than that.\n>\n> Switching the type of \"i\" to \"unsigned\" makes the warning go away, so\n> let's do that.\n>\n> Arguably size_t is an even better type (for this and for the other\n> length fields), but switching to it produces a similar but distinct\n> warning:\n>\n> config.c: In function ‘git_config_copy_or_rename_section_in_file’:\n> config.c:3170:13: error: array subscript -1 is outside array bounds of ‘char[1024]’ [-Werror=array-bounds]\n> 3170 | output[0] = '\\t';\n> | ~~~~~~^~~\n> config.c:3076:7: note: while referencing ‘buf’\n> 3076 | char buf[1024];\n> | ^~~\n>\n> If we were to ever switch off of fgets() to strbuf_getline() or similar,\n> we'd probably need to use size_t to avoid other overflow problems. But\n> for now we know we're safe because of the small fixed size of our\n> buffer.\n>\n> Signed-off-by: Jeff King \n> ---\n\nThanks. 80 lines of informative log message to explain a one liner\nwas surprisingly pleasnt to read. Nicely done.\n\n> config.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/config.c b/config.c\n> index 8db9c77098..2b79fe76ad 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -3115,7 +3115,7 @@ static int git_config_copy_or_rename_section_in_file(const char *config_filename\n> \t}\n> \n> \twhile (fgets(buf, sizeof(buf), config_file)) {\n> -\t\tint i;\n> +\t\tunsigned i;\n> \t\tint length;\n> \t\tint is_section = 0;\n> \t\tchar *output = buf;\n"},{"id":"402909","messageId":"20200805151712.GD9546@syl.lan","threadId":"53980","inReplyTo":"20200804074652.GB284046@coredump.intra.peff.net","subject":"Re: [PATCH 2/3] revision: avoid out-of-bounds read/write on empty pathspec","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-08-05T15:17:12Z","receivedAt":"2020-08-05T15:58:51Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Aug 04, 2020 at 03:46:52AM -0400, Jeff King wrote:\n> Running t4216 with ASan results in it complaining of an out-of-bounds\n> read in prepare_to_use_bloom_filter(). The issue is this code to strip a\n> trailing slash:\n>\n> last_index = pi->len - 1;\n> if (pi->match[last_index] == '/') {\n>\n> because we have no guarantee that pi->len isn't zero. This can happen if\n> the pathspec is \".\", as we translate that to an empty string. And if\n> that read of random memory does trigger the conditional, we'd then do an\n> out-of-bounds write:\n>\n> path_alloc = xstrdup(pi->match);\n> path_alloc[last_index] = '\\0';\n>\n> Let's make sure to check the length before subtracting. Note that for an\n> empty pathspec, we'd end up bailing from the function a few lines later,\n> which makes it tempting to just:\n>\n> if (!pi->len)\n> return;\n>\n> early here. But our code here is stripping a trailing slash, and we need\n> to check for emptiness after stripping that slash, too. So we'd have two\n> blocks, which would require repeating some cleanup code.\n>\n> Instead, just skip the trailing-slash for an empty string. Setting\n> last_index at all in the case is awkward since it will have a nonsense\n> value (and it uses an \"int\", which is a too-small type for a string\n> anyway). So while we're here, let's:\n>\n> - drop last_index entirely; it's only used in two spots right next to\n> each other and writing out \"pi->len - 1\" in both is actually easier\n> to follow\n>\n> - use xmemdupz() to duplicate the string. This is slightly more\n> efficient, but more importantly makes the intent more clear by\n> allocating the correct-sized substring in the first place. It also\n> eliminates any question of whether path_alloc is as long as\n> pi->match (which it would not be if pi->match has any embedded NULs,\n> though in practice this is probably impossible).\n>\n> Signed-off-by: Jeff King \n> ---\n> Another variant is to actually stop assigning revs->bloom_filter_settings\n> so early, so that we don't have to clean it up. And then once we're sure\n> we're going to use it and have passed all of our early-return checks,\n> then assign it. But that conflicts with the get_bloom_filter_settings()\n> patch in:\n>\n> https://lore.kernel.org/git/08479793c1274d5ee0f063578bb0f4d93c910fa9.1596480582.git.me@ttaylorr.com/\n>\n> so I didn't go that way.\n\nGood, I was going to ask about that. Thanks for thinking of those\npatches and avoiding introducing a conflicting patch (of course, I\nimplemented this other approach in github/git, and so will pay the price\nwhen I deal with the conflict myself ;-)).\n\n>\n> revision.c | 7 ++-----\n> 1 file changed, 2 insertions(+), 5 deletions(-)\n>\n> diff --git a/revision.c b/revision.c\n> index 6de29cdf7a..5ed86e4524 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -669,7 +669,6 @@ static void prepare_to_use_bloom_filter(struct rev_info *revs)\n> \tstruct pathspec_item *pi;\n> \tchar *path_alloc = NULL;\n> \tconst char *path, *p;\n> -\tint last_index;\n> \tsize_t len;\n> \tint path_component_nr = 1;\n>\n> @@ -692,12 +691,10 @@ static void prepare_to_use_bloom_filter(struct rev_info *revs)\n> \t\treturn;\n>\n> \tpi = &revs->pruning.pathspec.items[0];\n> -\tlast_index = pi->len - 1;\n>\n> \t/* remove single trailing slash from path, if needed */\n> -\tif (pi->match[last_index] == '/') {\n> -\t\tpath_alloc = xstrdup(pi->match);\n> -\t\tpath_alloc[last_index] = '\\0';\n> +\tif (pi->len > 0 && pi->match[pi->len - 1] == '/') {\n> +\t\tpath_alloc = xmemdupz(pi->match, pi->len - 1);\n\nLooks correct. Thanks.\n\n> \t\tpath = path_alloc;\n> \t} else\n> \t\tpath = pi->match;\n> --\n> 2.28.0.536.ga4d8134877\n\n Reviewed-by: Taylor Blau \n\nThanks,\nTaylor\n"},{"id":"402929","messageId":"20200805151507.GC9546@syl.lan","threadId":"53980","inReplyTo":"xmqqv9hys7ag.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 1/3] config: work around gcc-10 -Wstringop-overflow warning","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-08-05T15:15:07Z","receivedAt":"2020-08-05T17:42:59Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Aug 04, 2020 at 09:30:15AM -0700, Junio C Hamano wrote:\n> Jeff King writes:\n>\n> > Compiling with gcc-10, -O2, and -fsanitize=undefined results in a\n> > compiler warning:\n> >\n> > config.c: In function ‘git_config_copy_or_rename_section_in_file’:\n> > config.c:3170:17: error: writing 1 byte into a region of size 0 [-Werror=stringop-overflow=]\n> > 3170 | output[0] = '\\t';\n> > | ~~~~~~~~~~^~~~~~\n> > config.c:3076:7: note: at offset -1 to object ‘buf’ with size 1024 declared here\n> > 3076 | char buf[1024];\n> > | ^~~\n> >\n> > This is a false positive. The interesting lines of code are:\n> >\n> > int i;\n> > char *output = buf;\n> > ...\n> > for (i = 0; buf[i] && isspace(buf[i]); i++)\n> > ; /* do nothing */\n> > ...\n> > int offset;\n> > offset = section_name_match(&buf[i], old_name);\n> > if (offset > 0) {\n> > ...\n> > output += offset + i;\n> > if (strlen(output) > 0) {\n> > \t\t /*\n> > \t\t * More content means there's\n> > \t\t * a declaration to put on the\n> > \t\t * next line; indent with a\n> > \t\t * tab\n> > \t\t */\n> > \t\t output -= 1;\n> > \t\t output[0] = '\\t';\n> > \t }\n> > }\n> >\n> > So we do assign output to buf initially. Later we increment it based on\n> > \"offset\" and \"i\" and then subtract \"1\" from it. That latter step is what\n> > the compiler is complaining about; it could lead to going off the left\n> > side of the array if \"output == buf\" at the moment of the subtraction.\n> > For that to be the case, then \"offset + i\" would have to be 0. But that\n> > can't happen:\n> >\n> > - we know that \"offset\" is at least 1, since we're in a conditional\n> > block that checks that\n> >\n> > - we know that \"i\" is not negative, since it started at 0 and only\n> > incremented over whitespace\n> >\n> > So the sum must be at least 1, and therefore it's OK to subtract one\n> > from \"output\".\n> >\n> > But that's not quite the whole story. Since \"i\" is an int, it could in\n> > theory be possible to overflow to negative (when counting whitespace on\n> > a very large string). But we know that's impossible because we're\n> > counting the 1024-byte buffer we just fed to fgets(), so it can never be\n> > larger than that.\n> >\n> > Switching the type of \"i\" to \"unsigned\" makes the warning go away, so\n> > let's do that.\n> >\n> > Arguably size_t is an even better type (for this and for the other\n> > length fields), but switching to it produces a similar but distinct\n> > warning:\n> >\n> > config.c: In function ‘git_config_copy_or_rename_section_in_file’:\n> > config.c:3170:13: error: array subscript -1 is outside array bounds of ‘char[1024]’ [-Werror=array-bounds]\n> > 3170 | output[0] = '\\t';\n> > | ~~~~~~^~~\n> > config.c:3076:7: note: while referencing ‘buf’\n> > 3076 | char buf[1024];\n> > | ^~~\n> >\n> > If we were to ever switch off of fgets() to strbuf_getline() or similar,\n> > we'd probably need to use size_t to avoid other overflow problems. But\n> > for now we know we're safe because of the small fixed size of our\n> > buffer.\n> >\n> > Signed-off-by: Jeff King \n> > ---\n>\n> Thanks. 80 lines of informative log message to explain a one liner\n> was surprisingly pleasnt to read. Nicely done.\n\nAgreed, and sorry that this took me so long to read (I thought that I\nhad read it when you sent it, but apparently not). Your reasoning is\nsensible, and I agree that your fix is appropriate.\n\n Reviewed-by: Taylor Blau \n\n> > config.c | 2 +-\n> > 1 file changed, 1 insertion(+), 1 deletion(-)\n> >\n> > diff --git a/config.c b/config.c\n> > index 8db9c77098..2b79fe76ad 100644\n> > --- a/config.c\n> > +++ b/config.c\n> > @@ -3115,7 +3115,7 @@ static int git_config_copy_or_rename_section_in_file(const char *config_filename\n> > \t}\n> >\n> > \twhile (fgets(buf, sizeof(buf), config_file)) {\n> > -\t\tint i;\n> > +\t\tunsigned i;\n> > \t\tint length;\n> > \t\tint is_section = 0;\n> > \t\tchar *output = buf;\n\nThanks,\nTaylor\n"},{"id":"402938","messageId":"20200805151953.GE9546@syl.lan","threadId":"53980","inReplyTo":"5159cfbb-55a4-8010-66a3-f6b4e4b05d7e@gmail.com","subject":"Re: [PATCH 3/3] revision: avoid leak when preparing bloom filter for \"/\"","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-08-05T15:19:53Z","receivedAt":"2020-08-05T19:23:52Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Aug 04, 2020 at 09:09:21AM -0400, Derrick Stolee wrote:\n> On 8/4/2020 3:50 AM, Jeff King wrote:\n> > If we're given an empty pathspec, we refuse to set up bloom filters, as\n> > described in f3c2a36810 (revision: empty pathspecs should not use Bloom\n> > filters, 2020-07-01).\n> >\n> > But before the empty string check, we drop any trailing slash by\n> > allocating a new string without it. So a pathspec consisting only of \"/\"\n> > will allocate that string, but then still cause us to bail, leaking the\n> > new string. Let's make sure to free it.\n> >\n> > Signed-off-by: Jeff King \n> > ---\n> > Just noticed while reading the function to fix the previous patch.\n> >\n> > I'm not even sure if it's possible to get here with a pathspec of \"/\",\n> > since we'd probably give a \"/ is outside repository\" error before then.\n> >\n> > So maybe this case doesn't even matter. If it doesn't, then it might\n> > simplify the function a bit to do the empty-pathspec check before\n\nFor what it's worth, I am almost certain that this isn't possible after\nyour last patch, but I also agree that it's not hurting anything in the\nmeantime, either. So...\n\n> > handling trailing slashes. But handling it does help make it more clear\n> > this function is doing the right thing no matter what input it is given,\n> > so that's what I went with here.\n>\n> Works for me. Thanks for your careful attention here.\n\nWorks for me, too.\n\n Reviewed-by: Taylor Blau \n\n> -Stolee\n>\n> > revision.c | 1 +\n> > 1 file changed, 1 insertion(+)\n> >\n> > diff --git a/revision.c b/revision.c\n> > index 5ed86e4524..b80868556b 100644\n> > --- a/revision.c\n> > +++ b/revision.c\n> > @@ -702,6 +702,7 @@ static void prepare_to_use_bloom_filter(struct rev_info *revs)\n> > \tlen = strlen(path);\n> > \tif (!len) {\n> > \t\trevs->bloom_filter_settings = NULL;\n> > +\t\tfree(path_alloc);\n> > \t\treturn;\n> > \t}\n> >\n> >\n\nThanks,\nTaylor\n"}]}