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

Improper css var() causes crash #974

Closed
sureshvv opened this issue Oct 21, 2019 · 12 comments
Closed

Improper css var() causes crash #974

sureshvv opened this issue Oct 21, 2019 · 12 comments
Labels
Milestone

Comments

@sureshvv
Copy link

@sureshvv sureshvv commented Oct 21, 2019

Getting this error when I process this html file
yy1.txt

@Tontyna

This comment has been minimized.

Copy link
Contributor

@Tontyna Tontyna commented Oct 21, 2019

That's a crash with announcement -- do you see the WARNING Unsupported computed value `calc(var(--line-height) / var(--line-height-ratio))` set in variable `--font-size` for property `font-size`.?

That's because calc() isn't implemented in WeasyPrint. Although this shouldn't crash WeasyPrint.

And indeed, it's not the unsupported calc, that crashes WeasyPrint, but the fallback value -- WeasyPrint's compute function detects that it cannot replace the var(--font-size) and falls back to the font-size of the parent. Which happens to be a unit-less simple integer or float.
And crashes in the font_size() function. We could make font_size aware for simple numbers, but then the length() function will crash.

@liZe - this fallback to a unit-less parent property is fatal for all css properties that designate a length, didn't count the COMPUTER_FUNCTIONS that call the length() method and the css properties that directly call length()... Assuming a Dimension wasn't a problem until var() was implemented -- regular unit-less lengths are rejected in "Step 2 - Fetching and parsing CSS"

@sureshvv - even if WeasyPrint was able to calculate your --font-size variable it would crash because the calc() in your css doesn't supply a unit, just plain numbers.

@sureshvv

This comment has been minimized.

Copy link
Author

@sureshvv sureshvv commented Oct 22, 2019

Browsers seem to handle it well. The html file displays perfectly in all the browsers that I have tested on. wkhtmltopdf also handles the same html file but not as well ;)

Won't the default unit be "px"?

@Tontyna

This comment has been minimized.

Copy link
Contributor

@Tontyna Tontyna commented Oct 22, 2019

Browsers seem to handle it well.

Yes, they dont crash. My Browser ignores the calculated dimension-less values and falls back to the initial or inherited value.

@Tontyna

This comment has been minimized.

Copy link
Contributor

@Tontyna Tontyna commented Oct 22, 2019

this fallback to a unit-less parent property is fatal for all css properties ...

That's not true. Almost all of the 'lengthy' properties get a sane value from INITIAL_VALUES and/or the responsible @register_computer function handles the case when this value is a deliberate integer.

Looks like only font-size and word-spacing are affected by the unit-less fallback crash.

@Tontyna

This comment has been minimized.

Copy link
Contributor

@Tontyna Tontyna commented Oct 22, 2019

Looks like only font-size and word-spacing are affected by the unit-less fallback crash.

Minimal example to crash WeasyPrint by var():

<style>
div {
  --value: invalidValue;
  border-spacing: var(--value);
}
</style>
<div>crash</div>

The complete list of properties that kill WeasyPrint when defined via an invalid var() value is:

  • word-spacing
  • font-size
  • border-spacing
  • size
  • content
  • string-set
  • anchor
  • link
  • lang
  • transform
@Tontyna

This comment has been minimized.

Copy link
Contributor

@Tontyna Tontyna commented Oct 22, 2019

@sureshvv would you be so kind and rename this issue to sth like "improper css var() causes crash"?

@sureshvv sureshvv changed the title AttributeError: 'int' object has no attribute 'unit' Improper css var() causes crash Oct 23, 2019
@sureshvv

This comment has been minimized.

Copy link
Author

@sureshvv sureshvv commented Oct 23, 2019

Done. Now who wlll fix this? ;)

@Tontyna

This comment has been minimized.

Copy link
Contributor

@Tontyna Tontyna commented Oct 23, 2019

Now who wlll fix this?

  • I'm currently preparing a PR to prevent the crash.
  • AFAIK nobody is working on #357
@liZe liZe added the crash label Nov 8, 2019
@liZe liZe added this to the 51 milestone Nov 8, 2019
@liZe liZe closed this in a1df1de Dec 18, 2019
@Tontyna

This comment has been minimized.

Copy link
Contributor

@Tontyna Tontyna commented Dec 18, 2019

@liZe -- your commit is a lot more elegant than #977 👍

But it doesn't survive invalid values for font-size (fixed in font_size() by af78171) and transform (fixed with try..except by 0bb4e2c).

Cf. f0a432f, the full test for all KNOWN_VALUES.

@liZe

This comment has been minimized.

Copy link
Member

@liZe liZe commented Dec 19, 2019

@liZe -- your commit is a lot more elegant than #977

I’ve missed your PR, even with your comment and the bold title below. I don’t know how it’s possible. Maybe it’s time to sleep.

👓

(I’m really sorry…)

But it doesn't survive invalid values for font-size (fixed in font_size() by af78171) and transform (fixed with try..except by 0bb4e2c).

😞

Cf. f0a432f, the full test for all KNOWN_VALUES.

It’s much better than my little test.

I’ll cherry-pick some of your commits for sure, thanks a lot.

liZe added a commit that referenced this issue Dec 19, 2019
Validators return None when value is incorrect, let’s use the same behaviour
for transform.

Related to #974, #977.
@liZe

This comment has been minimized.

Copy link
Member

@liZe liZe commented Dec 19, 2019

I’ve cherry-picked your test (and "parametrize" it, it’s easier to debug).

As you said, transform was crashing. I’ve removed (in c21028e) the InvalidValues exception from this validator, as it was the only one doing this.

(But we should probably call validate_non_shorthand instead of calling PROPERTIES[…], I don’t know how initial/inherit are handled now. In this case, we’ll have to cherry-pick 0bb4e2c too.)

font-size works well, even without af78171. As the value is already computed, we never call the computer function with integers.

@Tontyna

This comment has been minimized.

Copy link
Contributor

@Tontyna Tontyna commented Dec 19, 2019

font-size works well, even without af78171

You're right, of course -- me needing more sleep, too

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.