Fix for toFixed method #1994

rerobika · 2017-09-04T12:03:41Z

This patch fixes this bug which caused corrupted stack by preventing unnecessary double to ascii conversion even if
the convertable number of digits is higher than allowed.
In addition, improved ecma_double_to_binary_floating_point function by removing a needless buffer.

JerryScript-DCO-1.0-Signed-off-by: Robert Fancsik frobert@inf.u-szeged.hu

robertsipka · 2017-09-04T13:16:12Z

jerry-core/ecma/base/ecma-helpers-conversion.c

  {
-    ecma_double_to_ascii (val, buffer_p, &char_cnt);
+    if (num_of_digits < 22)
+    {


Why 22? Please do not use magic number.

This magic number comes from here
https://github.com/jerryscript-project/jerryscript/blob/master/jerry-core/ecma/builtin-objects/ecma-builtin-number-prototype.c#L593

But I'm going to define a variable for it.

Great, thanks. Could you define that magic number 21 somewhere where both files reach it (and you can replace the condition to <= here) ?

Of course. :)

akosthekiss · 2017-09-11T16:48:58Z

jerry-core/ecma/base/ecma-globals.h


+/**
+ * Maximum number of digits in string representation of ecma-number exponent part
+ */


If we introduce such a constant then it should be used throughout the codebase. I've grepped for 21 under jerry-core/ecma and found other occurrences:

ecma-builtin-number-prototype.c:
L585 (ecma_builtin_number_prototype_object_to_fixed)
L727 (ecma_builtin_number_prototype_object_to_exponential)
L915 (ecma_builtin_number_prototype_object_to_precision)

ecma-helpers-conversion.c:
L1134-1138 (ecma_number_to_utf8_string)
L1146-1150 (ecma_number_to_utf8_string)

It should be thoroughly checked of course whether these 21s are the same 21 that we try to replace. But if they are, the new macro should be used in place of them.

After some discussion https://www.ecma-international.org/ecma-262/5.1/#sec-15.7.4.5 does not provide precise explanation about these 'magic numbers', so we can keep them instead of defining a new macro for them.

@rerobika For the sake of clarity: so, none of the above 5 instances of the numeric literal 21 have any connection with the "maximum number of digits in string representation of ecma-number exponent part" concept? But the functions already touched in the current patch (ecma_double_to_binary_floating_point and ecma_builtin_number_prototype_object_to_fixed) do use this concept? Seems strange to me.

OK, just checked ES5.1 15.7.4.5 (Number.prototype.toFixed) step 7 (currently touched). No explanation there either. So, the patch, as is now, introduces a new macro and inconsistently uses it in the code base. I start to get the feeling that we don't need this macro at all. Magic constants in the code are evil for sure, but such inconsistencies are even worse, IMHO.

(BTW, it seems to me that the current macro name + docstring are misleading. There is no way that we have 21 digits in the exponent. In double precision floating point, exponent is 11 bits, which is 4 base-10 digits at most.)

Summary: I think we should revert to NOT introducing a new macro and NOT touching other parts of the code base but where the bug is fixed.

Yeah, it seems like a better idea.

rerobika · 2017-09-21T09:23:47Z

@akosthekiss I've updated this PR, please review.

akosthekiss · 2017-10-02T12:34:22Z

jerry-core/ecma/base/ecma-helpers-conversion.c

+    }
+    else
+    {
+      /** NOTE: In this case no further conversations are required, execution will continue by 'toString' method */


I think that a more precise cross reference would help a lot here. It took me quite some time to dig out from the code why toString will be called later on in this case. It's because this ecma_double_to_binary_floating_point seems to be called (transitively) from ecma_builtin_number_prototype_object_to_fixed only in step 6., and in step 7., the exponent (here: *exp_p) is checked whether its larger than 21. (And if it's larger, then the value returned and the buffer written by this function are discarded, and ecma_builtin_number_prototype_object_to_string is called instead. So, I think it's definitely worth noting here that the number 21 above is to be kept in sync with the number 21 in ecma_builtin_number_prototype_object_to_fixed step 7.

I'll give more detailed information about this cross reference to make this code easier to understand.

akosthekiss · 2017-10-02T12:41:40Z

jerry-core/ecma/base/ecma-helpers-conversion.c

  else
  {
-    ecma_double_to_ascii (val, buffer_p, &char_cnt);
+    if (num_of_digits <= 21)


I think we can use the else if chain here, too. I.e.,

if (fabs (integer_part) < EPSILON) else if (integer_part < 10e16) else if (num_of_digits <= 21) else

(instead of nesting an if/else in the final else).

BTW, now that we have num_of_digits in this function, do we need integer_part < 10e16 in the second if or can we have num_of_digits <= 16? Integer comparisons should be faster than FP, in general.

That's a good idea.

rerobika · 2017-11-06T15:14:38Z

Patch is fixed and I've added more precise description.

akosthekiss · 2017-11-07T16:10:19Z

tests/jerry/regression-test-issue-1993.js

+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+assert(parseFloat(Number.MAX_VALUE).toFixed(5) == 1.7976931348623157e+308);


Hardcoding the numeric constant into the test may not be the best approach. Couldn't we check with assert(parseFloat(Number.MAX_VALUE).toFixed(5) == parseFloat(Number.MAX_VALUE).toString());? Seems to be a more maintainable approach to me.

akosthekiss · 2017-11-07T16:47:41Z

jerry-core/ecma/base/ecma-helpers-conversion.c

  {
-    ecma_double_to_ascii (val, buffer_p, &char_cnt);
+    /**
+     * According to ECMA-262 v5, 15.7.4.5 7th step: if x ≥ 10^21, then execution will continue by


We shouldn't use non-ASCII characters in comments (or anywhere in the sources, actually). ≥ is U+2265. I think, >= would work just as well.

akosthekiss · 2017-11-07T16:55:31Z

jerry-core/ecma/base/ecma-helpers-conversion.c

  else
  {
-    ecma_double_to_ascii (val, buffer_p, &char_cnt);
+    /**


This is not (should not be) a doxygen comment. Just use simple /*. (/** is for documenting functions, arguments, structs, fields, macros, etc. Not for commenting code inside a functions.)

akosthekiss · 2017-11-07T16:56:10Z

jerry-core/ecma/base/ecma-helpers-conversion.c

+     * method 7th step.
+     */
+    *exp_p = num_of_digits;
+    return (lit_utf8_size_t) char_cnt;


Is this a bit convoluted way of saying return 0;?

rerobika · 2017-11-08T14:08:21Z

I can agree with your suggestions above. According to them I've updated this PR.

akosthekiss

LGTM (it would be nice to have the wording of the comment text fixed before merging, though)

akosthekiss · 2017-11-09T16:07:25Z

jerry-core/ecma/base/ecma-helpers-conversion.c

+     /* According to ECMA-262 v5, 15.7.4.5 7th step: if x >= 10^21, then execution will continue by
+      * ToString(x) so in this case no further conversations are required. Number 21 in the else if condition
+      * above must be kept in sync with the number 21 in ecma_builtin_number_prototype_object_to_fixed
+      * method 7th step. */


minor text improvement suggestions:

"ECMA-262 v5, 15.7.4.5 7th step" -> "ECMA-262 v5, 15.7.4.5, step 7"

"continue by" -> "continue with"

"conversations" -> "conversions"

"ecma_builtin_number_prototype_object_to_fixed method 7th step." -> "function ecma_builtin_number_prototype_object_to_fixed, step 7."

Thanks for your suggestions. I've applied them!

akosthekiss · 2017-11-10T12:35:40Z

still LGTM

This patch fixes this bug which caused corrupted stack by preventing unnecessary double to ascii conversion even if the convertible number of digits is higher than allowed. In addition, improved ecma_double_to_binary_floating_point function by removing a needless buffer. JerryScript-DCO-1.0-Signed-off-by: Robert Fancsik frobert@inf.u-szeged.hu

LaszloLango · 2017-11-10T12:37:52Z

jerry-core/ecma/base/ecma-helpers-conversion.c

  else
  {
-    ecma_double_to_ascii (val, buffer_p, &char_cnt);
+     /* According to ECMA-262 v5, 15.7.4.5, step 7: if x >= 10^21, then execution will continue with


wrong indentation

LaszloLango

LGTM

robertsipka reviewed Sep 4, 2017

View reviewed changes

rerobika changed the title ~~Fix for issue #1993~~ Fix for toFixed method Sep 5, 2017

rerobika force-pushed the issue_1993 branch 2 times, most recently from daeac38 to 7881cc3 Compare September 8, 2017 14:18

akosthekiss reviewed Sep 11, 2017

View reviewed changes

rerobika force-pushed the issue_1993 branch from 7881cc3 to 9b79d5f Compare September 21, 2017 09:21

akosthekiss requested changes Oct 2, 2017

View reviewed changes

rerobika force-pushed the issue_1993 branch from 9b79d5f to 2305f45 Compare November 6, 2017 10:00

akosthekiss requested changes Nov 7, 2017

View reviewed changes

rerobika force-pushed the issue_1993 branch from 2305f45 to 1db5c15 Compare November 8, 2017 14:07

akosthekiss approved these changes Nov 9, 2017

View reviewed changes

rerobika force-pushed the issue_1993 branch from 1db5c15 to 66e89e7 Compare November 10, 2017 12:31

LaszloLango requested changes Nov 10, 2017

View reviewed changes

rerobika force-pushed the issue_1993 branch from 66e89e7 to 7826f2c Compare November 10, 2017 12:38

LaszloLango approved these changes Nov 10, 2017

View reviewed changes

akosthekiss merged commit 5d2000c into jerryscript-project:master Nov 10, 2017

akosthekiss mentioned this pull request Nov 10, 2017

Segmentation fault in ecma_builtin_number_prototype_object_to_fixed #1993

Closed

zherczeg mentioned this pull request Mar 27, 2018

Nominating Robert Sipka (robertsipka) to become a reviewer #2254

Closed

rerobika deleted the issue_1993 branch April 16, 2018 10:52

Fix for toFixed method #1994

Fix for toFixed method #1994

Uh oh!

Conversation

rerobika commented Sep 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robertsipka Sep 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rerobika commented Sep 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rerobika commented Nov 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rerobika commented Nov 8, 2017

Uh oh!

akosthekiss left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akosthekiss commented Nov 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LaszloLango left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

robertsipka Sep 5, 2017 •

edited

Loading