From: Phillip Wood <phillip.wood123@gmail.com>
Date: Fri, 16 May 2025 10:11:59 GMT
Subject: Re: [PATCH 10/11] bundle-uri: avoid using undefined output of `sscanf()`
Message-ID: <694d8e62-486b-43d4-966c-4fa1e5a95061@crinan.ddns.net>
In-Reply-To: <20250515202502.GK3320240@coredump.intra.peff.net>

On 15/05/2025 21:25, Jeff King wrote:
> On Thu, May 15, 2025 at 01:11:48PM +0000, Johannes Schindelin via GitGitGadget wrote:
> 
>> 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.
> 
> Definitely an issue, but...why are we using sscanf() at all?
> 
> Wouldn't strtoul() be the usual thing in our code base? Or even just
> repo_config_get_ulong()? The behavior of the latter would differ in that
> we'd complain about a garbage value in fetch.bundlecreationtoken, but
> wouldn't that be a good thing?

I had a similar thought, though to make sure that we parsed 64 bit 
values correctly on windows so we'd need something based on strtoumax() 
I think. There is another call to sscanf() in this file which the 
analyzer does not complain about because it stores the result in a local 
variable that is not used if the call to sscanf() fails. We should stop 
using sscanf() there as well. I wonder if we should add something about 
not using sscanf() to our coding guidelines. Apart from this file the 
only other use of sscanf() is in a test helper which doesn't seem so bad 
though if we removed that we could add sscanf() to banned.h.

Best Wishes

Phillip