refactor: Optimize json string-based parameter checking and object ha… #887
Conversation
|
Is there a test that can be added to validate the fix? |
I don't know how to add a test because it only appears in a particular wasm runtime environment |
…ndling logic 1、Parameter check 2、Memory allocation check 3、Buffer write check 4、Resource cleaning 5、Length check These changes can: 1、Prevent null pointer access 2、Ensure memory allocation 3、Check write to buffer 4、Clean up resources propierly 5、Avoid integer overflowj
|
You seem to have introduced a leak, that's why it crashes in Debug mode. |
| v = js_array_push(ctx, jsc->stack, 1, &val, 0); | ||
| if (check_exception_free(ctx, v)) | ||
| goto exception; |
There was a problem hiding this comment.
Removing this seems problematic: if val is an array that contains a self reference, the cycle will not be detected.
Same problem for non-Array objects with self references.
| JSPropertyEnum *props = NULL; | ||
| uint32_t prop_count = 0; | ||
|
|
There was a problem hiding this comment.
This array should be defined at the outer scope so the objects can be freed in case of exception.
| JSValue quoted = JS_ToQuotedStringFree(ctx, js_dup(val)); | ||
| if (JS_IsException(quoted)) | ||
| return -1; | ||
| return string_buffer_concat_value(jsc->b, quoted); |
There was a problem hiding this comment.
This is a memory leak: should be return string_buffer_concat_value_free(jsc->b, quoted)
| JSValue num_str = JS_ToString(ctx, val); | ||
| if (JS_IsException(num_str)) | ||
| return -1; | ||
| return string_buffer_concat_value(jsc->b, num_str); |
There was a problem hiding this comment.
This is a memory leak: should be return string_buffer_concat_value_free(jsc->b, num_str)
| JSValue str = JS_ToString(ctx, val); | ||
| if (JS_IsException(str)) | ||
| return -1; | ||
| return string_buffer_concat_value(jsc->b, str); |
There was a problem hiding this comment.
This is a memory leak: should be return string_buffer_concat_value_free(jsc->b, str)
| return -1; | ||
| return string_buffer_concat_value(jsc->b, num_str); | ||
| } | ||
|
|
There was a problem hiding this comment.
You have many instances of trailing spaces in the patch, please remove these.
| exception: | ||
| JS_FreeValue(ctx, indent1); | ||
| JS_FreeValue(ctx, sep); | ||
| JS_FreeValue(ctx, sep1); | ||
| JS_FreeValue(ctx, tab); | ||
| JS_FreeValue(ctx, prop); |
There was a problem hiding this comment.
Move the definitions to the outer scope and add js_free_prop_enum(ctx, props, prop_count);
| p = JS_VALUE_GET_OBJ(val); | ||
| cl = p->class_id; | ||
| if (cl == JS_CLASS_STRING) { | ||
| val = JS_ToStringFree(ctx, val); | ||
| if (JS_IsException(val)) | ||
| goto exception; | ||
| goto concat_primitive; | ||
| } else if (cl == JS_CLASS_NUMBER) { | ||
| val = JS_ToNumberFree(ctx, val); | ||
| if (JS_IsException(val)) | ||
| goto exception; | ||
| goto concat_primitive; | ||
| } else if (cl == JS_CLASS_BOOLEAN || cl == JS_CLASS_BIG_INT) { | ||
| set_value(ctx, &val, js_dup(p->u.object_data)); | ||
| goto concat_primitive; | ||
| } |
There was a problem hiding this comment.
Removing this code has some side effects:
JSON.stringify(new String())->'{}'instead of'""'(as in v8)JSON.stringify(new Boolean())->'{}'instead of'false'(as in v8)JSON.stringify(new Number())->'{}'instead of'0'(as in v8)
I am not sure if tc39 has tests for these, but since node has consistent behavior for boxed values, we should implement these the same way.
|
Sorry, I used emscripten for - O1 packaging and the wasm runtime ran successfully. Maybe it's not a bug in QuickJS. I will close this PR. |
885