[PATCH] builtin/gc.c: fix -Wdeclaration-after-statement

[PATCH] builtin/gc.c: fix -Wdeclaration-after-statement

[Ævar Arnfjörð Bjarmason]

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.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

This landed in the latest "next" push-out, causing e.g this CI
failure:
https://github.com/git/git/actions/runs/3467538041/jobs/5792504315

 builtin/gc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index 56b107e7f0b..22a990db0be 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1543,6 +1543,7 @@ 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);
@@ -1550,7 +1551,6 @@ 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) {
 		git_configset_init(&cs);
 		git_configset_add_file(&cs, config_file);
-- 
2.38.0.1473.g172bcc0511c

Re: [PATCH] builtin/gc.c: fix -Wdeclaration-after-statement

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 2146 bytes --]

Hi Ævar,

On Tue, 15 Nov 2022, Ævar Arnfjörð Bjarmason wrote:

> 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.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>
> This landed in the latest "next" push-out, causing e.g this CI
> failure:
> https://github.com/git/git/actions/runs/3467538041/jobs/5792504315

I looked at the same thing all morning, and found that
https://lore.kernel.org/git/20221111231910.26769-1-ronan@rjp.ie/
_probably_ was designed to fix the same issue (and I think you agree that
that patch should be used instead of yours because it is more complete in
fixing left-over issues).

However, try as I might, I did not find out yet why it does not apply
cleanly over here (I got side-tracked into range-diff'ing patches
downloaded from the archive, which is quite the challenge and not even fun
because `range-diff` requires commits, not emails, so I side-tracked
myself into teaching `range-diff` to accept mbox arguments).

Maybe you can adjust that patch so it applies cleanly?

Ciao,
Dscho

>
>  builtin/gc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/gc.c b/builtin/gc.c
> index 56b107e7f0b..22a990db0be 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -1543,6 +1543,7 @@ 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);
> @@ -1550,7 +1551,6 @@ 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) {
>  		git_configset_init(&cs);
>  		git_configset_add_file(&cs, config_file);
> --
> 2.38.0.1473.g172bcc0511c
>
>
>

Re: [PATCH] builtin/gc.c: fix -Wdeclaration-after-statement

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 3352 bytes --]

Hi Ævar,

On Tue, 15 Nov 2022, Johannes Schindelin wrote:

> On Tue, 15 Nov 2022, Ævar Arnfjörð Bjarmason wrote:
>
> > 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.
> >
> > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> > ---
> >
> > This landed in the latest "next" push-out, causing e.g this CI
> > failure:
> > https://github.com/git/git/actions/runs/3467538041/jobs/5792504315
>
> I looked at the same thing all morning, and found that
> https://lore.kernel.org/git/20221111231910.26769-1-ronan@rjp.ie/
> _probably_ was designed to fix the same issue (and I think you agree that
> that patch should be used instead of yours because it is more complete in
> fixing left-over issues).
>
> However, try as I might, I did not find out yet why it does not apply
> cleanly over here (I got side-tracked into range-diff'ing patches
> downloaded from the archive, which is quite the challenge and not even fun
> because `range-diff` requires commits, not emails, so I side-tracked
> myself into teaching `range-diff` to accept mbox arguments).
>
> Maybe you can adjust that patch so it applies cleanly?

Seems that the v2 of "maintenance: improve error reporting for
unregister" [*1*] was _partially_ folded into v3 of "maintenance: add
option to register in a specific config": At least the config change that
talks about `--config-file` inste of `--config` _is_ part of v3, and one
reason that the former patch does not apply cleanly on top of the latter
patch. Which is a bit funny because the former patch was sent out two days
_after_ the latter patch.

And it looks as if there is one more thing that needs to be forward-ported
from that no-longer-applying patch: the initialization of the configset,
otherwise we're potentially trying to clear an uninitialized data
structure.

This is the patch I am currently testing (on top of Git for Windows'
`shears/seen` branch:
https://github.com/git-for-windows/git/commit/cd7b86d19f):

-- snip --
diff --git a/builtin/gc.c b/builtin/gc.c
index 635f12499d68..a3f63880dfba 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1569,6 +1569,7 @@ 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);
@@ -1576,9 +1577,8 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi
 		usage_with_options(builtin_maintenance_unregister_usage,
 				   options);

-	struct config_set cs;
+	git_configset_init(&cs);
 	if (config_file) {
-		git_configset_init(&cs);
 		git_configset_add_file(&cs, config_file);
 		list = git_configset_get_value_multi(&cs, key);
 	} else {
-- snap --

Ævar, can you please have a thorough look and see whether there is
anything else we were missing before advancing this to `next`?

Thanks,
Dscho

Footnote *1*: https://lore.kernel.org/git/20221111231910.26769-1-ronan@rjp.ie/
Footnote *2*: https://lore.kernel.org/git/20221109190708.22725-3-ronan@rjp.ie/

Re: [PATCH] builtin/gc.c: fix -Wdeclaration-after-statement

[Ævar Arnfjörð Bjarmason]

On Tue, Nov 15 2022, Johannes Schindelin wrote:

> Hi Ævar,
>
> On Tue, 15 Nov 2022, Johannes Schindelin wrote:
>
>> On Tue, 15 Nov 2022, Ævar Arnfjörð Bjarmason wrote:
>>
>> > 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.
>> >
>> > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> > ---
>> >
>> > This landed in the latest "next" push-out, causing e.g this CI
>> > failure:
>> > https://github.com/git/git/actions/runs/3467538041/jobs/5792504315
>>
>> I looked at the same thing all morning, and found that
>> https://lore.kernel.org/git/20221111231910.26769-1-ronan@rjp.ie/
>> _probably_ was designed to fix the same issue (and I think you agree that
>> that patch should be used instead of yours because it is more complete in
>> fixing left-over issues).
>>
>> However, try as I might, I did not find out yet why it does not apply
>> cleanly over here (I got side-tracked into range-diff'ing patches
>> downloaded from the archive, which is quite the challenge and not even fun
>> because `range-diff` requires commits, not emails, so I side-tracked
>> myself into teaching `range-diff` to accept mbox arguments).
>>
>> Maybe you can adjust that patch so it applies cleanly?
>
> Seems that the v2 of "maintenance: improve error reporting for
> unregister" [*1*] was _partially_ folded into v3 of "maintenance: add
> option to register in a specific config": At least the config change that
> talks about `--config-file` inste of `--config` _is_ part of v3, and one
> reason that the former patch does not apply cleanly on top of the latter
> patch. Which is a bit funny because the former patch was sent out two days
> _after_ the latter patch.
>
> And it looks as if there is one more thing that needs to be forward-ported
> from that no-longer-applying patch: the initialization of the configset,
> otherwise we're potentially trying to clear an uninitialized data
> structure.
>
> This is the patch I am currently testing (on top of Git for Windows'
> `shears/seen` branch:
> https://github.com/git-for-windows/git/commit/cd7b86d19f):
>
> -- snip --
> diff --git a/builtin/gc.c b/builtin/gc.c
> index 635f12499d68..a3f63880dfba 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -1569,6 +1569,7 @@ 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);
> @@ -1576,9 +1577,8 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi
>  		usage_with_options(builtin_maintenance_unregister_usage,
>  				   options);
>
> -	struct config_set cs;
> +	git_configset_init(&cs);
>  	if (config_file) {
> -		git_configset_init(&cs);
>  		git_configset_add_file(&cs, config_file);
>  		list = git_configset_get_value_multi(&cs, key);
>  	} else {
> -- snap --
>
> Ævar, can you please have a thorough look and see whether there is
> anything else we were missing before advancing this to `next`?

Thanks, I submitted a v2 just now that addresses the unit'd issue as
well, and as we're fixing that we can just reduce the scope of the
variable, which makes it lifetime more obvious:

https://lore.kernel.org/git/patch-v2-1.1-f37e99c9d59-20221115T160240Z-avarab@gmail.com/

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

[Ævar Arnfjörð Bjarmason]

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(-)

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;
 }
-- 
2.38.0.1473.g172bcc0511c

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

[Taylor Blau]

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

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

[ronan]

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/

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

[Taylor Blau]

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

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

[ronan]

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?

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

[Taylor Blau]

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