Fix free of NULL value in function ecma_typedarray_helper_dispatch_construct #4473

lygstate · 2021-01-15T15:27:41Z

Fix free of NULL value in function ecma_typedarray_helper_dispatch_construct

Currently, ecma_op_get_prototype_from_constructor may return NULL
and the function didn't raise that exception.
Also optimize multiple assignment of prototype_obj_p and
multiple access of JERRY_CONTEXT (current_new_target) out.

This fixes the esnext tests

--- a/tests/test262-esnext-excludelist.xml
+++ b/tests/test262-esnext-excludelist.xml
@@ -198,24 +198,14 @@
   <test id="built-ins/TypedArray/prototype/toLocaleString/BigInt/get-length-uses-internal-arraylength.js"><reason></reason></test>
   <test id="built-ins/TypedArray/prototype/toLocaleString/BigInt/return-result.js"><reason></reason></test>
   <test id="built-ins/TypedArrayConstructors/ctors-bigint/buffer-arg/byteoffset-is-negative-zero.js"><reason></reason></test>
-  <test id="built-ins/TypedArrayConstructors/ctors-bigint/buffer-arg/custom-proto-access-throws.js"><reason></reason></test>
   <test id="built-ins/TypedArrayConstructors/ctors-bigint/buffer-arg/defined-negative-length.js"><reason></reason></test>
   <test id="built-ins/TypedArrayConstructors/ctors-bigint/buffer-arg/toindex-byteoffset.js"><reason></reason></test>
-  <test id="built-ins/TypedArrayConstructors/ctors-bigint/length-arg/custom-proto-access-throws.js"><reason></reason></test>
   <test id="built-ins/TypedArrayConstructors/ctors-bigint/length-arg/toindex-length.js"><reason></reason></test>
-  <test id="built-ins/TypedArrayConstructors/ctors-bigint/no-args/custom-proto-access-throws.js"><reason></reason></test>
-  <test id="built-ins/TypedArrayConstructors/ctors-bigint/object-arg/custom-proto-access-throws.js"><reason></reason></test>
-  <test id="built-ins/TypedArrayConstructors/ctors-bigint/typedarray-arg/custom-proto-access-throws.js"><reason></reason></test>
   <test id="built-ins/TypedArrayConstructors/ctors/buffer-arg/byteoffset-is-negative-zero.js"><reason></reason></test>
-  <test id="built-ins/TypedArrayConstructors/ctors/buffer-arg/custom-proto-access-throws.js"><reason></reason></test>
   <test id="built-ins/TypedArrayConstructors/ctors/buffer-arg/defined-negative-length.js"><reason></reason></test>
   <test id="built-ins/TypedArrayConstructors/ctors/buffer-arg/toindex-byteoffset.js"><reason></reason></test>
-  <test id="built-ins/TypedArrayConstructors/ctors/length-arg/custom-proto-access-throws.js"><reason></reason></test>
   <test id="built-ins/TypedArrayConstructors/ctors/length-arg/toindex-length.js"><reason></reason></test>
-  <test id="built-ins/TypedArrayConstructors/ctors/no-args/custom-proto-access-throws.js"><reason></reason></test>
-  <test id="built-ins/TypedArrayConstructors/ctors/object-arg/custom-proto-access-throws.js"><reason></reason></test>
   <test id="built-ins/TypedArrayConstructors/ctors/object-arg/returns.js"><reason></reason></test>
-  <test id="built-ins/TypedArrayConstructors/ctors/typedarray-arg/custom-proto-access-throws.js"><reason></reason></test>
   <test id="built-ins/TypedArrayConstructors/from/BigInt/custom-ctor-returns-other-instance.js"><reason></reason></test>
   <test id="built-ins/TypedArrayConstructors/from/BigInt/custom-ctor.js"><reason></reason></test>
   <test id="built-ins/TypedArrayConstructors/from/BigInt/new-instance-using-custom-ctor.js"><reason></reason></test>

NOTE:This depeds on #4478 (review)

JerryScript-DCO-1.0-Signed-off-by: Yonggang Luo luoyonggang@gmail.com

rerobika

LGTM

galpeter

Could we have a JS test case of this?

zherczeg · 2021-01-15T17:27:37Z

jerry-core/ecma/builtin-objects/typedarray/ecma-builtin-typedarray-helpers.c


-  if (JERRY_CONTEXT (current_new_target))
+  if (current_new_target)


current_new_target != NULL

zherczeg · 2021-01-15T17:27:44Z

jerry-core/ecma/builtin-objects/typedarray/ecma-builtin-typedarray-helpers.c

@@ -40,11 +41,20 @@ ecma_typedarray_helper_dispatch_construct (const ecma_value_t *arguments_list_p,
 {
  JERRY_ASSERT (arguments_list_len == 0 || arguments_list_p != NULL);
  ecma_builtin_id_t proto_id = ecma_typedarray_helper_get_prototype_id (typedarray_id);
-  ecma_object_t *prototype_obj_p = ecma_builtin_get (proto_id);
+  ecma_object_t *current_new_target = JERRY_CONTEXT (current_new_target);


current_new_target_p

zherczeg · 2021-01-15T17:27:54Z

jerry-core/ecma/builtin-objects/typedarray/ecma-builtin-typedarray-helpers.c

@@ -53,7 +63,7 @@ ecma_typedarray_helper_dispatch_construct (const ecma_value_t *arguments_list_p,
                                                ecma_typedarray_helper_get_shift_size (typedarray_id),
                                                typedarray_id);

-  if (JERRY_CONTEXT (current_new_target))
+  if (current_new_target)


current_new_target != NULL

lygstate · 2021-01-15T17:28:12Z

Could we have a JS test case of this?

Not sure about that, this tends to be un-implement feature, just not crash it.
If we add test and then the feature are implemented how to handle that?

lygstate · 2021-01-15T19:10:42Z

Could we have a JS test case of this?

You are right, test case are needed:) I've add one,
I am not sure if prototype_obj_p may be NULL,
if NULL are not possible then JERRY_ASSERT may better.

jerry-core/ecma/builtin-objects/typedarray/ecma-builtin-typedarray-helpers.c

rerobika · 2021-01-15T19:14:45Z

jerry-core/ecma/builtin-objects/typedarray/ecma-builtin-typedarray-helpers.c

+    prototype_obj_p = ecma_builtin_get (proto_id);
+  }
+
+  if (prototype_obj_p == NULL)


This if is not needed, since ecma_builtin_get (proto_id); never returns NULL

OK, then I nested into the if statement

rerobika · 2021-01-15T19:45:40Z

jerry-core/ecma/builtin-objects/typedarray/ecma-builtin-typedarray-helpers.c

  {
-    prototype_obj_p = ecma_op_get_prototype_from_constructor (JERRY_CONTEXT (current_new_target), proto_id);
+    prototype_obj_p = ecma_op_get_prototype_from_constructor (current_new_target_p, proto_id);
+    if (jcontext_has_pending_exception ())


This if is unneccessary. If ecma_op_get_prototype_from_constructor return NULL it means there is already a raised exception on the context.

So what should I do, like this?

if (prototype_obj_p == NULL) { return ECMA_VALUE_ERROR; }

lygstate · 2021-01-15T21:01:17Z

jerry-core/ecma/builtin-objects/typedarray/ecma-builtin-typedarray-helpers.c

@@ -20,6 +20,7 @@
 #include "ecma-builtins.h"
 #include "ecma-gc.h"
 #include "ecma-objects.h"
+#include "ecma-exceptions.h"


should not include

Yeah it's unnecessary. But please for the lord use whole sentences and punctuation marks rather than halfwords.

…nstruct Currently, ecma_op_get_prototype_from_constructor may return NULL and the function didn't raise that exception. Also optimize multiple assignment of prototype_obj_p and multiple access of JERRY_CONTEXT (current_new_target) out. This fixes jerryscript-project#4463 JerryScript-DCO-1.0-Signed-off-by: Yonggang Luo luoyonggang@gmail.com

rerobika

LGTM

zherczeg

LGTM

lygstate changed the title ~~Fix free of NULL value in function ecma_typedarray_helper_dispatch_co…~~ Fix free of NULL value in function ecma_typedarray_helper_dispatch_construct Jan 15, 2021

lygstate force-pushed the fix-null-typedarray-prototype branch from b1208f8 to 285461b Compare January 15, 2021 15:45

rerobika approved these changes Jan 15, 2021

View reviewed changes

galpeter reviewed Jan 15, 2021

View reviewed changes

lygstate force-pushed the fix-null-typedarray-prototype branch from 285461b to 5ea1bba Compare January 15, 2021 17:24

zherczeg requested changes Jan 15, 2021

View reviewed changes

lygstate force-pushed the fix-null-typedarray-prototype branch 4 times, most recently from d0f7aea to 6593ccc Compare January 15, 2021 19:07

rerobika requested changes Jan 15, 2021

View reviewed changes

lygstate force-pushed the fix-null-typedarray-prototype branch 2 times, most recently from 8401c63 to 9a5254e Compare January 15, 2021 19:39

rerobika requested changes Jan 15, 2021

View reviewed changes

lygstate force-pushed the fix-null-typedarray-prototype branch from 9a5254e to 8e216a2 Compare January 15, 2021 19:58

lygstate requested a review from rerobika January 15, 2021 20:21

lygstate commented Jan 15, 2021

View reviewed changes

lygstate force-pushed the fix-null-typedarray-prototype branch 2 times, most recently from 6c9f073 to f292b4c Compare January 17, 2021 05:44

lygstate mentioned this pull request Jan 17, 2021

current_new_target -> current_new_target_p #4478

Merged

lygstate force-pushed the fix-null-typedarray-prototype branch from f292b4c to 0c52980 Compare January 17, 2021 14:39

rerobika approved these changes Jan 18, 2021

View reviewed changes

rerobika requested a review from zherczeg January 18, 2021 09:27

rerobika added the bug Undesired behaviour label Jan 18, 2021

zherczeg approved these changes Jan 18, 2021

View reviewed changes

zherczeg merged commit 90d206d into jerryscript-project:master Jan 18, 2021

lygstate deleted the fix-null-typedarray-prototype branch January 18, 2021 10:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix free of NULL value in function ecma_typedarray_helper_dispatch_construct #4473

Fix free of NULL value in function ecma_typedarray_helper_dispatch_construct #4473

lygstate commented Jan 15, 2021 •

edited

Loading

rerobika left a comment

galpeter left a comment

zherczeg Jan 15, 2021

zherczeg Jan 15, 2021

zherczeg Jan 15, 2021

lygstate commented Jan 15, 2021

lygstate commented Jan 15, 2021

rerobika Jan 15, 2021

lygstate Jan 15, 2021

rerobika Jan 15, 2021

lygstate Jan 15, 2021

lygstate Jan 15, 2021

rerobika Jan 16, 2021

rerobika left a comment

zherczeg left a comment


		if (JERRY_CONTEXT (current_new_target))
		if (current_new_target)

Fix free of NULL value in function ecma_typedarray_helper_dispatch_construct #4473

Fix free of NULL value in function ecma_typedarray_helper_dispatch_construct #4473

Conversation

lygstate commented Jan 15, 2021 • edited Loading

rerobika left a comment

Choose a reason for hiding this comment

galpeter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lygstate commented Jan 15, 2021

lygstate commented Jan 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rerobika left a comment

Choose a reason for hiding this comment

zherczeg left a comment

Choose a reason for hiding this comment

lygstate commented Jan 15, 2021 •

edited

Loading