From: Junio C Hamano <gitster@pobox•com>
To: "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail•com>
Cc: git@vger•kernel.org, Johannes Schindelin <johannes.schindelin@gmx•de>
Subject: Re: [PATCH 10/11] bundle-uri: avoid using undefined output of `sscanf()`
Date: Thu, 15 May 2025 12:21:09 -0700 [thread overview]
Message-ID: <xmqqldqxznmi.fsf@gitster.g> (raw)
In-Reply-To: <4dc3e2335afb42e5006ead7b9b18d33bdae7238f.1747314709.git.gitgitgadget@gmail.com> (Johannes Schindelin via GitGitGadget's message of "Thu, 15 May 2025 13:11:48 +0000")
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail•com>
writes:
> From: Johannes Schindelin <johannes.schindelin@gmx•de>
>
> In c429bed102 (bundle-uri: store fetch.bundleCreationToken, 2023-01-31)
> code was introduced that assumes that an `sscanf()` call leaves its
> output variables unchanged unless the return value indicates success.
>
> However, the POSIX documentation makes no such guarantee:
> https://pubs.opengroup.org/onlinepubs/9699919799/functions/sscanf.html
>
> So let's make sure that the output variable `maxCreationToken` is
> always well-defined.
>
> diff --git a/bundle-uri.c b/bundle-uri.c
> index 96d2ba726d99..13a42f92387e 100644
> --- a/bundle-uri.c
> +++ b/bundle-uri.c
> @@ -532,11 +532,13 @@ static int fetch_bundles_by_token(struct repository *r,
> */
> if (!repo_config_get_value(r,
> "fetch.bundlecreationtoken",
> - &creationTokenStr) &&
> - sscanf(creationTokenStr, "%"PRIu64, &maxCreationToken) == 1 &&
> - bundles.items[0]->creationToken <= maxCreationToken) {
> - free(bundles.items);
> - return 0;
The original said "if we successfully parsed and the value of the
token is larger than the token, we are done", which is probably OK,
but the problem is if we were fed garbage and failed to parse it, we
would have smudged maxCreationToken to some unknown value, and the
code path that follows here, which assumes that maxCreationToken is
left as initialized to 0 will be broken.
So the problem is real, but I find the rewritten form a bit hard to
follow. Namely, when sscanf() failed to grab maxCreationToken, we
never compared it with bundles.items[0]->creationToken, which makes
perfect sense to me, but now...
> + &creationTokenStr)) {
> + if (sscanf(creationTokenStr, "%"PRIu64, &maxCreationToken) != 1)
> + maxCreationToken = 0;
> + if (bundles.items[0]->creationToken <= maxCreationToken) {
> + free(bundles.items);
> + return 0;
> + }
... the updated code does use the just-assigned-because-we-failed-to-parse
value 0 in comparison.
I have to wonder if the attached patch is simpler to reason about,
more in line with what the original wanted to do, and more correct?
When we fail to grab the configuration, or when the value we grabbed
from the configuration does not parse, then we reset
maxCreationToken to 0, but otherwise we have a valid
maxCreationToken so use it to see if we should return early.
The cases we do not return early here are either (1) we did not have
usable configured value, in which case maxCreationToken is set to 0
before reaching the loop after this code, or (2) the value of
maxCreationToken we grabbed from the configuration is smaller than
the creationToken in the bundle list, in which case that value is
used when entering the loop.
Thanks.
bundle-uri.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git c/bundle-uri.c w/bundle-uri.c
index f3579e228e..13a43f8e32 100644
--- c/bundle-uri.c
+++ w/bundle-uri.c
@@ -531,11 +531,11 @@ static int fetch_bundles_by_token(struct repository *r,
* is not strictly smaller than the maximum creation token in the
* bundle list, then do not download any bundles.
*/
- if (!repo_config_get_value(r,
- "fetch.bundlecreationtoken",
- &creationTokenStr) &&
- sscanf(creationTokenStr, "%"PRIu64, &maxCreationToken) == 1 &&
- bundles.items[0]->creationToken <= maxCreationToken) {
+ if (repo_config_get_value(r, "fetch.bundlecreationtoken",
+ &creationTokenStr) ||
+ sscanf(creationTokenStr, "%"PRIu64, &maxCreationToken) != 1)
+ maxCreationToken = 0;
+ else if (bundles.items[0]->creationToken <= maxCreationToken) {
free(bundles.items);
return 0;
}
next prev parent reply other threads:[~2025-05-15 19:21 UTC|newest]
Thread overview: 36+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-05-15 13:11 [PATCH 00/11] CodeQL-inspired fixes Johannes Schindelin via GitGitGadget
2025-05-15 13:11 ` [PATCH 01/11] commit: simplify code Johannes Schindelin via GitGitGadget
2025-05-15 19:48 ` Jeff King
2025-05-15 20:37 ` Junio C Hamano
2025-05-15 20:49 ` Jeff King
2025-05-15 13:11 ` [PATCH 02/11] fetch: carefully clear local variable's address after use Johannes Schindelin via GitGitGadget
2025-05-15 19:40 ` Jeff King
2025-05-15 13:11 ` [PATCH 03/11] commit-graph: avoid malloc'ing a local variable Johannes Schindelin via GitGitGadget
2025-05-15 19:54 ` Jeff King
2025-05-15 21:40 ` Junio C Hamano
2025-05-15 13:11 ` [PATCH 04/11] upload-pack: rename `enum` to reflect the operation Johannes Schindelin via GitGitGadget
2025-05-15 19:55 ` Jeff King
2025-05-15 13:11 ` [PATCH 05/11] has_dir_name(): make code more obvious Johannes Schindelin via GitGitGadget
2025-05-15 20:04 ` Jeff King
2025-05-15 13:11 ` [PATCH 06/11] fetch: avoid unnecessary work when there is no current branch Johannes Schindelin via GitGitGadget
2025-05-15 20:11 ` Jeff King
2025-05-15 13:11 ` [PATCH 07/11] Avoid redundant conditions Johannes Schindelin via GitGitGadget
2025-05-15 20:13 ` Jeff King
2025-05-15 13:11 ` [PATCH 08/11] trace2: avoid "futile conditional" Johannes Schindelin via GitGitGadget
2025-05-15 20:16 ` Jeff King
2025-05-15 13:11 ` [PATCH 09/11] commit-graph: avoid using stale stack addresses Johannes Schindelin via GitGitGadget
2025-05-15 20:19 ` Jeff King
2025-05-15 13:11 ` [PATCH 10/11] bundle-uri: avoid using undefined output of `sscanf()` Johannes Schindelin via GitGitGadget
2025-05-15 19:21 ` Junio C Hamano [this message]
2025-05-15 20:25 ` Jeff King
2025-05-16 10:11 ` Phillip Wood
2025-05-16 13:40 ` Phillip Wood
2025-05-16 15:42 ` Jeff King
2025-05-19 9:03 ` Phillip Wood
2025-05-22 6:03 ` Jeff King
2025-05-15 13:11 ` [PATCH 11/11] sequencer: stop pretending that an assignment is a condition Johannes Schindelin via GitGitGadget
2025-05-15 18:51 ` Junio C Hamano
2025-05-15 20:26 ` Jeff King
2025-05-16 10:13 ` Phillip Wood
2025-05-15 20:26 ` [PATCH 00/11] CodeQL-inspired fixes Jeff King
2025-05-15 20:58 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqldqxznmi.fsf@gitster.g \
--to=gitster@pobox$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=gitgitgadget@gmail$(echo .)com \
--cc=johannes.schindelin@gmx$(echo .)de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox