{"thread":{"id":"55866","subject":"[PATCH] remote: initialize values that might not be set","startedAt":"2021-06-07T12:39:45Z","lastAt":"2021-06-11T17:56:53Z","messageCount":6,"participants":["Derrick Stolee via GitGitGadget","Johannes Schindelin","Junio C Hamano","Derrick Stolee"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"426597","messageId":"pull.974.git.1623069570553.gitgitgadget@gmail.com","threadId":"55866","inReplyTo":null,"subject":"[PATCH] remote: initialize values that might not be set","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-06-07T12:39:29Z","receivedAt":"2021-06-07T12:39:45Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <dstolee@microsoft.com>\n\nI noticed during an unrelated test with Valgrind that these variables\nmight be left un-set by stat_tracking_info() in some cases. Initialize\nthem so that a later branch upon their value is consistent.\n\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\n---\n    remote: initialize values that might not be set\n    \n    A very minor fixup.\n    \n    Thanks, -Stolee\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-974%2Fderrickstolee%2Fremote-uninitialized-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-974/derrickstolee/remote-uninitialized-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/974\n\n remote.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/remote.c b/remote.c\nindex c3f85c17ca7c..a116392fb057 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -2101,7 +2101,7 @@ int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs,\n int format_tracking_info(struct branch *branch, struct strbuf *sb,\n \t\t\t enum ahead_behind_flags abf)\n {\n-\tint ours, theirs, sti;\n+\tint ours = 0, theirs = 0, sti = 0;\n \tconst char *full_base;\n \tchar *base;\n \tint upstream_is_gone = 0;\n\nbase-commit: 71ca53e8125e36efbda17293c50027d31681a41f\n-- \ngitgitgadget\n"},{"id":"426668","messageId":"nycvar.QRO.7.76.6.2106072355110.55@tvgsbejvaqbjf.bet","threadId":"55866","inReplyTo":"pull.974.git.1623069570553.gitgitgadget@gmail.com","subject":"Re: [PATCH] remote: initialize values that might not be set","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-06-07T21:59:14Z","receivedAt":"2021-06-07T21:59:25Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Stolee,\n\nOn Mon, 7 Jun 2021, Derrick Stolee via GitGitGadget wrote:\n\n> From: Derrick Stolee <dstolee@microsoft.com>\n>\n> I noticed during an unrelated test with Valgrind that these variables\n> might be left un-set by stat_tracking_info() in some cases. Initialize\n> them so that a later branch upon their value is consistent.\n>\n> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>\n> ---\n>     remote: initialize values that might not be set\n>\n>     A very minor fixup.\n>\n>     Thanks, -Stolee\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-974%2Fderrickstolee%2Fremote-uninitialized-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-974/derrickstolee/remote-uninitialized-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/974\n>\n>  remote.c | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/remote.c b/remote.c\n> index c3f85c17ca7c..a116392fb057 100644\n> --- a/remote.c\n> +++ b/remote.c\n> @@ -2101,7 +2101,7 @@ int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs,\n>  int format_tracking_info(struct branch *branch, struct strbuf *sb,\n>  \t\t\t enum ahead_behind_flags abf)\n>  {\n> -\tint ours, theirs, sti;\n> +\tint ours = 0, theirs = 0, sti = 0;\n\nWhile I like this change, I am somewhat confused where the values are used\nfor branching. The only time I see them used when `stat_branch_pair()` has\n_not_ initialized `ours` and `theirs` is in those `trace2_data_intmax()`\ncalls. Otherwise `sti` is set to -1 and the other users of `ours` and\n`theirs` aren't reached.\n\nIf my reading of the code is correct, maybe the commit message could be\nadjusted to talk about tracing instead of branching?\n\nThanks,\nDscho\n\n>  \tconst char *full_base;\n>  \tchar *base;\n>  \tint upstream_is_gone = 0;\n>\n> base-commit: 71ca53e8125e36efbda17293c50027d31681a41f\n> --\n> gitgitgadget\n>\n"},{"id":"426672","messageId":"xmqqlf7ltg4q.fsf@gitster.g","threadId":"55866","inReplyTo":"nycvar.QRO.7.76.6.2106072355110.55@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] remote: initialize values that might not be set","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-06-07T23:21:57Z","receivedAt":"2021-06-07T23:22:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n>> diff --git a/remote.c b/remote.c\n>> index c3f85c17ca7c..a116392fb057 100644\n>> --- a/remote.c\n>> +++ b/remote.c\n>> @@ -2101,7 +2101,7 @@ int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs,\n>>  int format_tracking_info(struct branch *branch, struct strbuf *sb,\n>>  \t\t\t enum ahead_behind_flags abf)\n>>  {\n>> -\tint ours, theirs, sti;\n>> +\tint ours = 0, theirs = 0, sti = 0;\n>\n> While I like this change, I am somewhat confused where the values are used\n> for branching. The only time I see them used when `stat_branch_pair()` has\n> _not_ initialized `ours` and `theirs` is in those `trace2_data_intmax()`\n> calls. Otherwise `sti` is set to -1 and the other users of `ours` and\n> `theirs` aren't reached.\n>\n> If my reading of the code is correct, maybe the commit message could be\n> adjusted to talk about tracing instead of branching?\n\nI too wondered why initializing them to 0 is safe (instead of hiding\nlatent bugs).  I think that stat_tracking_info() would always return\n-1 if returns before reaching the point in stat_branch_pair(), but\nit is not clear how we can futureproof the whole thing.\n\nIf these two are initialized to say -1 here, and then we had some\nsanity check, perhaps like so:\n\n\tsti = stat_tracking_info(branch, &ours, &theirs, &full_base, 0, abf);\n+\tassert(sti < 0 || (0 <= ours && 0 <= theirs));\n\tif (sti < 0) {\n\t\tif (!full_base)\n\t...\n\nto enforce the invariant we assume (i.e. OK sti means ours and\ntheirs are set), it would allow us to sleep better, perhaps?\n"},{"id":"426965","messageId":"nycvar.QRO.7.76.6.2106101046470.57@tvgsbejvaqbjf.bet","threadId":"55866","inReplyTo":"xmqqlf7ltg4q.fsf@gitster.g","subject":"Re: [PATCH] remote: initialize values that might not be set","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-06-10T09:24:52Z","receivedAt":"2021-06-10T09:25:02Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Tue, 8 Jun 2021, Junio C Hamano wrote:\n\n> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n>\n> >> diff --git a/remote.c b/remote.c\n> >> index c3f85c17ca7c..a116392fb057 100644\n> >> --- a/remote.c\n> >> +++ b/remote.c\n> >> @@ -2101,7 +2101,7 @@ int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs,\n> >>  int format_tracking_info(struct branch *branch, struct strbuf *sb,\n> >>  \t\t\t enum ahead_behind_flags abf)\n> >>  {\n> >> -\tint ours, theirs, sti;\n> >> +\tint ours = 0, theirs = 0, sti = 0;\n> >\n> > While I like this change, I am somewhat confused where the values are used\n> > for branching. The only time I see them used when `stat_branch_pair()` has\n> > _not_ initialized `ours` and `theirs` is in those `trace2_data_intmax()`\n> > calls. Otherwise `sti` is set to -1 and the other users of `ours` and\n> > `theirs` aren't reached.\n> >\n> > If my reading of the code is correct, maybe the commit message could be\n> > adjusted to talk about tracing instead of branching?\n>\n> I too wondered why initializing them to 0 is safe (instead of hiding\n> latent bugs).  I think that stat_tracking_info() would always return\n> -1 if returns before reaching the point in stat_branch_pair(),\n\nWhile that is true, I was trying to make a different point: I noticed that\nthe `ours`/`theirs` variables _are_ used, even if `sti` is negative. The\ncode that I looked at reads like this:\n\n\tint format_tracking_info(struct branch *branch, struct strbuf *sb,\n\t\t\t\t enum ahead_behind_flags abf)\n\t{\n\t\tint ours, theirs, sti;\n\t\tconst char *full_base;\n\t\tchar *base;\n\t\tint upstream_is_gone = 0;\n\n\t\ttrace2_region_enter(\"tracking\", \"stat_tracking_info\", NULL);\n\t\tsti = stat_tracking_info(branch, &ours, &theirs, &full_base, 0, abf);\n\t\ttrace2_data_intmax(\"tracking\", NULL, \"stat_tracking_info/ab_flags\", abf);\n\t\ttrace2_data_intmax(\"tracking\", NULL, \"stat_tracking_info/ab_result\", sti);\n\t\tif (abf == AHEAD_BEHIND_FULL) {\n\t\t    trace2_data_intmax(\"tracking\", NULL, \"stat_tracking_info/ab_ahead\", ours);\n\t\t    trace2_data_intmax(\"tracking\", NULL, \"stat_tracking_info/ab_behind\", theirs);\n\t\t}\n\t\ttrace2_region_leave(\"tracking\", \"stat_tracking_info\", NULL);\n\n\t\tif (sti < 0) {\n\t\t\tif (!full_base)\n\t\t\t\treturn 0;\n\t\t\tupstream_is_gone = 1;\n\t\t}\n\nYou will notice that there are two Trace2 calls in that conditional `abf\n== AHEAD_BEHIND_FULL` block.\n\nNow, what I failed to realize when reviewing this code (and I _bet_ Stolee\nwas in the same boat when they contributed the patch) is that this version\nof `format_tracking_info()` is different from what is in v2.32.0. It is\nthe version we have in the `microsoft/git` fork, and it has not yet made\nit upstream. To be precise, it is this commit:\nhttps://github.com/microsoft/git/commit/91209e591b0398c8334a78001a245807f7eb348a\n\nIn light of this, it might make more sense for us to fixup! this commit\nthusly:\n\n-- snip --\ndiff --git a/remote.c b/remote.c\nindex caed9cbc31b1..cfb7b6bd8d30 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -2110,7 +2110,7 @@ int format_tracking_info(struct branch *branch, struct strbuf *sb,\n \tsti = stat_tracking_info(branch, &ours, &theirs, &full_base, 0, abf);\n \ttrace2_data_intmax(\"tracking\", NULL, \"stat_tracking_info/ab_flags\", abf);\n \ttrace2_data_intmax(\"tracking\", NULL, \"stat_tracking_info/ab_result\", sti);\n-\tif (abf == AHEAD_BEHIND_FULL) {\n+\tif (sti >= 0 && abf == AHEAD_BEHIND_FULL) {\n \t    trace2_data_intmax(\"tracking\", NULL, \"stat_tracking_info/ab_ahead\", ours);\n \t    trace2_data_intmax(\"tracking\", NULL, \"stat_tracking_info/ab_behind\", theirs);\n \t}\n-- snap --\n\nThis would be in line with how `format_tracking_info()` avoids accessing\n`ours` and `theirs` if `stat_tracking_info()` returned a negative value.\n\nI opened the corresponding PR here:\nhttps://github.com/microsoft/git/pull/373\n\n> but it is not clear how we can futureproof the whole thing.\n>\n> If these two are initialized to say -1 here, and then we had some\n> sanity check, perhaps like so:\n>\n> \tsti = stat_tracking_info(branch, &ours, &theirs, &full_base, 0, abf);\n> +\tassert(sti < 0 || (0 <= ours && 0 <= theirs));\n> \tif (sti < 0) {\n> \t\tif (!full_base)\n> \t...\n>\n> to enforce the invariant we assume (i.e. OK sti means ours and\n> theirs are set), it would allow us to sleep better, perhaps?\n\nAs I have stated elsewhere, I am somewhat doubtful of the benefit those\n`assert()` calls give us.\n\nI wish there was a way to integrate some sort of static analysis that\nwould warn us about using uninitialized values.\n\nOf course, we would have to make sure that it does not show as many false\npositives about `struct strbuf` and `struct strvec` \"overrunning\" on their\nbuffer. This is what dominates Coverity's report, for example.\n\nFWIW I played a little with CodeQL on GitHub, but have not found time to\ncontinue on that in a long time... my current state is pushed as `codeql`\nto my fork: https://github.com/git/git/compare/master...dscho:codeql, just\nin case somebody interested wants to take this further).\n\nCiao,\nDscho\n"},{"id":"427076","messageId":"xmqqeed95gaw.fsf@gitster.g","threadId":"55866","inReplyTo":"nycvar.QRO.7.76.6.2106101046470.57@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] remote: initialize values that might not be set","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-06-11T01:41:11Z","receivedAt":"2021-06-11T01:41:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n> You will notice that there are two Trace2 calls in that conditional `abf\n> == AHEAD_BEHIND_FULL` block.\n\nYes, the calls use ours/theirs uninitialized.  Is it sensible to\nshow 0 there, or \"(unset)\" or its moral equivalent (e.g. \"-1\")?  Not\nshowing them indeed is an option, which is what you did below, and\nthat I find sensible, too.\n\n> Now, what I failed to realize when reviewing this code (and I _bet_ Stolee\n> was in the same boat when they contributed the patch) is that this version\n> of `format_tracking_info()` is different from what is in v2.32.0. It is\n> the version we have in the `microsoft/git` fork, and it has not yet made\n> it upstream. To be precise, it is this commit:\n> https://github.com/microsoft/git/commit/91209e591b0398c8334a78001a245807f7eb348a\n>\n> In light of this, it might make more sense for us to fixup! this commit\n> thusly:\n>\n> -- snip --\n> diff --git a/remote.c b/remote.c\n> index caed9cbc31b1..cfb7b6bd8d30 100644\n> --- a/remote.c\n> +++ b/remote.c\n> @@ -2110,7 +2110,7 @@ int format_tracking_info(struct branch *branch, struct strbuf *sb,\n>  \tsti = stat_tracking_info(branch, &ours, &theirs, &full_base, 0, abf);\n>  \ttrace2_data_intmax(\"tracking\", NULL, \"stat_tracking_info/ab_flags\", abf);\n>  \ttrace2_data_intmax(\"tracking\", NULL, \"stat_tracking_info/ab_result\", sti);\n> -\tif (abf == AHEAD_BEHIND_FULL) {\n> +\tif (sti >= 0 && abf == AHEAD_BEHIND_FULL) {\n>  \t    trace2_data_intmax(\"tracking\", NULL, \"stat_tracking_info/ab_ahead\", ours);\n>  \t    trace2_data_intmax(\"tracking\", NULL, \"stat_tracking_info/ab_behind\", theirs);\n>  \t}\n"},{"id":"427137","messageId":"fab42762-33a3-d74a-87ba-3245d9169df4@gmail.com","threadId":"55866","inReplyTo":"nycvar.QRO.7.76.6.2106101046470.57@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] remote: initialize values that might not be set","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-06-11T17:56:48Z","receivedAt":"2021-06-11T17:56:53Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 6/10/2021 5:24 AM, Johannes Schindelin wrote:> Now, what I failed to realize when reviewing this code (and I _bet_ Stolee\n> was in the same boat when they contributed the patch) is that this version\n> of `format_tracking_info()` is different from what is in v2.32.0. It is\n> the version we have in the `microsoft/git` fork, and it has not yet made\n> it upstream. To be precise, it is this commit:\n> https://github.com/microsoft/git/commit/91209e591b0398c8334a78001a245807f7eb348a\n\nI _did_ miss that this wasn't necessary in v2.32.0 and only exists in\nmicrosoft/git. My cherry-pick applied cleanly, but I should have been\nmore careful.\n\n> In light of this, it might make more sense for us to fixup! this commit\n> thusly:\n\nI've approved your PR in microsoft/git. That settles the real problem\nand this patch can be dropped.\n\nSorry for the noise!\n\n-Stolee\n"}]}