From 7328ffb0340a7f092b251f345be1c6339dcb7431 Mon Sep 17 00:00:00 2001 From: Tamo Date: Thu, 4 Nov 2021 16:20:53 +0100 Subject: [PATCH] stop panicking in case of internal error --- filter_parser/fuzz/fuzz_targets/parse.rs | 11 ++++++++--- filter_parser/src/error.rs | 7 ++++++- filter_parser/src/lib.rs | 12 ++++++------ filter_parser/src/value.rs | 10 ++++++---- 4 files changed, 26 insertions(+), 14 deletions(-) diff --git a/filter_parser/fuzz/fuzz_targets/parse.rs b/filter_parser/fuzz/fuzz_targets/parse.rs index 99d4a03a6..6d0069c15 100644 --- a/filter_parser/fuzz/fuzz_targets/parse.rs +++ b/filter_parser/fuzz/fuzz_targets/parse.rs @@ -1,13 +1,18 @@ #![no_main] -use filter_parser::FilterCondition; +use filter_parser::{ErrorKind, FilterCondition}; use libfuzzer_sys::fuzz_target; fuzz_target!(|data: &[u8]| { if let Ok(s) = std::str::from_utf8(data) { - // When we are fuzzing the parser we can get stack overflow really easily. + // When we are fuzzing the parser we can get a stack overflow very easily. // But since this doesn't happens with a normal build we are just going to limit the fuzzer to 500 characters. if s.len() < 500 { - let _ = FilterCondition::parse(s); + match FilterCondition::parse(s) { + Err(e) if matches!(e.kind(), ErrorKind::InternalError(_)) => { + panic!("Found an internal error: `{:?}`", e) + } + _ => (), + } } } }); diff --git a/filter_parser/src/error.rs b/filter_parser/src/error.rs index fbfbbe30b..a0ea2efac 100644 --- a/filter_parser/src/error.rs +++ b/filter_parser/src/error.rs @@ -63,9 +63,14 @@ pub enum ErrorKind<'a> { } impl<'a> Error<'a> { - pub fn kind(context: Span<'a>, kind: ErrorKind<'a>) -> Self { + pub fn kind(&self) -> &ErrorKind<'a> { + &self.kind + } + + pub fn new_from_kind(context: Span<'a>, kind: ErrorKind<'a>) -> Self { Self { context, kind } } + pub fn char(self) -> char { match self.kind { ErrorKind::Char(c) => c, diff --git a/filter_parser/src/lib.rs b/filter_parser/src/lib.rs index e6f8a75d1..9335ef185 100644 --- a/filter_parser/src/lib.rs +++ b/filter_parser/src/lib.rs @@ -162,12 +162,12 @@ fn parse_geo_radius(input: Span) -> IResult { // if we were able to parse `_geoRadius` and can't parse the rest of the input we returns a failure cut(delimited(char('('), separated_list1(tag(","), ws(|c| recognize_float(c))), char(')'))), )(input) - .map_err(|e| e.map(|_| Error::kind(input, ErrorKind::Geo))); + .map_err(|e| e.map(|_| Error::new_from_kind(input, ErrorKind::Geo))); let (input, args) = parsed?; if args.len() != 3 { - return Err(nom::Err::Failure(Error::kind(input, ErrorKind::Geo))); + return Err(nom::Err::Failure(Error::new_from_kind(input, ErrorKind::Geo))); } let res = FilterCondition::GeoLowerThan { @@ -186,9 +186,9 @@ fn parse_geo_point(input: Span) -> IResult { // if we were able to parse `_geoPoint` we are going to return a Failure whatever happens next. cut(delimited(char('('), separated_list1(tag(","), ws(|c| recognize_float(c))), char(')'))), ))(input) - .map_err(|e| e.map(|_| Error::kind(input, ErrorKind::ReservedGeo("_geoPoint"))))?; + .map_err(|e| e.map(|_| Error::new_from_kind(input, ErrorKind::ReservedGeo("_geoPoint"))))?; // if we succeeded we still returns a Failure because geoPoints are not allowed - Err(nom::Err::Failure(Error::kind(input, ErrorKind::ReservedGeo("_geoPoint")))) + Err(nom::Err::Failure(Error::new_from_kind(input, ErrorKind::ReservedGeo("_geoPoint")))) } /// primary = (WS* ~ "(" expression ")" ~ WS*) | geoRadius | condition | to @@ -199,7 +199,7 @@ fn parse_primary(input: Span) -> IResult { ws(char('(')), cut(parse_expression), cut_with_err(ws(char(')')), |c| { - Error::kind(input, ErrorKind::MissingClosingDelimiter(c.char())) + Error::new_from_kind(input, ErrorKind::MissingClosingDelimiter(c.char())) }), ), |c| parse_geo_radius(c), @@ -209,7 +209,7 @@ fn parse_primary(input: Span) -> IResult { |c| parse_geo_point(c), ))(input) // if the inner parsers did not match enough information to return an accurate error - .map_err(|e| e.map_err(|_| Error::kind(input, ErrorKind::InvalidPrimary))) + .map_err(|e| e.map_err(|_| Error::new_from_kind(input, ErrorKind::InvalidPrimary))) } /// expression = or diff --git a/filter_parser/src/value.rs b/filter_parser/src/value.rs index 5f4677a2e..79fc00acd 100644 --- a/filter_parser/src/value.rs +++ b/filter_parser/src/value.rs @@ -15,11 +15,11 @@ pub fn parse_value(input: Span) -> IResult { return Err(err); } match parse_geo_radius(input) { - Ok(_) => return Err(nom::Err::Failure(Error::kind(input, ErrorKind::MisusedGeo))), + Ok(_) => return Err(nom::Err::Failure(Error::new_from_kind(input, ErrorKind::MisusedGeo))), // if we encountered a failure it means the user badly wrote a _geoRadius filter. // But instead of showing him how to fix his syntax we are going to tell him he should not use this filter as a value. Err(e) if e.is_failure() => { - return Err(nom::Err::Failure(Error::kind(input, ErrorKind::MisusedGeo))) + return Err(nom::Err::Failure(Error::new_from_kind(input, ErrorKind::MisusedGeo))) } _ => (), } @@ -45,9 +45,11 @@ pub fn parse_value(input: Span) -> IResult { )(input) .map(|(s, t)| (s, t.into())) // if we found nothing in the alt it means the user did not input any value - .map_err(|e| e.map_err(|_| Error::kind(input, ErrorKind::ExpectedValue))) + .map_err(|e| e.map_err(|_| Error::new_from_kind(input, ErrorKind::ExpectedValue))) // if we found encountered a failure it means the user really tried to input a value, but had an unmatched quote - .map_err(|e| e.map_fail(|c| Error::kind(input, ErrorKind::MissingClosingDelimiter(c.char())))) + .map_err(|e| { + e.map_fail(|c| Error::new_from_kind(input, ErrorKind::MissingClosingDelimiter(c.char()))) + }) } fn is_key_component(c: char) -> bool {