Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Untrusted RLP length overflow check #6227

Merged
merged 2 commits into from
Aug 3, 2017
Merged

Untrusted RLP length overflow check #6227

merged 2 commits into from
Aug 3, 2017

Conversation

NikVolf
Copy link
Contributor

@NikVolf NikVolf commented Aug 3, 2017

No description provided.

@rphmeier rphmeier added A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. M4-core ⛓ Core client code / Rust. labels Aug 3, 2017
@@ -371,7 +371,8 @@ impl<'a> BasicDecoder<'a> {
}
let len = decode_usize(&bytes[1..begin_of_value])?;

let last_index_of_value = begin_of_value + len;
let last_index_of_value = begin_of_value.overflowing_add(len)
Copy link
Contributor

Choose a reason for hiding this comment

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

does this compile? i thought overflowing_add returns (usize, bool). checked_add returns an Option, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i'm not putting pleasereview for a reason here :)
i have some issues with cargo here

@@ -385,7 +386,7 @@ impl<'a> BasicDecoder<'a> {

#[cfg(test)]
mod tests {
use UntrustedRlp;
use {UntrustedRlp, DecodeError};
Copy link
Contributor

Choose a reason for hiding this comment

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

DecoderError

@rphmeier rphmeier added A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. and removed A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. labels Aug 3, 2017
@NikVolf NikVolf added A0-pleasereview 🤓 Pull request needs code review. and removed A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. labels Aug 3, 2017
@NikVolf
Copy link
Contributor Author

NikVolf commented Aug 3, 2017

Fixes #6226

@rphmeier
Copy link
Contributor

rphmeier commented Aug 3, 2017

LGTM

@rphmeier rphmeier added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Aug 3, 2017
@rphmeier rphmeier merged commit ae9f356 into master Aug 3, 2017
@rphmeier rphmeier deleted the rlp-check branch August 3, 2017 19:20
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants