Subject: Re: [PATCH 3/7] builtin/bugreport.c: avoid size_t overflow

From: Victoria Dye <[email-masked]>
Date: Tue, 2 Aug 2022 09:26:42 -0700

Ævar Arnfjörð Bjarmason wrote:
> 
> On Mon, Aug 01 2022, Victoria Dye via GitGitGadget wrote:
> 
>> From: Victoria Dye <[email-masked]>
>>
>> Avoid size_t overflow when reporting the available disk space in
>> 'get_disk_info' by casting the block size and available block count to
>> 'uint64_t' before multiplying them. Without this change, 'st_mult' would
>> (correctly) report size_t overflow on 32-bit systems at or exceeding 2^32
>> bytes of available space.
>>
>> Signed-off-by: Victoria Dye <[email-masked]>
>> ---
>>  builtin/bugreport.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/builtin/bugreport.c b/builtin/bugreport.c
>> index 35b1fc48bf1..720889a37ad 100644
>> --- a/builtin/bugreport.c
>> +++ b/builtin/bugreport.c
>> @@ -258,7 +258,7 @@ static int get_disk_info(struct strbuf *out)
>>  	}
>>  
>>  	strbuf_addf(out, "Available space on '%s': ", buf.buf);
>> -	strbuf_humanise_bytes(out, st_mult(stat.f_bsize, stat.f_bavail));
>> +	strbuf_humanise_bytes(out, (uint64_t)stat.f_bsize * (uint64_t)stat.f_bavail);
> 
> Doesn't this remove the overflow guard on 64 bit systems to support
> those 32 bit systems?
> 

It does, but the total disk space available on a system should be able to
fit into a 64-bit integer. I considered adding an explicit
'unsigned_mult_overflows', but decided against it because it's almost
certainly overkill for such an implausible edge case.

> I also don't tthink it's correct that this would "correctly
> report...". Before this we were simply assuming that "size_t" and
> "unsigned long" & "fsblkcnt_t" would all yield the same thing.
> 

The point I was making is that, if your 'size_t' is 32 bits, but you have
more than ~4GB of disk space available on your system, the result of the
multiplication will overflow 'size_t'. So, 'st_mult' failing because it
detects an overflow is "correct", rather than e.g. a false positive.

> But I don't think per [1] and [2] that POSIX is giving us any guarantees
> in that regard, even on 32 bit systems, but perhaps it's a reasonable
> assumption in practice.
> 
> 1. https://pubs.opengroup.org/onlinepubs/009695399/basedefs/sys/statvfs.h.html
> 2. https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/sys_types.h.html