Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: panic on overflow in release mode #5150

Merged

Conversation

stringhandler
Copy link
Collaborator

Description

Add a toml config to do the same overflow/underflow check in debug mode in release mode.

Motivation and Context

By default, Rust will wrap an integer in release mode instead of throwing the overflow error seen in debug mode. Panicking at this time is better than silently using the wrong value. IMO this could lead to all sorts of exploits.

Some Tari specific background:
In the MMR crate there is this check:

       let peaks = find_peaks(mmr_size);
        let mut peak_hashes = Vec::with_capacity(peaks.len() - 1);

We found in testing that on some machines this would panic and others would pass. (I can't remember if it was this exact line, since this should fail with allocating that much memory)

How Has This Been Tested?

Checked on rust pen to confirm that rust underflows in release mode with this code

fn main() {
   let vec: Vec<u8> = Vec::new();
   let val =  vec.len() - 1;
   println!("{}", val);
}

@stringhandler stringhandler added the P-controversial Process - This PR or Issue is controversial and/or requires more attention that simpler issues label Jan 30, 2023
Copy link
Contributor

@hansieodendaal hansieodendaal left a comment

Choose a reason for hiding this comment

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

LGTM - totally agree

@hansieodendaal
Copy link
Contributor

utACK

sdbondi pushed a commit to tari-project/tari-dan that referenced this pull request Jan 31, 2023
@stringhandler stringhandler merged commit 5f5808b into tari-project:development Jan 31, 2023
@stringhandler stringhandler deleted the st-panic-on-overflow branch January 31, 2023 08:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P-controversial Process - This PR or Issue is controversial and/or requires more attention that simpler issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants