-
Notifications
You must be signed in to change notification settings - Fork 777
Fix JSON parsing of escaped strings #7545
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, this looks simpler than my solution from https://github.com/WebAssembly/binaryen/pull/7486/files#diff-88a72bcb9f2621a8ab37e23773a8209463146494d55a988255efff2fa35cf8b6R276-R290
src/support/json.h
Outdated
if (*close == '\\') { | ||
// Skip the \ and the character after it, which it escapes. | ||
close++; | ||
assert(*close); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure how useful this assertion is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is necessary, because if this is 0, then it is not valid to do the next close++
and then read *close
, which would be past the string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see, it's checking for the end of a C-style string. Can we add a && "unexpected end of string"
to clarify?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see this file was switched to throwing from asserts meanwhile, as I merged in main. I updated the errors to have text like that.
close++; | ||
assert(*close); | ||
} | ||
close++; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: ++close
here and above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are just pointers, and the current code is consistent with usage elsewhere in the file - is it worth adding an inconsistency here do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, probably not.
To find the end of a string, we must be more careful of escaping - we assumed
any
\"
was an escaped double-quote, but it might be part of\\"
, that is, wherethere is an escaped
\
before us, and the double-quote is not escaped itself.This fixes StringLifting on real-world Java code I am testing on.