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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Nov 7, 2019, 12:49 UTC
Message-ID: <nycvar.QRO.7.76.6.1911071345440.46@tvgsbejvaqbjf.bet>
In-Reply-To: <20191107040605.GA8096@danh.dev>

Hi Danh,

On Thu, 7 Nov 2019, Danh Doan wrote:

Show 33 quoted lines

> On 2019-11-07 12:37:00 +0900, Junio C Hamano wrote:
> > 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.

I am a bit busy right now, but it _was_ my hope that adding a Visual Studio job to our Azure Pipeline would enable everybody to perform tests like this one.

In other words, I _think_ that you can add something like

	#pragma warning(enable: 26451)

to `compat/msvc.h` and then open a PR at https://github.com/git/git, the Azure Pipeline should produce precisely what you want.

(If I were you, I would also try to save some CO2 by ripping out all jobs except the `vs_build` one from `azure-pipelines.yml`.)

Ciao, Dscho