Skip to content

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.

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

Merged
merged 7 commits into from
Apr 24, 2025
Merged

Fix JSON parsing of escaped strings #7545

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
eee62ff
work
kripken Apr 23, 2025
e118ca3
typo
kripken Apr 23, 2025
d01959b
fix test
kripken Apr 24, 2025
ba65c92
Update src/support/json.h
kripken Apr 24, 2025
0ecb450
Merge remote-tracking branch 'myself/json.escape.slash' into json.esc…
kripken Apr 24, 2025
8f5fb6d
Merge remote-tracking branch 'origin/main' into json.escape.slash
kripken Apr 24, 2025
d3baacc
text
kripken Apr 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions scripts/test/fuzzing.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,9 @@
'type-refining-gufa-exact.wast',
# TODO: fuzzer support for custom descriptors
'custom-descriptors.wast',
# TODO: fix split_wast() on tricky escaping situations like a string ending
# in \\" (the " is not escaped - there is an escaped \ before it)
'string-lifting-section.wast',
]


Expand Down
20 changes: 13 additions & 7 deletions src/support/json.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,14 +282,20 @@ struct Value {
skip();
if (*curr == '"') {
// String
// Start |close| at the opening ", and in the loop below we will always
// Start |close| after the opening ", and in the loop below we will always
// begin looking at the first character after.
char* close = curr;
// Skip escaped "
do {
close = strchr(close + 1, '"');
} while (*(close - 1) == '\\');
THROW_IF(!close, "malformed JSON string");
char* close = curr + 1;
// Skip escaped ", which appears as \". We need to be careful though, as
// \" might also be \\" which would be an escaped \ and an *un*escaped ".
while (*close && *close != '"') {
if (*close == '\\') {
// Skip the \ and the character after it, which it escapes.
close++;
THROW_IF(!*close, "unexpected end of JSON string (quoting)");
}
close++;
Copy link
Member

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.

Copy link
Member Author

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?

Copy link
Member

Choose a reason for hiding this comment

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

No, probably not.

}
THROW_IF(!close, "unexpected end of JSON string");
*close = 0; // end this string, and reuse it straight from the input
char* raw = curr + 1;
if (stringEncoding == ASCII) {
Expand Down
11 changes: 10 additions & 1 deletion test/lit/passes/string-lifting-section.wast
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

;; Lower first to generate the string.consts custom section, then lift it back.

;; RUN: foreach %s %t wasm-opt -all --string-lowering --string-lifting -S -o - | filecheck %s
;; RUN: wasm-opt %s -all --string-lowering --string-lifting -S -o - | filecheck %s

(module
;; CHECK: (type $0 (array (mut i16)))
Expand Down Expand Up @@ -37,6 +37,8 @@

;; CHECK: (import "string.const" "5" (global $"string.const_\"unpaired low surrogate \\ed\\bd\\88 \"" (ref extern)))

;; CHECK: (import "string.const" "6" (global $"string.const_\"z\\\\\"" (ref extern)))

;; CHECK: (import "wasm:js-string" "fromCharCodeArray" (func $fromCharCodeArray (type $3) (param (ref null $0) i32 i32) (result (ref extern))))

;; CHECK: (import "wasm:js-string" "fromCodePoint" (func $fromCodePoint (type $4) (param i32) (result (ref extern))))
Expand Down Expand Up @@ -94,6 +96,9 @@
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (string.const "unpaired low surrogate \ed\bd\88 ")
;; CHECK-NEXT: )
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (string.const "z\\")
;; CHECK-NEXT: )
;; CHECK-NEXT: )
(func $tricky-consts
;; These tricky strings should remain exactly the same after lowering and
Expand All @@ -110,6 +115,10 @@
(drop
(string.const "unpaired low surrogate \ED\BD\88 ")
)
;; A string with \", but the " is not escaped, as the \ is part of \\.
(drop
(string.const "z\\")
)
)
)

Loading