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 the potential overflow in char refs #1009

Merged
merged 2 commits into from
Dec 10, 2024
Merged

Conversation

leethomason
Copy link
Owner

This lifts the tests from #1003 (which are quite nice.) The fix in #1003 was based on "not the best" code in TinyXML-2, so this cleans it up a bit, and attempts a tidier fix.

@leethomason leethomason mentioned this pull request Dec 5, 2024
@ped7g
Copy link

ped7g commented Dec 5, 2024

github shows tab-identation with tab=8 while old code is indented by space (by 4).

I think you can avoid this by adding .editorconfig file to the repo, or just use spaces consistently. :)

WRT to code changes, LGTM, although I'm itching to simplify the implementation a bit, but that's more like personal bias to how manual parsers can be written...

actually no, thinking about it, it's not just personal bias, your code can produce unexpected results for specially crafted inputs (not dangerous in security way, it would just produce ucs codepoint value within your limit but not matching the source file string, but my mind is a bit obsessed by code correctness).

But if I write my version, I should probably test it if it compiles and works... argh... :D

Are you open to such larger change suggestions and only if they are properly tested or is it worth just to propose untested code to you? Thank you for patience with me. ;)

@leethomason
Copy link
Owner Author

The fix wasn't quite correct - could overflow on bigger numbers. But - on the other hand - a bunch of leading zeros shouldn't cause a security problem. I think this is correct this time.

@leethomason
Copy link
Owner Author

@ped7g go easy on big changes - TinyXML2 is old code. It's not going to do anything new, although it's important to keep it running and safe. Point being, big changes don't generally do much but add risk. I'm certainly happy to look at an example PR of what you are thinking about.

@leethomason leethomason merged commit 4cbb251 into master Dec 10, 2024
12 checks passed
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