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 segfault on bogus SVG data #412

Merged
merged 9 commits into from
Dec 13, 2023
Merged

Conversation

a740g
Copy link
Contributor

@a740g a740g commented Dec 4, 2023

This PR avoids a segfault that happens when binary data pretending to be valid SVG text is passed to the SVG parser.

For example:

img& = _LOADIMAGE("mysvg.svg", 32)  ' here if mysvg.svg has invalid binary junk it will trigger the segfault

For memory loads we check the data byte by byte while copying the data for nsvgParse().
For file loads we check the file extension and check the data before passing it to nsvgParse().

@a740g a740g added the bug Something isn't working label Dec 4, 2023
@a740g a740g self-assigned this Dec 4, 2023
Copy link
Contributor

@mkilgore mkilgore left a comment

Choose a reason for hiding this comment

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

@a740g do we have any sense of where in nanosvg it blows up with a seg-fault?

I think ideally we'd address the issue in there, but only if it's simple enough. If it's a more fundamental problem with nanosvg then this type of approach makes sense, but we may want to consider looking for a replacement for nanosvg that correctly validates the input 🤷‍♂️

svgString[size] = '\0';
// Bail if we have binary data. We'll also copy the data while doing the check to avoid another pass
for (size_t i = 0; i < size; i++) {
auto c = svgString[i] = buffer[i];
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that char is typically signed so the +127 unicode values would trigger the c < 32 check (I think the auto will be char due to the type of svgString). You should probably specify unsigned here if we want to allow them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are correct. Nice catch!

@a740g
Copy link
Contributor Author

a740g commented Dec 5, 2023

@a740g do we have any sense of where in nanosvg it blows up with a seg-fault?

I think ideally we'd address the issue in there, but only if it's simple enough. If it's a more fundamental problem with nanosvg then this type of approach makes sense, but we may want to consider looking for a replacement for nanosvg that correctly validates the input 🤷‍♂️

I am checking on this one now. It usually happens inside nsvgParse().

@a740g a740g merged commit f8576b2 into QB64-Phoenix-Edition:main Dec 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants