Skip to content

Conversation

@LaszloLango
Copy link
Contributor

Fixes #1549

JerryScript-DCO-1.0-Signed-off-by: László Langó llango.u-szeged@partner.samsung.com

@LaszloLango LaszloLango added the bug Undesired behaviour label Feb 10, 2017
@robertsipka
Copy link
Contributor

LGTM (informally).


if (!lit_is_utf8_string_valid ((lit_utf8_byte_t *) source_p, (lit_utf8_size_t) source_size))
{
return ecma_raise_syntax_error (ECMA_ERR_MSG ("The input should be a valid UTF8 string."));
Copy link
Member

Choose a reason for hiding this comment

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

Input must be a valid UTF-8 string.

{
return 0;
}

Copy link
Member

Choose a reason for hiding this comment

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

Do we need this? jerry_parse_and_save_snapshot calls jerry_parse.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes we do. It does not call jerry_parse. It calls parser_parse_script

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps this is really not needed anymore.

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

@zherczeg
Copy link
Member

I have been thinking about this in the weekend. Perhaps we should make this a runtime enable.disable feature, and do check for all functions which accepts utf8/cesu8 input. This patch is more like a workaround than a fix.

@LaszloLango LaszloLango force-pushed the fix-issue-1549 branch 3 times, most recently from 83b0356 to 3c1c77a Compare February 15, 2017 14:50
@LaszloLango LaszloLango changed the title Add input validation to 'jerry_parse' and 'jerry_parse_and_save_snapshot' Add new input validator API functions Feb 15, 2017
@LaszloLango
Copy link
Contributor Author

@zherczeg, @robertsipka I've updated the PR. Please review again.

- [jerry_get_utf8_string_length](#jerry_get_utf8_string_length)
- [jerry_is_utf8_string_valid](#jerry_is_utf8_string_valid)


Copy link
Member

Choose a reason for hiding this comment

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

Do we need two newlines here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we do. This is the style in the whole MD


```c
bool
jerry_is_utf8_string_valid (const jerry_char_t *utf8_buf_p, /**< UTF-8 string */
Copy link
Member

Choose a reason for hiding this comment

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

What do you think of jerry_is_valid_utf8_string? Somehow that is more natural to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good to me. Should we rename the internal functions too? lit_is_valid_utf8_string?

Copy link
Member

Choose a reason for hiding this comment

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

At some point we should. If you wish you can do it in this patch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I'll do it in this patch.

{
return 0;
}

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps this is really not needed anymore.


**See also**

- [jerry_run_simple](#jerry_run_simple)
Copy link
Member

Choose a reason for hiding this comment

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

jerry_run_simple expects utf8 string, not cesu8

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed, removed the reference from jerry_is_cesu8_string_valid

@LaszloLango LaszloLango force-pushed the fix-issue-1549 branch 2 times, most recently from cae38f4 to 755a1e8 Compare February 16, 2017 08:08
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 after the rename.

* @return true - if UTF-8 string is well-formed
* false - otherwise
*/
bool
Copy link
Member

Choose a reason for hiding this comment

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

Please rename this function as well (jerry_is_valid_utf8_string)


/**
* Input validatator functions
*/
Copy link
Member

Choose a reason for hiding this comment

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

Here as well.

- [jerry_object_property_foreach_t](#jerry_object_property_foreach_t)


# Input validatator functions
Copy link
Member

Choose a reason for hiding this comment

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

validator

bool jerry_foreach_object_property (const jerry_value_t obj_val, jerry_object_property_foreach_t foreach_p,
void *user_data_p);

/**
Copy link
Member

Choose a reason for hiding this comment

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

validator

Fixes jerryscript-project#1549

JerryScript-DCO-1.0-Signed-off-by: László Langó llango.u-szeged@partner.samsung.com
Copy link
Member

@dbatyai dbatyai left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Undesired behaviour

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants