Skip to content

Conversation

P-E-P
Copy link
Member

@P-E-P P-E-P commented Sep 3, 2025

When the byte size required for an array overflow we should emit an error.

Fixes #3962

@P-E-P P-E-P force-pushed the huge_array_allocation branch from 8dbcf21 to 455cf4c Compare September 3, 2025 17:17
@P-E-P P-E-P marked this pull request as ready for review September 3, 2025 17:19
@P-E-P P-E-P requested a review from philberty September 3, 2025 17:20
@P-E-P P-E-P force-pushed the huge_array_allocation branch 2 times, most recently from f860507 to ad2b82c Compare September 3, 2025 17:34
Copy link
Member

@philberty philberty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice one didnt know you can do that

@philberty philberty force-pushed the huge_array_allocation branch from ad2b82c to c9af76e Compare September 3, 2025 19:23
= wi::ext (max - min + 1, precision, sign).to_uhwi ();

unsigned int res;
if (__builtin_umul_overflow (TREE_INT_CST_ELT (TYPE_SIZE_UNIT (array_type),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like there is a regression here on running on 32 bit platforms. So this __builtin wont work.

Its definetlyy worth checking this in C/C++ on compiler explorer and if they make the same error and work back from that error message.

@philberty
Copy link
Member

I think your Fix is partly right there are two parts to fix this properly.

  1. Overflow check the value
  2. Add a MAX alloc limit of 2gb i think is what rustc does from reading it ages ago.

So 1,

I think you need to add a check like you have or you could put it into typechecking over in

But maybe its enough to leave it wher you have it.

You should be able to do:

if (TREE_OVERFLOW_P (capacity_expr)) 
{
}

We track the capacity expr as part of the Array type now.

PArt 2: detect the HUGE array


tree len = capacity_expr
tree esize = TYPE_SIZE_UNIT (elt_type);     // bytes, INTEGER_CST
tree bytes = fold_build2 (MULT_EXPR, sizetype,
                          fold_convert (sizetype, len),
                          fold_convert (sizetype, esize));

if (TREE_OVERFLOW_P (bytes)) 

Or also put in a new constant for 2gb maybe make it a new option in lang.opts so it can be changed but it defaults to 2gb as the final check.

I think should do this.

Copy link
Member

@philberty philberty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added some suggestions to try in the comments

@philberty
Copy link
Member

Also debug_tree is super useful here

When the byte size required for an array overflow we should emit an
error.

gcc/rust/ChangeLog:

	* backend/rust-compile-expr.cc (CompileExpr::array_copied_expr): Check
	for overflow on array memory size and emit an error.

gcc/testsuite/ChangeLog:

	* rust/compile/issue-3962.rs: New test.

Signed-off-by: Pierre-Emmanuel Patry <[email protected]>
@P-E-P P-E-P force-pushed the huge_array_allocation branch from c9af76e to 573c26a Compare September 4, 2025 09:42
@philberty
Copy link
Member

I think this is a good sign your getting:

Executing on host: /home/runner/work/gccrs/gccrs/gccrs-build/gcc/testsuite/rust/../../gccrs -B/home/runner/work/gccrs/gccrs/gccrs-build/gcc/testsuite/rust/../../  /home/runner/work/gccrs/gccrs/gcc/testsuite/rust/compile/issue-3962.rs  -m32  -fdiagnostics-plain-output   -frust-incomplete-and-experimental-compiler-do-not-use   -S -o issue-3962.s    (timeout = 300)
spawn -ignore SIGHUP /home/runner/work/gccrs/gccrs/gccrs-build/gcc/testsuite/rust/../../gccrs -B/home/runner/work/gccrs/gccrs/gccrs-build/gcc/testsuite/rust/../../ /home/runner/work/gccrs/gccrs/gcc/testsuite/rust/compile/issue-3962.rs -m32 -fdiagnostics-plain-output -frust-incomplete-and-experimental-compiler-do-not-use -S -o issue-3962.s
/home/runner/work/gccrs/gccrs/gcc/testsuite/rust/compile/issue-3962.rs:2:19: error: left shift count >= width of type
compiler exited with status 1
FAIL: rust/compile/issue-3962.rs  at line 3 (test for errors, line 2)
FAIL: rust/compile/issue-3962.rs (test for excess errors)
Excess errors:
/home/runner/work/gccrs/gccrs/gcc/testsuite/rust/compile/issue-3962.rs:2:19: error: left shift count >= width of type

@philberty
Copy link
Member

not100% sure whats the best way to add tests for errors on m32 vs m64 here @dkm might know better

@P-E-P
Copy link
Member Author

P-E-P commented Sep 4, 2025

2. Add a MAX alloc limit of 2gb i think is what rustc does from reading it ages ago.

Or also put in a new constant for 2gb maybe make it a new option in lang.opts so it can be changed but it defaults to 2gb as the final check.

Are you sure about that ?

https://godbolt.org/z/Mr684qqsT

rustc does not throw an error message when the allocation is too big.

EDIT: It does since rustc 1.55 although I can't find anything about a 2Gb limit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Todo
Development

Successfully merging this pull request may close these issues.

crash with shifts inside array length
2 participants