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 <congdanhqx@gmail.com>
Date: Nov 7, 2019, 04:06 UTC
Message-ID: <20191107040605.GA8096@danh.dev>
In-Reply-To: <xmqqtv7gs6g3.fsf@gitster-ct.c.googlers.com>

On 2019-11-07 12:37:00 +0900, Junio C Hamano wrote:

Show 22 quoted lines

> Danh Doan <congdanhqx@gmail.com> 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