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

Fix svg parsing in case it uses empty use tag or use with image href #7044

Merged
merged 1 commit into from
Apr 30, 2021

Conversation

andrejslogins
Copy link
Contributor

@andrejslogins andrejslogins commented Apr 27, 2021

Fix cases where parsing string to svg was crashing.

Added the two test cases where this occurs.
1st case (empty ) occurs because xLinkAttribute is null (line 476)
2nd case (image href) occurs because element.parentNode is null

Didn't bump dist files as i'm not aware if this should be done in scope of PRs or not

@andrejslogins andrejslogins force-pushed the fix-svg-use-crash-cases branch from 5db9ca9 to fd9726a Compare April 27, 2021 08:40
xlinkAttribute = el.getAttribute('xlink:href') || el.getAttribute('href');

if (xlinkAttribute === null) {
return;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is this function returning on the others return early case? are we safe returning undefined?

Copy link
Contributor Author

@andrejslogins andrejslogins Apr 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no explicit returns are there in the following code, so it's safe

edit for clarity:

There are no other return statements in parseUseDirectives

Copy link
Contributor Author

@andrejslogins andrejslogins Apr 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only usage here (src/parser.js):

 fabric.parseSVGDocument = function(doc, callback, reviver, parsingOptions) {
    if (!doc) {
      return;
    }

    parseUseDirectives(doc);
    ..........
    ..........
    ..........

@andrejslogins andrejslogins requested a review from asturur April 27, 2021 09:40
@andrejslogins
Copy link
Contributor Author

andrejslogins commented Apr 27, 2021

For checks:

  • Pr quality check / coverage (pull_request) :Error: Resource not accessible by integration , looks like this is unrelated to the PR itself

@andrejslogins
Copy link
Contributor Author

@asturur All good from your side?

@asturur
Copy link
Member

asturur commented Apr 30, 2021

Sorry i have been away

@asturur
Copy link
Member

asturur commented Apr 30, 2021

So to clarify my question, the first time i looked at the PR i must have opened to much diff lines or to few, and i was seeing some other returns, but i see i was watching something else.

Pr looks good, i m merging it. Next time try to avoid unnecessary format changes, however they may look better, they create non necessary diff changes to look at.

Unless is a specific linting PR.

@asturur asturur merged commit 763b08b into fabricjs:master Apr 30, 2021
@andrejslogins
Copy link
Contributor Author

sure, will keep in mind. thanks

@asturur asturur mentioned this pull request May 22, 2021
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

Successfully merging this pull request may close these issues.

2 participants