Soundness issue: Macros should overflow-check integer parameters

[Manishearth]

Found by @veluca93

In the two _refs macros, $pre and $post are added for a bounds check, however there is nothing that checks if they overflow.

arrayref/src/lib.rs

Lines 112 to 113 in 6a0d584

	unsafe fn as_arrays<T>(a: &[T]) -> ( $( &[T; $pre], )* &[T], $( &[T; $post], )*) {
	let min_len = $( $pre + )* $( $post + )* 0;

It's pretty hard to actually trigger this since very-large arrays are not quite allowed by the compiler anyway on at least x86, but it is worth guarding against.

[droundy]

I believe I've fixed this, and even made this sum guaranteed to be computed at compile time by changing let to const. I'd appreciate a look at my commit if you have time, as it's not the most elegant solution.

[veluca93]

Since in Rust no slice can have more than isize::MAX element unless its elements are ZSTs, and the code doesn't do UB with ZSTs, I would say it is fine -- however, I think it would be better to just panic in the overflow case, instead of returning the maximum usize.

Relatedly -- why not saturating_add if you were going for saturating semantics?

[droundy]

The issue is that I'm not sure how to panic in a const function. I suppose the answer is to put a not const assert that the min len is less than the max. David Roundy

…

On Sat, Jul 20, 2024, 2:07 AM Luca Versari ***@***.***> wrote: Since in Rust no slice can have more than isize::MAX element unless its elements are ZSTs, and the code doesn't do UB with ZSTs, I would say it is fine -- however, I think it would be better to just panic in the overflow case, instead of returning the maximum usize. Relatedly -- why not saturating_add if you were going for saturating semantics? — Reply to this email directly, view it on GitHub <#26 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABBSKNS43CTIVRNYSZFDADZNISERAVCNFSM6AAAAABINVNRAKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENBRGA2TGNBVGY> . You are receiving this because you modified the open/close state.Message ID: ***@***.***>

[veluca93]

The issue is that I'm not sure how to panic in a const function. I suppose the answer is to put a not const assert that the min len is less than the max. David Roundy

You can just panic!: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=220a29c113e540296a43becdce7a4367

[droundy]

I switched to saturating add and added an explicit panic afterwards, which seems a reasonable solution. Somehow after seeing that checked_add is not yet const it didn't occur to me to look at saturating_add.

[veluca93]

This should work as far as I can tell!

[droundy]

Thanks! David Roundy

…

On Sat, Jul 20, 2024, 7:08 AM Luca Versari ***@***.***> wrote: This should work as far as I can tell! — Reply to this email directly, view it on GitHub <#26 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABBSKMYBMCAMWXCTDI2NYLZNJVOZAVCNFSM6AAAAABINVNRAKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENBRGE3DEOJTGA> . You are receiving this because you modified the open/close state.Message ID: ***@***.***>