Skip to content

Commit

Permalink
fix: parsing stack overflow (#703)
Browse files Browse the repository at this point in the history
* feat: use a custom parser input

* test: add SO tests

* fixes
  • Loading branch information
DaniPopes authored Jul 30, 2024
1 parent 6540523 commit 4790c47
Show file tree
Hide file tree
Showing 12 changed files with 215 additions and 113 deletions.
66 changes: 35 additions & 31 deletions crates/dyn-abi/src/coerce.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,11 @@ use alloy_primitives::{Address, Function, Sign, I256, U256};
use alloy_sol_types::Word;
use core::fmt;
use hex::FromHexError;
use parser::utils::{array_parser, char_parser, spanned};
use parser::{
new_input,
utils::{array_parser, char_parser, spanned},
Input,
};
use winnow::{
ascii::{alpha0, alpha1, digit1, hex_digit0, hex_digit1, space0},
combinator::{cut_err, dispatch, empty, fail, opt, preceded, trace},
Expand Down Expand Up @@ -75,7 +79,7 @@ impl DynSolType {
#[doc(alias = "tokenize")] // from ethabi
pub fn coerce_str(&self, s: &str) -> Result<DynSolValue> {
ValueParser::new(self)
.parse(s)
.parse(new_input(s))
.map_err(|e| crate::Error::TypeParser(parser::Error::parser(e)))
}
}
Expand All @@ -85,13 +89,13 @@ struct ValueParser<'a> {
list_end: Option<char>,
}

impl<'i> Parser<&'i str, DynSolValue, ContextError> for ValueParser<'_> {
fn parse_next(&mut self, input: &mut &'i str) -> PResult<DynSolValue, ContextError> {
impl<'i> Parser<Input<'i>, DynSolValue, ContextError> for ValueParser<'_> {
fn parse_next(&mut self, input: &mut Input<'i>) -> PResult<DynSolValue, ContextError> {
#[cfg(feature = "debug")]
let name = self.ty.sol_type_name();
#[cfg(not(feature = "debug"))]
let name = "value_parser";
trace(name, move |input: &mut &str| match self.ty {
trace(name, move |input: &mut Input<'i>| match self.ty {
DynSolType::Bool => bool(input).map(DynSolValue::Bool),
&DynSolType::Int(size) => {
int(size).parse_next(input).map(|int| DynSolValue::Int(int, size))
Expand Down Expand Up @@ -142,14 +146,14 @@ impl<'a> ValueParser<'a> {
}

#[inline]
fn string<'s, 'i: 's>(&'s self) -> impl Parser<&'i str, &'i str, ContextError> + 's {
trace("string", |input: &mut &'i str| {
fn string<'s, 'i: 's>(&'s self) -> impl Parser<Input<'i>, &'i str, ContextError> + 's {
trace("string", |input: &mut Input<'i>| {
let Some(delim) = input.chars().next() else {
return Ok("");
};
let has_delim = matches!(delim, '"' | '\'');
if has_delim {
*input = &input[1..];
let _ = input.next_token();
}

// TODO: escapes?
Expand Down Expand Up @@ -181,7 +185,7 @@ impl<'a> ValueParser<'a> {
}

#[inline]
fn array<'i: 'a>(self) -> impl Parser<&'i str, Vec<DynSolValue>, ContextError> + 'a {
fn array<'i: 'a>(self) -> impl Parser<Input<'i>, Vec<DynSolValue>, ContextError> + 'a {
#[cfg(feature = "debug")]
let name = format!("{}[]", self.ty);
#[cfg(not(feature = "debug"))]
Expand All @@ -193,7 +197,7 @@ impl<'a> ValueParser<'a> {
fn fixed_array<'i: 'a>(
self,
len: usize,
) -> impl Parser<&'i str, Vec<DynSolValue>, ContextError> + 'a {
) -> impl Parser<Input<'i>, Vec<DynSolValue>, ContextError> + 'a {
#[cfg(feature = "debug")]
let name = format!("{}[{len}]", self.ty);
#[cfg(not(feature = "debug"))]
Expand All @@ -215,12 +219,12 @@ impl<'a> ValueParser<'a> {
fn tuple<'i: 's, 't: 's, 's>(
&'s self,
tuple: &'t Vec<DynSolType>,
) -> impl Parser<&'i str, Vec<DynSolValue>, ContextError> + 's {
) -> impl Parser<Input<'i>, Vec<DynSolValue>, ContextError> + 's {
#[cfg(feature = "debug")]
let name = DynSolType::Tuple(tuple.clone()).to_string();
#[cfg(not(feature = "debug"))]
let name = "tuple";
trace(name, move |input: &mut &'i str| {
trace(name, move |input: &mut Input<'i>| {
space0(input)?;
char_parser('(').parse_next(input)?;

Expand Down Expand Up @@ -279,7 +283,7 @@ impl fmt::Display for Error {
}

#[inline]
fn bool(input: &mut &str) -> PResult<bool> {
fn bool(input: &mut Input<'_>) -> PResult<bool> {
trace(
"bool",
dispatch! {alpha1.context(StrContext::Label("boolean"));
Expand All @@ -293,7 +297,7 @@ fn bool(input: &mut &str) -> PResult<bool> {
}

#[inline]
fn int<'i>(size: usize) -> impl Parser<&'i str, I256, ContextError> {
fn int<'i>(size: usize) -> impl Parser<Input<'i>, I256, ContextError> {
#[cfg(feature = "debug")]
let name = format!("int{size}");
#[cfg(not(feature = "debug"))]
Expand All @@ -310,14 +314,14 @@ fn int<'i>(size: usize) -> impl Parser<&'i str, I256, ContextError> {
}

#[inline]
fn int_sign(input: &mut &str) -> PResult<Sign> {
trace("int_sign", |input: &mut &str| match input.as_bytes().first() {
fn int_sign(input: &mut Input<'_>) -> PResult<Sign> {
trace("int_sign", |input: &mut Input<'_>| match input.as_bytes().first() {
Some(b'+') => {
*input = &input[1..];
let _ = input.next_slice(1);
Ok(Sign::Positive)
}
Some(b'-') => {
*input = &input[1..];
let _ = input.next_slice(1);
Ok(Sign::Negative)
}
Some(_) | None => Ok(Sign::Positive),
Expand All @@ -326,12 +330,12 @@ fn int_sign(input: &mut &str) -> PResult<Sign> {
}

#[inline]
fn uint<'i>(len: usize) -> impl Parser<&'i str, U256, ContextError> {
fn uint<'i>(len: usize) -> impl Parser<Input<'i>, U256, ContextError> {
#[cfg(feature = "debug")]
let name = format!("uint{len}");
#[cfg(not(feature = "debug"))]
let name = "uint";
trace(name, move |input: &mut &str| {
trace(name, move |input: &mut Input<'_>| {
let (s, (intpart, fract)) = spanned((
prefixed_int,
opt(preceded(
Expand Down Expand Up @@ -393,12 +397,12 @@ fn uint<'i>(len: usize) -> impl Parser<&'i str, U256, ContextError> {
}

#[inline]
fn prefixed_int<'i>(input: &mut &'i str) -> PResult<&'i str> {
trace("prefixed_int", |input: &mut &'i str| {
fn prefixed_int<'i>(input: &mut Input<'i>) -> PResult<&'i str> {
trace("prefixed_int", |input: &mut Input<'i>| {
let has_prefix = matches!(input.get(..2), Some("0b" | "0B" | "0o" | "0O" | "0x" | "0X"));
let checkpoint = input.checkpoint();
if has_prefix {
*input = &input[2..];
let _ = input.next_slice(2);
// parse hex since it's the most general
hex_digit1(input)
} else {
Expand All @@ -416,7 +420,7 @@ fn prefixed_int<'i>(input: &mut &'i str) -> PResult<&'i str> {
}

#[inline]
fn int_units(input: &mut &str) -> PResult<usize> {
fn int_units(input: &mut Input<'_>) -> PResult<usize> {
trace(
"int_units",
dispatch! {alpha0;
Expand All @@ -430,12 +434,12 @@ fn int_units(input: &mut &str) -> PResult<usize> {
}

#[inline]
fn fixed_bytes<'i>(len: usize) -> impl Parser<&'i str, Word, ContextError> {
fn fixed_bytes<'i>(len: usize) -> impl Parser<Input<'i>, Word, ContextError> {
#[cfg(feature = "debug")]
let name = format!("bytes{len}");
#[cfg(not(feature = "debug"))]
let name = "bytesN";
trace(name, move |input: &mut &str| {
trace(name, move |input: &mut Input<'_>| {
if len > Word::len_bytes() {
return Err(ErrMode::from_external_error(
input,
Expand All @@ -455,23 +459,23 @@ fn fixed_bytes<'i>(len: usize) -> impl Parser<&'i str, Word, ContextError> {
}

#[inline]
fn address(input: &mut &str) -> PResult<Address> {
fn address(input: &mut Input<'_>) -> PResult<Address> {
trace("address", hex_str.try_map(hex::FromHex::from_hex)).parse_next(input)
}

#[inline]
fn function(input: &mut &str) -> PResult<Function> {
fn function(input: &mut Input<'_>) -> PResult<Function> {
trace("function", hex_str.try_map(hex::FromHex::from_hex)).parse_next(input)
}

#[inline]
fn bytes(input: &mut &str) -> PResult<Vec<u8>> {
fn bytes(input: &mut Input<'_>) -> PResult<Vec<u8>> {
trace("bytes", hex_str.try_map(hex::decode)).parse_next(input)
}

#[inline]
fn hex_str<'i>(input: &mut &'i str) -> PResult<&'i str> {
trace("hex_str", |input: &mut &'i str| {
fn hex_str<'i>(input: &mut Input<'i>) -> PResult<&'i str> {
trace("hex_str", |input: &mut Input<'i>| {
// Allow empty `bytes` only with a prefix.
let has_prefix = opt("0x").parse_next(input)?.is_some();
let s = hex_digit0(input)?;
Expand Down
11 changes: 9 additions & 2 deletions crates/json-abi/src/item.rs
Original file line number Diff line number Diff line change
Expand Up @@ -155,9 +155,9 @@ impl AbiItem<'_> {
/// );
/// ```
pub fn parse(mut input: &str) -> parser::Result<Self> {
// need this for Constructor, since the keyword is also the name of the function
// Need this copy for Constructor, since the keyword is also the name of the function.
let copy = input;
match parser::utils::parse_item(&mut input)? {
match parser::utils::parse_item_keyword(&mut input)? {
"constructor" => Constructor::parse(copy).map(Into::into),
"function" => Function::parse(input).map(Into::into),
"error" => Error::parse(input).map(Into::into),
Expand Down Expand Up @@ -688,4 +688,11 @@ mod tests {
})
);
}

// https://github.com/alloy-rs/core/issues/702
#[test]
fn parse_stack_overflow() {
let s = "error J((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((";
AbiItem::parse(s).unwrap_err();
}
}
13 changes: 8 additions & 5 deletions crates/primitives/src/utils/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,10 @@
use crate::B256;
use alloc::{boxed::Box, collections::TryReserveError, vec::Vec};
use cfg_if::cfg_if;
use core::{fmt, mem::MaybeUninit};
use core::{
fmt,
mem::{ManuallyDrop, MaybeUninit},
};

mod units;
pub use units::{
Expand All @@ -29,7 +32,7 @@ pub type Units = Unit;
/// The prefix used for hashing messages according to EIP-191.
pub const EIP191_PREFIX: &str = "\x19Ethereum Signed Message:\n";

/// Tries to create a `Vec` of `n` elements, each initialized to `elem`.
/// Tries to create a [`Vec`] containing the arguments.
#[macro_export]
macro_rules! try_vec {
() => {
Expand Down Expand Up @@ -75,10 +78,10 @@ pub fn box_try_new_uninit<T>() -> Result<Box<MaybeUninit<T>>, TryReserveError> {
// Make sure we got exactly 1 element.
vec.shrink_to(1);

let ptr = vec.as_mut_ptr();
core::mem::forget(vec);
let mut vec = ManuallyDrop::new(vec);

// SAFETY: `vec` is exactly one element long and has not been deallocated.
Ok(unsafe { Box::from_raw(ptr) })
Ok(unsafe { Box::from_raw(vec.as_mut_ptr()) })
}

/// Tries to collect the elements of an iterator into a `Vec`.
Expand Down
18 changes: 11 additions & 7 deletions crates/sol-type-parser/src/ident.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use winnow::{
error::{ErrMode, ErrorKind, ParserError},
stream::{AsBStr, Stream},
PResult,
};

Expand Down Expand Up @@ -54,22 +55,25 @@ pub const fn is_valid_identifier(s: &str) -> bool {
/// Parses a Solidity identifier.
#[inline]
pub fn identifier<'a>(input: &mut &'a str) -> PResult<&'a str> {
identifier_parser(input)
}

#[inline]
pub(crate) fn identifier_parser<'a, I>(input: &mut I) -> PResult<&'a str>
where
I: Stream<Slice = &'a str> + AsBStr,
{
// See note in `is_valid_identifier` above.
// Use the faster `slice::Iter` instead of `str::Chars`.
let mut chars = input.as_bytes().iter().map(|b| *b as char);
let mut chars = input.as_bstr().iter().map(|b| *b as char);

let Some(true) = chars.next().map(is_id_start) else {
return Err(ErrMode::from_error_kind(input, ErrorKind::Fail));
};

// 1 for the first character, we know it's ASCII
let len = 1 + chars.take_while(|c| is_id_continue(*c)).count();
// SAFETY: Only ASCII characters are valid in identifiers
unsafe {
let ident = input.get_unchecked(..len);
*input = input.get_unchecked(len..);
Ok(ident)
}
Ok(input.next_slice(len))
}

#[cfg(test)]
Expand Down
76 changes: 76 additions & 0 deletions crates/sol-type-parser/src/input.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
#![allow(missing_docs, missing_copy_implementations, missing_debug_implementations)]

// Recursion implementation modified from `toml`: https://github.com/toml-rs/toml/blob/a02cbf46cab4a8683e641efdba648a31498f7342/crates/toml_edit/src/parser/mod.rs#L99

use core::fmt;
use winnow::{
error::{ContextError, FromExternalError},
Parser,
};

#[allow(dead_code)]
#[derive(Clone, Debug, PartialEq, Eq)]
enum CustomError {
RecursionLimitExceeded,
}

impl fmt::Display for CustomError {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
match self {
Self::RecursionLimitExceeded => f.write_str("recursion limit exceeded"),
}
}
}

#[cfg(feature = "std")]
impl std::error::Error for CustomError {}

pub type Input<'a> = winnow::Stateful<&'a str, RecursionCheck>;

pub fn new_input(input: &str) -> Input<'_> {
winnow::Stateful { input, state: Default::default() }
}

pub fn check_recursion<'a, O>(
mut parser: impl Parser<Input<'a>, O, ContextError>,
) -> impl Parser<Input<'a>, O, ContextError> {
move |input: &mut Input<'a>| {
input.state.enter().map_err(|err| {
winnow::error::ErrMode::from_external_error(input, winnow::error::ErrorKind::Eof, err)
.cut()
})?;
let result = parser.parse_next(input);
input.state.exit();
result
}
}

#[derive(Clone, Debug, Default, PartialEq, Eq)]
pub struct RecursionCheck {
current: usize,
}

const LIMIT: usize = 80;

impl RecursionCheck {
#[cfg(any())]
fn check_depth(_depth: usize) -> Result<(), CustomError> {
if LIMIT <= _depth {
return Err(CustomError::RecursionLimitExceeded);
}

Ok(())
}

fn enter(&mut self) -> Result<(), CustomError> {
self.current += 1;
if LIMIT <= self.current {
return Err(CustomError::RecursionLimitExceeded);
}
Ok(())
}

fn exit(&mut self) {
self.current -= 1;
}
}
Loading

0 comments on commit 4790c47

Please sign in to comment.