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

Message ID 20191104095923.116086-2-gitter.spiros@gmail.com
State New
Headers show
Series
  • Untitled series #197375
Related show

Commit Message

Elia Pinto Nov. 4, 2019, 9:59 a.m. UTC
Fix the LGTM warning fired by the rule that finds code that could convert the result of an integer
multiplication to a larger type. Since the conversion applies after the multiplication,
arithmetic overflow may still occur.

Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
---
 commit-graph.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Comments

Junio C Hamano Nov. 6, 2019, 2:23 a.m. UTC | #1
Elia Pinto <gitter.spiros@gmail.com> writes:

> Fix the LGTM warning fired by the rule that finds code that could convert the result of an integer
> multiplication to a larger type. Since the conversion applies after the multiplication,
> arithmetic overflow may still occur.

Overly long?

>  	chunk_offsets[0] = 8 + (num_chunks + 1) * GRAPH_CHUNKLOOKUP_WIDTH;
>  	chunk_offsets[1] = chunk_offsets[0] + GRAPH_FANOUT_SIZE;
> -	chunk_offsets[2] = chunk_offsets[1] + hashsz * ctx->commits.nr;
> -	chunk_offsets[3] = chunk_offsets[2] + (hashsz + 16) * ctx->commits.nr;
> +	chunk_offsets[2] = chunk_offsets[1] + (uint64_t)hashsz * ctx->commits.nr;
> +	chunk_offsets[3] = chunk_offsets[2] + ((uint64_t)hashsz + 16) * ctx->commits.nr;

chunk_offsets[] is u64[], while hashsz and is uint and
ctx->commits.nr is int.  OK.

> @@ -1426,7 +1426,7 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)
>  	}
>  	if (ctx->num_commit_graphs_after > 1) {
>  		chunk_offsets[num_chunks + 1] = chunk_offsets[num_chunks] +
> -						hashsz * (ctx->num_commit_graphs_after - 1);
> +						(uint64_t)hashsz * (ctx->num_commit_graphs_after - 1);

Likewise.

> @@ -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?

>  	}
>  	write_graph_chunk_fanout(f, ctx);
>  	write_graph_chunk_oids(f, hashsz, ctx);
Danh Doan Nov. 7, 2019, 2:23 a.m. UTC | #2
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.
Junio C Hamano Nov. 7, 2019, 3:37 a.m. UTC | #3
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?
Danh Doan Nov. 7, 2019, 4:06 a.m. UTC | #4
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.
Johannes Schindelin Nov. 7, 2019, 12:45 p.m. UTC | #5
Hi Danh,

On Thu, 7 Nov 2019, Danh Doan wrote:

> 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.

If you up-cast, maybe do the second operand so that nobody has to spend
cycles trying to remember whether the cast or the arithmetic operand
bind stronger? I.e. `num_chunks * (uint64_t)ctx->commits.nr`.

Ciao,
Dscho
Johannes Schindelin Nov. 7, 2019, 12:49 p.m. UTC | #6
Hi Danh,

On Thu, 7 Nov 2019, Danh Doan wrote:

> 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

Patch
diff mbox series

diff --git a/commit-graph.c b/commit-graph.c
index a0f868522b..0be15f1cd4 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -1415,8 +1415,8 @@  static int write_commit_graph_file(struct write_commit_graph_context *ctx)
 
 	chunk_offsets[0] = 8 + (num_chunks + 1) * GRAPH_CHUNKLOOKUP_WIDTH;
 	chunk_offsets[1] = chunk_offsets[0] + GRAPH_FANOUT_SIZE;
-	chunk_offsets[2] = chunk_offsets[1] + hashsz * ctx->commits.nr;
-	chunk_offsets[3] = chunk_offsets[2] + (hashsz + 16) * ctx->commits.nr;
+	chunk_offsets[2] = chunk_offsets[1] + (uint64_t)hashsz * ctx->commits.nr;
+	chunk_offsets[3] = chunk_offsets[2] + ((uint64_t)hashsz + 16) * ctx->commits.nr;
 
 	num_chunks = 3;
 	if (ctx->num_extra_edges) {
@@ -1426,7 +1426,7 @@  static int write_commit_graph_file(struct write_commit_graph_context *ctx)
 	}
 	if (ctx->num_commit_graphs_after > 1) {
 		chunk_offsets[num_chunks + 1] = chunk_offsets[num_chunks] +
-						hashsz * (ctx->num_commit_graphs_after - 1);
+						(uint64_t)hashsz * (ctx->num_commit_graphs_after - 1);
 		num_chunks++;
 	}
 
@@ -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);
 	}
 	write_graph_chunk_fanout(f, ctx);
 	write_graph_chunk_oids(f, hashsz, ctx);