617734 - DoubleToCString is interesting

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[timeless]

http://mxr.mozilla.org/mozilla-central/ident?i=kBase10MaximalLength

js/src/v8-dtoa/dtoa.h
line 48 -- static const int kBase10MaximalLength = 17;
js/src/v8-dtoa/conversions.cc
line 72 -- const int kV8DtoaBufferCapacity = kBase10MaximalLength + 1;


http://mxr.mozilla.org/mozilla-central/source/js/src/v8-dtoa/conversions.cc?mark=79,87,94-98#68


68       int decimal_point;
69       int sign;
70       char* decimal_rep;
71       //bool used_gay_dtoa = false;     MOZ: see above
72       const int kV8DtoaBufferCapacity = kBase10MaximalLength + 1;

so kV8DtoaBufferCapacity = 18

73       char v8_dtoa_buffer[kV8DtoaBufferCapacity];

sizeof v8_dtoa_buffer = 18

74       int length;
75 
76       if (DoubleToAscii(v, DTOA_SHORTEST, 0,
77                         Vector<char>(v8_dtoa_buffer, kV8DtoaBufferCapacity),
78                         &sign, &length, &decimal_point)) {
79         decimal_rep = v8_dtoa_buffer;

so decimal_rep aliases v8_dtoa_buffer
80       } else {
81         return NULL;    // MOZ: see above
85       }
86 
87       if (sign) builder.AddCharacter('-');
88 
89       if (length <= decimal_point && decimal_point <= 21) {
94       } else if (0 < decimal_point && decimal_point <= 21) {

let decimal_point = 21;

coverity thinks we can enter here.

95         // ECMA-262 section 9.8.1 step 7.
96         builder.AddSubstring(decimal_rep, decimal_point);
97         builder.AddCharacter('.');

here we index past the 18th index of v8_dtoa_buffer:
98         builder.AddString(decimal_rep + decimal_point);

Comment 1

[Jason Orendorff [:jorendorff]]

Nick, would you please take a quick look? If bumping the buffer size to 22 makes the code more obviously, locally correct, I'm for it.

Comment 2

[Nicholas Nethercote [inactive]]

I think coverity is wrong, but the argument is moderately subtle.

dtoa.h says this:

  // The maximal length of digits a double can have in base 10.
  // Note that DoubleToAscii null-terminates its input. So the given buffer should
  // be at least kBase10MaximalLength + 1 characters long.
  static const int kBase10MaximalLength = 17;

The code starting at line 76 actually looks like this:

      if (DoubleToAscii(v, DTOA_SHORTEST, 0,
                        Vector<char>(v8_dtoa_buffer, kV8DtoaBufferCapacity),
                        &sign, &length, &decimal_point)) {
        decimal_rep = v8_dtoa_buffer;
      } else {
        return NULL;    // MOZ: see above
        //decimal_rep = dtoa(v, 0, 0, &decimal_point, &sign, NULL);
        //used_gay_dtoa = true;
        //length = StrLength(decimal_rep);
      }

I suspect that the kBase10MaximalLength is correct, but that Gay's dtoa (which is used in the else-branch) could return a length larger than 21.  Ie. the path that Coverity mentions isn't possible.

This all assumes that kBase10MaximalLength is correct.  I looked into DoubleToAscii() and its children, it's rather complicated, I was unable to verify that it's correct.  But the code does look very carefully written, and I know that the author is Florian Loitsch, who literally wrote the paper on the algorithm used by DoubleToAscii() ("Printing Floating-Point Numbers Quickly and Accurately with Integers", PLDI 2010.)  So I'd give it the benefit of the doubt.

Is there a way to suppress a Coverity warning?

Comment 3

[Jason Orendorff [:jorendorff]]

(In reply to comment #2)
> This all assumes that kBase10MaximalLength is correct.  I looked into
> DoubleToAscii() and its children, it's rather complicated, I was unable to
> verify that it's correct.

Yep, this is what happened to me too. :)

> But the code does look very carefully written, and I
> know that the author is Florian Loitsch, who literally wrote the paper on the
> algorithm used by DoubleToAscii() ("Printing Floating-Point Numbers Quickly and
> Accurately with Integers", PLDI 2010.)  So I'd give it the benefit of the
> doubt.

Well... I don't doubt the author's grasp of the algorithm. But, you know, the code isn't easy to reason about locally, and it's operating through pointers unchecked, and... you know, anyone can make a mistake. Plus we've hacked on this code a little bit, so who knows.

Even if it took somebody clever to write it, it shouldn't take someone cleverer than you, me, and Coverity combined to confirm that it's not going to randomly crash. We can do a little better. At least an assertion. Something.

Taking.

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: patch (against 58545:49f6b73ae373)

How's this?

Comment 5

[timeless]

Comment on attachment 496619 [details] [diff] [review]
patch (against 58545:49f6b73ae373)

i believe we're running Coverity against NDEBUG so using ASSERT is pointless.

Comment 6

[Nicholas Nethercote [inactive]]

So what's the point of this bug?  To work out if the code is correct, or to find a way to prevent Coverity from complaining?

Comment 7

[Jason Orendorff [:jorendorff]]

Comment on attachment 496619 [details] [diff] [review]
patch (against 58545:49f6b73ae373)

timeless wants to shut Coverity up. I want to be able to see the code is correct ...but I'll take an assertion over nothing.

Comment 8

[Daniel Veditz [:dveditz]]

so assuming nothing is wrong -> sg:nse and if the assertion fires then we'll file a sg:critical bug based on that?

Comment 9

[timeless]

we could go that way, alternatively we could use something which aborts in release builds, if we use something like that we would have a larger userbase to discover if this is a problem.

the question is really: how confident are we that we'll find problems first in our <debug build user space> before real users get in trouble?

Comment 10

[Josh Aas]

Is there any reason to keep this bug hidden?

Comment 11

[Jeff Walden [:Waldo]]

I'm not sure I'm comfortable unhiding it yet, going by the comments here.  :-\  This needs a generous dollop of elbow grease to get to the point of being confident of safety.  (Not sure what I think of landing an assertion and waiting to see if it fails, myself.  This probably reproduces [if indeed it does reproduce] only with a carefully-crafted testcase of the sort not likely to be found in random browsing and testing.)

Comment 12

[Chris Peterson [:cpeterson]]

njn: is your r+'d patch (from 2010) for this Coverity warning still relevant?

Comment 13

[Nicholas Nethercote [inactive]]

> njn: is your r+'d patch (from 2010) for this Coverity warning still relevant?

I think it's still relevant, but I was never that comfortable with it (and timeless gave it f-) which is why I didn't land it.

Comment 14

[Chris Peterson [:cpeterson]]

njn: can we just replace your patches ASSERT with MOZ_RELEASE_ASSERT to make everyone happy?

Comment 15

[Nicholas Nethercote [inactive]]

(In reply to Chris Peterson (:cpeterson) from comment #14)
> njn: can we just replace your patches ASSERT with MOZ_RELEASE_ASSERT to make
> everyone happy?

That sounds ok.

Comment 16

[Andrew McCreight [:mccr8]]

The double to string conversion code appears to live in MFBT now, though I don't see anything called DoubleToCString. I didn't dig very deep into it.