Subject: Re: [PATCH 31/32] commit-graph.c: fix code that could convert the result of an integer multiplication to a larger type

From: Danh Doan <[email-masked]>
Date: Thu, 7 Nov 2019 11:06:05 +0700

On 2019-11-07 12:37:00 +0900, Junio C Hamano wrote:
> Danh Doan <[email-masked]> writes:
> 
> > On 2019-11-06 11:23:00 +0900, Junio C Hamano wrote:
> >> > @@ -1454,7 +1454,7 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)
> >> >  			    num_chunks);
> >> >  		ctx->progress = start_delayed_progress(
> >> >  			progress_title.buf,
> >> > -			num_chunks * ctx->commits.nr);
> >> > +			(uint64_t)num_chunks * ctx->commits.nr);
> >> 
> >> Hmph, do we need this?  I understand that the second parameter to
> >> the callee is u64, so the caller needs to come up with u64 without
> >> overflow, but doesn't that automatically get promoted?
> >
> > Neither num_chunks nor ctx->commits.nr is promoted because both of
> > them are int. The result of `num_chunks * ctx->commits.nr' will be int
> > and will be promoted to u64 to pass to caller.
> 
> Ah, yes.  Thanks.
> 
> The commit title is about "integer multiplication", but can the same
> issue arise with addition and subtraction as well, by the way?

Yes, the same issue will arise with all binary (and ternary) arithmetic operators
(+, -, *, /, %, ^, &, |, <<, >> and ?:).

IIRC, gcc doesn't have any warning for this kind of issue.

Microsoft Visual Studio (2017+) has C26451 for this.
https://docs.microsoft.com/en-us/visualstudio/code-quality/c26451?view=vs-2017
If our friends at Microsoft could help, we can check the remaining one
in our codebase.

-- 
Danh