Change numLocalBytes to u32
#500
Conversation
No, sorry, I just started adding them for future use as I was fixing security issues. Agreed it would be good to convert them to |
| @@ -71,7 +71,7 @@ typedef struct M3Function | |||
| u16 numRetAndArgSlots; | |||
|
|
|||
| u16 numLocals; // not including args | |||
| u16 numLocalBytes; | |||
| u32 numLocalBytes; | |||
There was a problem hiding this comment.
There are many more u16 fields here that are counting slots. A slot is 4 (or 8) bytes, so only increasing this variable isn't going to help much in the general case.
Line 2893 in 35b5e2f
There was a problem hiding this comment.
This PR addresses the issue caused by this line. I don't think we need to change the slot count to u32, but I'm not sure which other variable types we might need to adjust. It makes more sense to me to add -fsanitize=integer in the CI.
There was a problem hiding this comment.
If you use 4 times as many locals as you are using now, you'll wrap around the maxStackSlots slot counter. So just changing this line only moves the problem by a factor of 4, not 2^16. Is that good enough?
It makes more sense to me to add -fsanitize=integer in the CI.
Fixing the overflows (by returning error) and enabling UBSan does sound nice.
There was a problem hiding this comment.
If you use 4 times as many locals as you are using now, you'll wrap around the
maxStackSlotsslot counter. So just changing this line only moves the problem by a factor of 4, not 2^16. Is that good enough?
I can verify that when I set d_m3MaxFunctionStackHeight to 32767, it first encounters m3Err_functionStackOverflow in AllocateSlotsWithinRange.
|
I made #504 for running regression tests. It's unfortunately a large change, splitting run-spec-test.py so the JSON parsing is separate from downloading the spec tests. |
689b26a
to
4472181
Compare
When we have 8192 `i64` locals, the required bytes are 65536.
4472181
to
9172d69
Compare
When we have 8192
i64locals, the required bytes are 65536.IIUC, the test cases in the
test/regressiondirectory are not actually being tested.