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

From: Victoria Dye <vdye@github.com>
Date: Aug 2, 2022, 16:26 UTC
Message-ID: <c075e483-6e00-5de4-b923-af8983ba384e@github.com>
In-Reply-To: <xmqqpmhjmwu4.fsf@gitster.g>

Junio C Hamano wrote:

Show 13 quoted lines

> "Victoria Dye via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> From: Victoria Dye <vdye@github.com>
>>
>> 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.
> 
> Sane.  But shouldn't the cast be to off_t, which is what
> strbuf_humanise_bytes() takes anyway?
>

I chose 'uint64_t' to mimic 'throughput_string()' [1], but the signed 'off_t' is a better choice given its use in 'strbuf_humanise_bytes()'.

On a related note, while writing this I made the (unsubstantiated) assumption that 'off_t' would be a 64-bit int, even on 32-bit systems. Your comment prompted me to confirm that assumption; while 'off_t' is not always guaranteed to be an int64 by default [2], Git is compiled with '#define _FILE_OFFSET_BITS 64' [3] so 'off_t' is equivalent to 'off64_t'.

I'll update the casts to 'off_t' in V2. Thanks!

[1] https://lore.kernel.org/git/20171110173956.25105-4-newren@gmail.com/ [2] https://www.gnu.org/software/libc/manual/html_mono/libc.html#index-off_005ft [3] https://lore.kernel.org/git/7vr6smc1de.fsf@assigned-by-dhcp.cox.net/

Show 19 quoted lines

>>
>> Signed-off-by: Victoria Dye <vdye@github.com>
>> ---
>>  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);
>>  	strbuf_addf(out, " (mount flags 0x%lx)\n", stat.f_flag);
>>  	strbuf_release(&buf);
>>  #endif