diff mbox series

[v2] maintenance --unregister: fix uninit'd data use & -Wdeclaration-after-statement

Message ID patch-v2-1.1-f37e99c9d59-20221115T160240Z-avarab@gmail.com (mailing list archive)
State Accepted
Commit be0fd5722853e56fab8bc99cea9791d7b90fd0dd
Headers show
Series [v2] maintenance --unregister: fix uninit'd data use & -Wdeclaration-after-statement | expand

Commit Message

Ævar Arnfjörð Bjarmason Nov. 15, 2022, 4:04 p.m. UTC 
Since (maintenance: add option to register in a specific config,
2022-11-09) we've been unable to build with "DEVELOPER=1" without
"DEVOPTS=no-error", as the added code triggers a
"-Wdeclaration-after-statement" warning.

And worse than that, the data handed to git_configset_clear() is
uninitialized, as can be spotted with e.g.:

	./t7900-maintenance.sh -vixd --run=23 --valgrind
	[...]
	+ git maintenance unregister --force
	Conditional jump or move depends on uninitialised value(s)
	   at 0x6B5F1E: git_configset_clear (config.c:2367)
	   by 0x4BA64E: maintenance_unregister (gc.c:1619)
	   by 0x4BD278: cmd_maintenance (gc.c:2650)
	   by 0x409905: run_builtin (git.c:466)
	   by 0x40A21C: handle_builtin (git.c:721)
	   by 0x40A58E: run_argv (git.c:788)
	   by 0x40AF68: cmd_main (git.c:926)
	   by 0x5D39FE: main (common-main.c:57)
	 Uninitialised value was created by a stack allocation
	   at 0x4BA22C: maintenance_unregister (gc.c:1557)

Let's fix both of these issues, and also move the scope of the
variable to the "if" statement it's used in, to make it obvious where
it's used.

Helped-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
Range-diff against v1:
1:  54d405f15f1 ! 1:  f37e99c9d59 builtin/gc.c: fix -Wdeclaration-after-statement
    @@ Metadata
     Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## Commit message ##
    -    builtin/gc.c: fix -Wdeclaration-after-statement
    +    maintenance --unregister: fix uninit'd data use & -Wdeclaration-after-statement
     
    -    In 1f80129d61b (maintenance: add option to register in a specific
    -    config, 2022-11-09) code was added which triggers a
    -    "-Wdeclaration-after-statement" warning, which is on by default with
    -    DEVELOPER=1.
    +    Since (maintenance: add option to register in a specific config,
    +    2022-11-09) we've been unable to build with "DEVELOPER=1" without
    +    "DEVOPTS=no-error", as the added code triggers a
    +    "-Wdeclaration-after-statement" warning.
     
    +    And worse than that, the data handed to git_configset_clear() is
    +    uninitialized, as can be spotted with e.g.:
    +
    +            ./t7900-maintenance.sh -vixd --run=23 --valgrind
    +            [...]
    +            + git maintenance unregister --force
    +            Conditional jump or move depends on uninitialised value(s)
    +               at 0x6B5F1E: git_configset_clear (config.c:2367)
    +               by 0x4BA64E: maintenance_unregister (gc.c:1619)
    +               by 0x4BD278: cmd_maintenance (gc.c:2650)
    +               by 0x409905: run_builtin (git.c:466)
    +               by 0x40A21C: handle_builtin (git.c:721)
    +               by 0x40A58E: run_argv (git.c:788)
    +               by 0x40AF68: cmd_main (git.c:926)
    +               by 0x5D39FE: main (common-main.c:57)
    +             Uninitialised value was created by a stack allocation
    +               at 0x4BA22C: maintenance_unregister (gc.c:1557)
    +
    +    Let's fix both of these issues, and also move the scope of the
    +    variable to the "if" statement it's used in, to make it obvious where
    +    it's used.
    +
    +    Helped-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## builtin/gc.c ##
    -@@ builtin/gc.c: static int maintenance_unregister(int argc, const char **argv, const char *prefi
    - 	int found = 0;
    - 	struct string_list_item *item;
    - 	const struct string_list *list;
    -+	struct config_set cs;
    - 
    - 	argc = parse_options(argc, argv, prefix, options,
    - 			     builtin_maintenance_unregister_usage, 0);
     @@ builtin/gc.c: static int maintenance_unregister(int argc, const char **argv, const char *prefi
      		usage_with_options(builtin_maintenance_unregister_usage,
      				   options);
      
     -	struct config_set cs;
      	if (config_file) {
    ++		struct config_set cs;
    ++
      		git_configset_init(&cs);
      		git_configset_add_file(&cs, config_file);
    + 		list = git_configset_get_value_multi(&cs, key);
    ++		git_configset_clear(&cs);
    + 	} else {
    + 		list = git_config_get_value_multi(key);
    + 	}
    +@@ builtin/gc.c: static int maintenance_unregister(int argc, const char **argv, const char *prefi
    + 		die(_("repository '%s' is not registered"), maintpath);
    + 	}
    + 
    +-	git_configset_clear(&cs);
    + 	free(maintpath);
    + 	return 0;
    + }

 builtin/gc.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Comments

Ronan Pigott Nov. 15, 2022, 4:32 p.m. UTC | #1 
November 15, 2022 9:04 AM, "Ævar Arnfjörð Bjarmason" <avarab@gmail.com> wrote:

> Since (maintenance: add option to register in a specific config,
> 2022-11-09) we've been unable to build with "DEVELOPER=1" without
> "DEVOPTS=no-error", as the added code triggers a
> "-Wdeclaration-after-statement" warning.
> 
> And worse than that, the data handed to git_configset_clear() is
> uninitialized, as can be spotted with e.g.:

Sorry, I'm a little confused. I had sent v1, v2, and v3 of the "maintenance: add option [...]" patch, but as I understand it v2 had already been applied so I was asked to resubmit the changes from v3 rebased on next.

I had done that in [1], but these issues were caught in review so I submitted a v2 of that correction in [2] which declares the configsset earlier and unconditionally initializes it is cleared. Are these further issues discovered after [2] was applied, or was there some issue rebasing the patches?

[1] https://lore.kernel.org/git/20221110225310.7488-1-ronan@rjp.ie/
[2] https://lore.kernel.org/git/20221111231910.26769-1-ronan@rjp.ie/
Taylor Blau Nov. 15, 2022, 5:34 p.m. UTC | #2 
On Tue, Nov 15, 2022 at 05:04:27PM +0100, Ævar Arnfjörð Bjarmason wrote:
> Since (maintenance: add option to register in a specific config,
> 2022-11-09) we've been unable to build with "DEVELOPER=1" without
> "DEVOPTS=no-error", as the added code triggers a
> "-Wdeclaration-after-statement" warning.
>
> And worse than that, the data handed to git_configset_clear() is
> uninitialized, as can be spotted with e.g.:
>
> 	./t7900-maintenance.sh -vixd --run=23 --valgrind
> 	[...]
> 	+ git maintenance unregister --force
> 	Conditional jump or move depends on uninitialised value(s)
> 	   at 0x6B5F1E: git_configset_clear (config.c:2367)
> 	   by 0x4BA64E: maintenance_unregister (gc.c:1619)
> 	   by 0x4BD278: cmd_maintenance (gc.c:2650)
> 	   by 0x409905: run_builtin (git.c:466)
> 	   by 0x40A21C: handle_builtin (git.c:721)
> 	   by 0x40A58E: run_argv (git.c:788)
> 	   by 0x40AF68: cmd_main (git.c:926)
> 	   by 0x5D39FE: main (common-main.c:57)
> 	 Uninitialised value was created by a stack allocation
> 	   at 0x4BA22C: maintenance_unregister (gc.c:1557)
>
> Let's fix both of these issues, and also move the scope of the
> variable to the "if" statement it's used in, to make it obvious where
> it's used.
>
> Helped-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>

Thanks, both. I could have sworn I picked up the right version of this
topic when queuing, but apparently not.

I pushed this out to 'next' and rebuilt jch and seen based on the new
version. The result now should be OK. Thanks again for noticing and
working together.

Thanks,
Taylor
Taylor Blau Nov. 15, 2022, 5:35 p.m. UTC | #3 
On Tue, Nov 15, 2022 at 04:32:03PM +0000, ronan@rjp.ie wrote:
> I had done that in [1], but these issues were caught in review so I
> submitted a v2 of that correction in [2] which declares the configsset
> earlier and unconditionally initializes it is cleared. Are these
> further issues discovered after [2] was applied, or was there some
> issue rebasing the patches?
>
> [1] https://lore.kernel.org/git/20221110225310.7488-1-ronan@rjp.ie/
> [2] https://lore.kernel.org/git/20221111231910.26769-1-ronan@rjp.ie/

It was human error on my part. Please let me know if the result in
'next' now is OK to you.

Thanks,
Taylor
Ronan Pigott Nov. 15, 2022, 6 p.m. UTC | #4 
November 15, 2022 10:35 AM, "Taylor Blau" <me@ttaylorr.com> wrote:

> It was human error on my part. Please let me know if the result in
> 'next' now is OK to you.

Had a look and I'm just wondering if we're sure it's alright to clear
the configset before using the list we found in it. Doesn't that list
point into the configset?
Taylor Blau Nov. 15, 2022, 6:54 p.m. UTC | #5 
On Tue, Nov 15, 2022 at 06:00:45PM +0000, ronan@rjp.ie wrote:
> November 15, 2022 10:35 AM, "Taylor Blau" <me@ttaylorr.com> wrote:
>
> > It was human error on my part. Please let me know if the result in
> > 'next' now is OK to you.
>
> Had a look and I'm just wondering if we're sure it's alright to clear
> the configset before using the list we found in it. Doesn't that list
> point into the configset?

It does. I should not have trusted the incoming patch so blindly. See:

    https://lore.kernel.org/git/2cbead254b77cb02d219bca8f628dc4362c045b0.1668538355.git.me@ttaylorr.com/

for a fixup on top.

Thanks,
Taylor
diff mbox series

Patch

diff --git a/builtin/gc.c b/builtin/gc.c
index 56b107e7f0b..d87cf84041f 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1550,11 +1550,13 @@  static int maintenance_unregister(int argc, const char **argv, const char *prefi
 		usage_with_options(builtin_maintenance_unregister_usage,
 				   options);
 
-	struct config_set cs;
 	if (config_file) {
+		struct config_set cs;
+
 		git_configset_init(&cs);
 		git_configset_add_file(&cs, config_file);
 		list = git_configset_get_value_multi(&cs, key);
+		git_configset_clear(&cs);
 	} else {
 		list = git_config_get_value_multi(key);
 	}
@@ -1590,7 +1592,6 @@  static int maintenance_unregister(int argc, const char **argv, const char *prefi
 		die(_("repository '%s' is not registered"), maintpath);
 	}
 
-	git_configset_clear(&cs);
 	free(maintpath);
 	return 0;
 }