Avoid parsing to non-finite floating point numbers #167

Activity

1. federicomenaquintero commented on Dec 5, 2019

   

   federicomenaquintero

   on Dec 5, 2019

   Contributor

   More actions

   FWIW, in librsvg I ended up with this:

   pub trait CssParserExt {
       fn expect_finite_number(&mut self) -> Result<f32, ValueErrorKind>;
   }
   
   impl<'i, 't> CssParserExt for Parser<'i, 't> {
       fn expect_finite_number(&mut self) -> Result<f32, ValueErrorKind> {
           finite_f32(self.expect_number()?)
       }
   }
   
   pub fn finite_f32(n: f32) -> Result<f32, ValueErrorKind> {
       if n.is_finite() {
           Ok(n)
       } else {
           Err(ValueErrorKind::Value("expected finite number".to_string()))
       }
   }

   Of course this is tied to librsvg's error enums. The code uses expect_finite_number everywhere instead of cssparser's stock expect_number.
2. federicomenaquintero commented on Dec 5, 2019

   

   federicomenaquintero

   on Dec 5, 2019

   Contributor

   More actions

   I guess the question is, should encountering a non-finite number just yield an UnexpectedToken error? I don't know the details of the CSS spec well enough :)
3. SimonSapin commented on Dec 5, 2019

   

   SimonSapin

   on Dec 5, 2019

   MemberAuthor

   More actions

   https://drafts.csswg.org/css-values/#numeric-ranges

   CSS theoretically supports infinite precision and infinite ranges for all value types; however in reality implementations have finite capacity. UAs should support reasonably useful ranges and precisions.

   Unfortunately I couldn’t easily find spec text that says what to do with inputs that exceed that chosen capacity.
4. emilio commented on Dec 5, 2019

   

   emilio

   on Dec 5, 2019

   Member

   More actions

   I think we should parse it, and clamp it to some appropriate range. We should probably just not serialize inf. Gecko doesn't serialize inf, maybe this bug was fixed?
5. SimonSapin commented on Dec 5, 2019

   

   SimonSapin

   on Dec 5, 2019

   MemberAuthor

   More actions

   Probably when we started using the dtoa crate:

   fn main() {
       dbg!(dtoa_s(std::f32::INFINITY));
       dbg!(dtoa_s(std::f32::NAN));
       dbg!(dtoa_s(std::f32::MAX));
   }
   
   fn dtoa_s(x: f32) -> String {
       let mut v = Vec::<u8>::new();
       dtoa::write(&mut v, x).unwrap();
       String::from_utf8(v).unwrap()
   }

   Playground, output:

   [src/main.rs:2] dtoa_s(std::f32::INFINITY) = "3.4028237e38"
   [src/main.rs:3] dtoa_s(std::f32::NAN) = "5.1042355e38"
   [src/main.rs:4] dtoa_s(std::f32::MAX) = "3.4028235e38"
   
6. federicomenaquintero commented on Dec 5, 2019

   

   federicomenaquintero

   on Dec 5, 2019

   Contributor

   More actions

   Section 5.1 says:

   If the value is outside the allowed range, then unless otherwise specified, the declaration is invalid and must be ignored.

   So, "too large" -> "parse error" sounds reasonable.

   But section 10.9 has an interesting note:

   Note: By definition, ±∞ are outside the allowed range for any property, and will clamp to the minimum/maximum value allowed.

   So clamping to $large_value sounds fine too! If that makes number values rountrippable, this sounds good.
7. SimonSapin commented on Dec 6, 2019

   

   SimonSapin

   on Dec 6, 2019

   MemberAuthor

   More actions

   Parse errors seems like it might not be web-compatible at least for z-index where some pages use ever-larger value