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

Avoid parsing to non-finite floating point numbers #167

Open
SimonSapin opened this issue Jun 27, 2017 · 7 comments
Open

Avoid parsing to non-finite floating point numbers #167

SimonSapin opened this issue Jun 27, 2017 · 7 comments

Comments

@SimonSapin
Copy link
Member

In #165, this input 3E833 is parsed as a Token::Number whose value is std::f32::INFINITY, which serializes as inf.

We should definitely not serialize any number as inf (maybe an arbitrary large number instead), but probably also avoid non-finite numbers in tokens in the first place.

CC servo/servo#15729

@federicomenaquintero
Copy link
Contributor

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.

@federicomenaquintero
Copy link
Contributor

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 :)

@SimonSapin
Copy link
Member Author

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.

@emilio
Copy link
Member

emilio commented Dec 5, 2019

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?

@SimonSapin
Copy link
Member Author

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"

@federicomenaquintero
Copy link
Contributor

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.

@SimonSapin
Copy link
Member Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants