{"thread":{"id":"62478","subject":"[PATCH] builtin/difftool: intialize some hashmap variables","startedAt":"2024-11-11T16:31:59Z","lastAt":"2024-11-11T23:51:44Z","messageCount":5,"participants":["Simon Marchi","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"507056","messageId":"20241111162148.337375-1-simon.marchi@efficios.com","threadId":"62478","inReplyTo":null,"subject":"[PATCH] builtin/difftool: intialize some hashmap variables","fromName":"Simon Marchi","fromEmail":"simon.marchi@efficios.com","sentAt":"2024-11-11T16:21:44Z","receivedAt":"2024-11-11T16:31:59Z","isPatch":true,"sender":{"key":"simon.marchi@efficios.com","avatar":null},"body":"When running a dir-diff command that produces no diff, variables\n`wt_modified` and `tmp_modified` are used while uninitialized, causing:\n\n $ /home/smarchi/src/git/git-difftool --dir-diff master\n free(): invalid pointer\n [1] 334004 IOT instruction (core dumped) /home/smarchi/src/git/git-difftool --dir-diff master\n $ valgrind --track-origins=yes /home/smarchi/src/git/git-difftool --dir-diff master\n ...\n Invalid free() / delete / delete[] / realloc()\n at 0x48478EF: free (vg_replace_malloc.c:989)\n by 0x422CAC: hashmap_clear_ (hashmap.c:208)\n by 0x283830: run_dir_diff (difftool.c:667)\n by 0x284103: cmd_difftool (difftool.c:801)\n by 0x238E0F: run_builtin (git.c:484)\n by 0x2392B9: handle_builtin (git.c:750)\n by 0x2399BC: cmd_main (git.c:921)\n by 0x356FEF: main (common-main.c:64)\n Address 0x1ffefff180 is on thread 1's stack\n in frame #2, created by run_dir_diff (difftool.c:358)\n ...\n\nIf taking any `goto finish` path before these variables are initialized,\n`hashmap_clear_and_free()` operates on uninitialized data, sometimes\ncausing a crash.\n\nFix it by zero-initializing these variables, making\n`hashmap_clear_and_free()` a no-op in that case.\n\nSigned-off-by: Simon Marchi \nCc: Junio C Hamano \nCc: René Scharfe \nCc: Taylor Blau \nCc: Patrick Steinhardt \n---\n builtin/difftool.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/difftool.c b/builtin/difftool.c\nindex ca1b0890659b..b902f5d2ae17 100644\n--- a/builtin/difftool.c\n+++ b/builtin/difftool.c\n@@ -376,7 +376,8 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n \tstruct checkout lstate, rstate;\n \tint err = 0;\n \tstruct child_process cmd = CHILD_PROCESS_INIT;\n-\tstruct hashmap wt_modified, tmp_modified;\n+\tstruct hashmap wt_modified = {0};\n+\tstruct hashmap tmp_modified = {0};\n \tint indices_loaded = 0;\n \n \tworkdir = repo_get_work_tree(the_repository);\n\nbase-commit: b31fb630c0fc6869a33ed717163e8a1210460d94\n-- \n2.47.0\n\n"},{"id":"507073","messageId":"20241111205453.GA5019@coredump.intra.peff.net","threadId":"62478","inReplyTo":"20241111162148.337375-1-simon.marchi@efficios.com","subject":"Re: [PATCH] builtin/difftool: intialize some hashmap variables","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-11-11T20:54:53Z","receivedAt":"2024-11-11T20:54:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 11, 2024 at 11:21:44AM -0500, Simon Marchi wrote:\n\n> When running a dir-diff command that produces no diff, variables\n> `wt_modified` and `tmp_modified` are used while uninitialized, causing:\n> \n> $ /home/smarchi/src/git/git-difftool --dir-diff master\n> free(): invalid pointer\n> [1] 334004 IOT instruction (core dumped) /home/smarchi/src/git/git-difftool --dir-diff master\n> $ valgrind --track-origins=yes /home/smarchi/src/git/git-difftool --dir-diff master\n> ...\n> Invalid free() / delete / delete[] / realloc()\n> at 0x48478EF: free (vg_replace_malloc.c:989)\n> by 0x422CAC: hashmap_clear_ (hashmap.c:208)\n> by 0x283830: run_dir_diff (difftool.c:667)\n> by 0x284103: cmd_difftool (difftool.c:801)\n> by 0x238E0F: run_builtin (git.c:484)\n> by 0x2392B9: handle_builtin (git.c:750)\n> by 0x2399BC: cmd_main (git.c:921)\n> by 0x356FEF: main (common-main.c:64)\n> Address 0x1ffefff180 is on thread 1's stack\n> in frame #2, created by run_dir_diff (difftool.c:358)\n> ...\n> \n> If taking any `goto finish` path before these variables are initialized,\n> `hashmap_clear_and_free()` operates on uninitialized data, sometimes\n> causing a crash.\n> \n> Fix it by zero-initializing these variables, making\n> `hashmap_clear_and_free()` a no-op in that case.\n\nThe fix makes sense. I wondered if this had been broken for a long time,\nand if so, how we managed not to notice it. But it looks like it is a\nrecent problem, via 7f795a1715 (builtin/difftool: plug several trivial\nmemory leaks, 2024-09-26).\n\n> diff --git a/builtin/difftool.c b/builtin/difftool.c\n> index ca1b0890659b..b902f5d2ae17 100644\n> --- a/builtin/difftool.c\n> +++ b/builtin/difftool.c\n> @@ -376,7 +376,8 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n> \tstruct checkout lstate, rstate;\n> \tint err = 0;\n> \tstruct child_process cmd = CHILD_PROCESS_INIT;\n> -\tstruct hashmap wt_modified, tmp_modified;\n> +\tstruct hashmap wt_modified = {0};\n> +\tstruct hashmap tmp_modified = {0};\n> \tint indices_loaded = 0;\n\nThat commit likewise frees some other local variables, but they are all\nproperly initialized. So touching these two are sufficient.\n\nI'm not sure if zero-initialization is being a little too familiar with\nthe hashmap internals, though. The other variables use HASHMAP_INIT().\nShould we do the same here, like this:\n\ndiff --git a/builtin/difftool.c b/builtin/difftool.c\nindex 1a68ab6699..86995390c7 100644\n--- a/builtin/difftool.c\n+++ b/builtin/difftool.c\n@@ -374,7 +374,8 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n \tstruct checkout lstate, rstate;\n \tint err = 0;\n \tstruct child_process cmd = CHILD_PROCESS_INIT;\n-\tstruct hashmap wt_modified, tmp_modified;\n+\tstruct hashmap wt_modified = HASHMAP_INIT(path_entry_cmp, NULL);\n+\tstruct hashmap tmp_modified = HASHMAP_INIT(path_entry_cmp, NULL);\n \tint indices_loaded = 0;\n \n \tworkdir = get_git_work_tree();\n@@ -594,14 +595,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n \t * should be copied back to the working tree.\n \t * Do not copy back files when symlinks are used and the\n \t * external tool did not replace the original link with a file.\n-\t *\n-\t * These hashes are loaded lazily since they aren't needed\n-\t * in the common case of --symlinks and the difftool updating\n-\t * files through the symlink.\n \t */\n-\thashmap_init(&wt_modified, path_entry_cmp, NULL, wtindex.cache_nr);\n-\thashmap_init(&tmp_modified, path_entry_cmp, NULL, wtindex.cache_nr);\n-\n \tfor (i = 0; i < wtindex.cache_nr; i++) {\n \t\tstruct hashmap_entry dummy;\n \t\tconst char *name = wtindex.cache[i]->name;\n\nThat loses the initial table growth that the original had, but I think\nletting it grow in the usual way is fine here.\n\n-Peff\n"},{"id":"507075","messageId":"09bf8815-320b-494a-87c4-34bb0df08595@efficios.com","threadId":"62478","inReplyTo":"20241111205453.GA5019@coredump.intra.peff.net","subject":"Re: [PATCH] builtin/difftool: intialize some hashmap variables","fromName":"Simon Marchi","fromEmail":"simon.marchi@efficios.com","sentAt":"2024-11-11T21:22:26Z","receivedAt":"2024-11-11T21:22:30Z","isPatch":true,"sender":{"key":"simon.marchi@efficios.com","avatar":null},"body":"On 11/11/24 3:54 PM, Jeff King wrote:\n> On Mon, Nov 11, 2024 at 11:21:44AM -0500, Simon Marchi wrote:\n> \n>> When running a dir-diff command that produces no diff, variables\n>> `wt_modified` and `tmp_modified` are used while uninitialized, causing:\n>>\n>> $ /home/smarchi/src/git/git-difftool --dir-diff master\n>> free(): invalid pointer\n>> [1] 334004 IOT instruction (core dumped) /home/smarchi/src/git/git-difftool --dir-diff master\n>> $ valgrind --track-origins=yes /home/smarchi/src/git/git-difftool --dir-diff master\n>> ...\n>> Invalid free() / delete / delete[] / realloc()\n>> at 0x48478EF: free (vg_replace_malloc.c:989)\n>> by 0x422CAC: hashmap_clear_ (hashmap.c:208)\n>> by 0x283830: run_dir_diff (difftool.c:667)\n>> by 0x284103: cmd_difftool (difftool.c:801)\n>> by 0x238E0F: run_builtin (git.c:484)\n>> by 0x2392B9: handle_builtin (git.c:750)\n>> by 0x2399BC: cmd_main (git.c:921)\n>> by 0x356FEF: main (common-main.c:64)\n>> Address 0x1ffefff180 is on thread 1's stack\n>> in frame #2, created by run_dir_diff (difftool.c:358)\n>> ...\n>>\n>> If taking any `goto finish` path before these variables are initialized,\n>> `hashmap_clear_and_free()` operates on uninitialized data, sometimes\n>> causing a crash.\n>>\n>> Fix it by zero-initializing these variables, making\n>> `hashmap_clear_and_free()` a no-op in that case.\n> \n> The fix makes sense. I wondered if this had been broken for a long time,\n> and if so, how we managed not to notice it. But it looks like it is a\n> recent problem, via 7f795a1715 (builtin/difftool: plug several trivial\n> memory leaks, 2024-09-26).\n\nAre there tests for this specific scenario (no diff between the two\nversions)?\n\n> \n>> diff --git a/builtin/difftool.c b/builtin/difftool.c\n>> index ca1b0890659b..b902f5d2ae17 100644\n>> --- a/builtin/difftool.c\n>> +++ b/builtin/difftool.c\n>> @@ -376,7 +376,8 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n>> \tstruct checkout lstate, rstate;\n>> \tint err = 0;\n>> \tstruct child_process cmd = CHILD_PROCESS_INIT;\n>> -\tstruct hashmap wt_modified, tmp_modified;\n>> +\tstruct hashmap wt_modified = {0};\n>> +\tstruct hashmap tmp_modified = {0};\n>> \tint indices_loaded = 0;\n> \n> That commit likewise frees some other local variables, but they are all\n> properly initialized. So touching these two are sufficient.\n\nIndeed, I checked the other variables, they look fine.\n\n> I'm not sure if zero-initialization is being a little too familiar with\n> the hashmap internals, though.\n\nUp to you. In other C projects I worked on, it was typical that\nzero-ing an object would get it in a valid initial empty state, properly\nhandled by the destruction functions. This way, a big struct containing\nother objects could be initialized simply by zero-ing it, without having\nto initialize each component explicitly.\n\n> The other variables use HASHMAP_INIT().\n> Should we do the same here, like this:\n> \n> diff --git a/builtin/difftool.c b/builtin/difftool.c\n> index 1a68ab6699..86995390c7 100644\n> --- a/builtin/difftool.c\n> +++ b/builtin/difftool.c\n> @@ -374,7 +374,8 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n> \tstruct checkout lstate, rstate;\n> \tint err = 0;\n> \tstruct child_process cmd = CHILD_PROCESS_INIT;\n> -\tstruct hashmap wt_modified, tmp_modified;\n> +\tstruct hashmap wt_modified = HASHMAP_INIT(path_entry_cmp, NULL);\n> +\tstruct hashmap tmp_modified = HASHMAP_INIT(path_entry_cmp, NULL);\n> \tint indices_loaded = 0;\n> \n> \tworkdir = get_git_work_tree();\n> @@ -594,14 +595,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n> \t * should be copied back to the working tree.\n> \t * Do not copy back files when symlinks are used and the\n> \t * external tool did not replace the original link with a file.\n> -\t *\n> -\t * These hashes are loaded lazily since they aren't needed\n> -\t * in the common case of --symlinks and the difftool updating\n> -\t * files through the symlink.\n> \t */\n> -\thashmap_init(&wt_modified, path_entry_cmp, NULL, wtindex.cache_nr);\n> -\thashmap_init(&tmp_modified, path_entry_cmp, NULL, wtindex.cache_nr);\n> -\n> \tfor (i = 0; i < wtindex.cache_nr; i++) {\n> \t\tstruct hashmap_entry dummy;\n> \t\tconst char *name = wtindex.cache[i]->name;\n> \n> That loses the initial table growth that the original had, but I think\n> letting it grow in the usual way is fine here.\n\nI thought about it, but was indeed afraid to be told that this removes\nan optimization. If you think it's fine, I'm happy with it too.\n\nPlease let me know if you want a v2 or if you are just going to merge an\nupdated version of this patch.\n\nThanks,\n\nSimon\n"},{"id":"507080","messageId":"20241111220959.GE5019@coredump.intra.peff.net","threadId":"62478","inReplyTo":"09bf8815-320b-494a-87c4-34bb0df08595@efficios.com","subject":"Re: [PATCH] builtin/difftool: intialize some hashmap variables","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-11-11T22:09:59Z","receivedAt":"2024-11-11T22:10:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 11, 2024 at 04:22:26PM -0500, Simon Marchi wrote:\n\n> > The fix makes sense. I wondered if this had been broken for a long time,\n> > and if so, how we managed not to notice it. But it looks like it is a\n> > recent problem, via 7f795a1715 (builtin/difftool: plug several trivial\n> > memory leaks, 2024-09-26).\n> \n> Are there tests for this specific scenario (no diff between the two\n> versions)?\n\nPresumably not, or they'd have been segfaulting. ;) So it may be worth\nadding some to t7800.\n\n> > I'm not sure if zero-initialization is being a little too familiar with\n> > the hashmap internals, though.\n> \n> Up to you. In other C projects I worked on, it was typical that\n> zero-ing an object would get it in a valid initial empty state, properly\n> handled by the destruction functions. This way, a big struct containing\n> other objects could be initialized simply by zero-ing it, without having\n> to initialize each component explicitly.\n\nWe often follow that rule in git.git, too, but have been increasingly\nmoving to macro initializers. They make it easier if we ever need a\nnon-zero state as an invariant (e.g., STRBUF_INIT always points to a\ndummy string). In this case, it does look like HASHMAP_INIT sets\ndo_count_items.\n\nIt's not a bug in your program since we still hashmap_init() over the\nzero'd state, but it does feel a bit weird to me.\n\n> Please let me know if you want a v2 or if you are just going to merge an\n> updated version of this patch.\n\nThat's up to the maintainer. IMHO it's worth using HASHMAP_INIT, though,\nand perhaps adding a test.\n\n-Peff\n"},{"id":"507088","messageId":"xmqqmsi59vn7.fsf@gitster.g","threadId":"62478","inReplyTo":"20241111220959.GE5019@coredump.intra.peff.net","subject":"Re: [PATCH] builtin/difftool: intialize some hashmap variables","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-11-11T23:51:40Z","receivedAt":"2024-11-11T23:51:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> Please let me know if you want a v2 or if you are just going to merge an\n>> updated version of this patch.\n>\n> That's up to the maintainer. IMHO it's worth using HASHMAP_INIT, though,\n> and perhaps adding a test.\n\nYup, please do.\n\nThanks.\n"}]}