8000 Fix crash when escaping JSON strings with UTF-8 characters by Morriar · Pull Request #3447 · sorbet/sorbet · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Fix crash when escaping JSON strings with UTF-8 characters #3447

8000
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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Sep 23, 2020
Merged

Fix crash when escaping JSON strings with UTF-8 characters #3447

merged 1 commit into from
Sep 23, 2020

Conversation

Morriar
Copy link
Collaborator
@Morriar Morriar commented Sep 23, 2020

Motivation

Running sorbet --print parse-tree-json -e "'a👋a'" with Sorbet's master results in a Bus error: 10 error.

This is because when we escape the String with JSON::escape we iterate over each character of the string:

string JSON::escape(string from) {
    //...
    for (auto ch : from) {
        switch (ch) {
            case '\\':
            // ...

For the a👋a string this mean we actually end up iterating over the code points of the UTF-8 chars:

61 -> a
fffffff0 -> 👋[0]
ffffff9f -> 👋[1]
ffffff91 -> 👋[2]
ffffff8b -> 👋[3]
61 -> a

This means that later when we will check if that character is a control character:

                if (ch <= 0x1f) {
                    string_view toAdd(from.data() + firstUnusedChar, currentChar - firstUnusedChar);
                    firstUnusedChar = currentChar + 1;
                    fmt::format_to(buf, "{}\\u{:04x}", toAdd, ch);
                    break;
                }

The condition will be true and we will print the code-point resulting in the bus error.

We can avoid this by adding a guard on the lower bound:

if (0x00 <= ch && ch <= 0x1f) {

Test plan

We had no test for parse-json so I copied and adapted the parse-whitequark one.

@Morriar
Fix crash when escaping JSON strings with UTF-8 characters
cc3454c 
Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
@Morriar Morriar requested a review from a team as a code owner September 23, 2020 18:03
@Morriar Morriar requested review from DarkDimius and removed request for a team September 23, 2020 18:03
Morriar
Morriar commented Sep 23, 2020
View reviewed changes
test/cli/parse-tree-json/parse-tree-json.out
},
{
"type" : "String",
"val" : "👋 a string"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

🎉

@Morriar Morriar requested review from elliottt and jez and removed request for DarkDimius September 23, 2020 18:04
elliottt
elliottt approved these changes Sep 23, 2020
View reviewed changes
Copy link
Collaborator
@elliottt elliottt left a comment

Choose a reason for hiding this comment

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

Great find, thank you for the test case!

@elliottt elliottt merged commit a7a412f into sorbet:master Sep 23, 2020
@Morriar Morriar deleted the at-fix-parse-tree-json branch September 23, 2020 18:56
@Morriar Morriar mentioned this pull request Sep 23, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@elliottt elliottt elliottt approved these changes

@jez jez Awaiting requested review from jez

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Morriar @elliottt
0