Re: [PATCH 10/11] bundle-uri: avoid using undefined output of `sscanf()`
- From
Junio C Hamano <gitster@pobox.com>
- Date
- May 15, 2025, 19:21 UTC
- Message-ID
- <xmqqldqxznmi.fsf@gitster.g>
- In-Reply-To
- <4dc3e2335afb42e5006ead7b9b18d33bdae7238f.1747314709.git.gitgitgadget@gmail.com>
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 25 quoted lines
> 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...
Show 7 quoted lines
> + &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; }