Skip to content

Fix buffer overrun while parsing malformed JSON hex escape sequence #2201

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
Feb 16, 2018
Merged

Fix buffer overrun while parsing malformed JSON hex escape sequence #2201

merged 1 commit into from
Feb 16, 2018

Conversation

martijnthe
Copy link
Contributor

Fixes #2200

JerryScript-DCO-1.0-Signed-off-by: Martijn The [email protected]

@martijnthe
Copy link
Contributor Author

cc @DanielBallaSZTE @zherczeg @renatahodovan

@martijnthe martijnthe force-pushed the bugfix-json-hex-escape-buffer-overrun branch from c0e6cf5 to ade7a0b Compare February 10, 2018 18:42
@martijnthe
Copy link
Contributor Author

martijnthe commented Feb 10, 2018 •
edited
Loading

@akosthekiss Any idea what's wrong with the sign-off signature?
What CI reports is exactly what's at the end of the commit message...
Should there be a newline at the end perhaps?
Or is it choking on the é?

@zherczeg
Copy link
Member

What do you think about #2194 ?

@martijnthe
Copy link
Contributor Author

@zherczeg ah, I had not seen there was already a PR for it.

I think #2194 is still not quite correct. I think the comparison should include the count for 1 char for the u and 4 chars for the hex value. It's also missing a test case as you pointed out.

In my PR I also replaced the various uses of magic number 4 with a #define, which is a bit better IMO.

@martijnthe
Copy link
Contributor Author

@akosthekiss Any idea what's wrong with the sign-off signature?
What CI reports is exactly what's at the end of the commit message...
Should there be a newline at the end perhaps?
Or is it choking on the é?

OK, the problem is the é was represented differently in the commit message text vs author field...

@martijnthe martijnthe force-pushed the bugfix-json-hex-escape-buffer-overrun branch from ade7a0b to 6dab5ea Compare February 12, 2018 09:53
@robertsipka
Copy link
Contributor

robertsipka commented Feb 12, 2018 •
edited
Loading

@martijnthe : Yes you are right, I've missed to increase with 1. I also strongly support the removal of magic constants. But the mistake that was made earlier to fix this kind of issue is still there. I suggest you to modify the condition as in #2194 instead of adding a new one before.

robertsipka
robertsipka reviewed Feb 12, 2018
View reviewed changes
jerry-core/ecma/builtin-objects/ecma-builtin-json.c Outdated
if ((end_p - current_p < ECMA_JSON_HEX_ESCAPE_SEQUENCE_LENGTH + 1)) {
return;
}

ecma_char_t code_unit;
if ((end_p - current_p >= 2) && !(lit_read_code_unit_from_hex (current_p + 1, 4, &code_unit)))
Copy link
Contributor

Choose a reason for hiding this comment

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

You should use ECMA_JSON_HEX_ESCAPE_SEQUENCE_LENGTH here too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh whoops, I intended to remove that condition there. It would be redundant otherwise.

@martijnthe martijnthe force-pushed the bugfix-json-hex-escape-buffer-overrun branch from 6dab5ea to d97a35f Compare February 12, 2018 10:31
@martijnthe
Copy link
Contributor Author

@robertsipka OK, fixed it.

Should I also create a #define to replace the magic 1 (the length of the u)?
What do you think?

robertsipka
robertsipka reviewed Feb 12, 2018
View reviewed changes
jerry-core/ecma/builtin-objects/ecma-builtin-json.c Outdated
ecma_char_t code_unit;
if ((end_p - current_p >= 2) && !(lit_read_code_unit_from_hex (current_p + 1, 4, &code_unit)))
if (!(lit_read_code_unit_from_hex (current_p + 1, 4, &code_unit)))
Copy link
Contributor

Choose a reason for hiding this comment

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

You've forgot to replace 4 with ECMA_JSON_HEX_ESCAPE_SEQUENCE_LENGTH here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I do not think it is necessary. LGTM (informally) after this minor fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

zherczeg
zherczeg reviewed Feb 12, 2018
View reviewed changes
jerry-core/ecma/builtin-objects/ecma-builtin-json.c Outdated
@@ -182,13 +187,17 @@ ecma_builtin_json_parse_string (ecma_json_token_t *token_p) /**< token argument
}
case LIT_CHAR_LOWERCASE_U:
{
if ((end_p - current_p < ECMA_JSON_HEX_ESCAPE_SEQUENCE_LENGTH + 1)) {
return;
Copy link
Member

Choose a reason for hiding this comment

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

what about <= ? Then the +1 is unnecessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@martijnthe martijnthe force-pushed the bugfix-json-hex-escape-buffer-overrun branch from d97a35f to 8d53e43 Compare February 12, 2018 12:01
zherczeg
zherczeg approved these changes Feb 12, 2018
View reviewed changes
Copy link
Member

@zherczeg zherczeg left a comment

Choose a reason for hiding this comment

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

LGTM

@robertsipka robertsipka mentioned this pull request Feb 12, 2018
Closed
@martijnthe martijnthe force-pushed the bugfix-json-hex-escape-buffer-overrun branch from 8d53e43 to fb896f9 Compare February 12, 2018 13:47
@martijnthe
Copy link
Contributor Author

OK, I also satisfied the linter now and the CI build is ✅

LaszloLango
LaszloLango approved these changes Feb 16, 2018
View reviewed changes
Copy link
Contributor

@LaszloLango LaszloLango left a comment

Choose a reason for hiding this comment

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

LGTM

@LaszloLango LaszloLango merged commit 3c57698 into jerryscript-project:master Feb 16, 2018
grgustaf pushed a commit to grgustaf/jerryscript that referenced this pull request Mar 19, 2018
@martijnthe @grgustaf
Fix buffer overrun while parsing malformed JSON hex escape sequence (j…
6bf8530 
…erryscript-project#2201)

Fixes jerryscript-project#2200

JerryScript-DCO-1.0-Signed-off-by: Martijn The [email protected]
@zherczeg zherczeg mentioned this pull request Mar 27, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@robertsipka robertsipka robertsipka left review comments

@zherczeg zherczeg zherczeg approved these changes

@LaszloLango LaszloLango LaszloLango approved these changes

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

Successfully merging this pull request may close these issues.
4 participants
@martijnthe @zherczeg @robertsipka @LaszloLango