369418 - Prevent script from injecting floating point infinity or NaN values into C++ land through DOM interfaces

Status: RESOLVED FIXED

Description

[Jesse Ruderman]

Attachment: testcase

Loading the testcase triggers three assertions:

###!!! ASSERTION: reflow state computed incorrect width: 'reflowState.ComputedWidth() == size.width - reflowState.mComputedBorderPadding.LeftRight()', file /Users/jruderman/trunk/mozilla/layout/base/nsPresShell.cpp, line 6065

###!!! ASSERTION: reflow roots should be reflown at existing size and svg.css should ensure we have no padding/border/margin: 'aReflowState.ComputedWidth() == GetSize().width && aReflowState.mComputedHeight == GetSize().height', file /Users/jruderman/trunk/mozilla/layout/svg/base/src/nsSVGForeignObjectFrame.cpp, line 183

###!!! ASSERTION: non-root frame's desired size changed during an incremental reflow: 'target == root || (desiredSize.width == size.width && desiredSize.height == size.height)', file /Users/jruderman/trunk/mozilla/layout/base/nsPresShell.cpp, line 6080

The third assertion also shows up in bug 366791.

Comment 1

[Jesse Ruderman]

Also, "Foo" and "Bar" seem to disappear as a result of removing the text node "Z", which doesn't seem right.

Comment 2

[Jonathan Watt [:jwatt]]

This actually crashes for me on WinXP.

Comment 3

[Jonathan Watt [:jwatt]]

So this all seems to be down to getting junk passed in to nsSVGSVGElement::SetCurrentScale from XPC land.

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/svg/content/src/nsSVGSVGElement.cpp&rev=1.81&mark=361#359

CC'ing some people who might know something about XPC code. XPC should hopefully do something sensible when an alphabetical character is assigned to an |attribute float currentScale| since there's no way the interface implementing code can know that has happened. (Perhaps just pass in zero?)

The crash I get is in cairo_win32_scaled_font_select_font BTW, but that's not so relevant since we're working on junk data.

Comment 4

[timeless]

i don't see why it matters, if someone can send you a random float you should deal. whether or not letters are converted to random floats.

Comment 5

[Jonathan Watt [:jwatt]]

If someone sends us a random float we should deal. We compare it to the max and min acceptable values and clamp if it's outside them. Any comparison against the value "1.#QNAN00" seems to always return false though, so we never clamp and the junk value get's passed through. If anyone knows _how_ to check for a value like this I'm more than happy to add those checks to SVG code.

Comment 6

[Brendan Eich [:brendan]]

NaNs are unordered with respect to all IEEE doubles including other NaNs. You have to detect them with tools such as fpclassify(3) (man isnan on a Mac) or similar.

/be

Comment 7

[Boris Zbarsky [PTO through Aug 25, :bzbarsky, bz on IRC]]

See http://lxr.mozilla.org/seamonkey/source/gfx/public/nsCoord.h#68

Comment 8

[Jonathan Watt [:jwatt]]

Thanks guys. I looked in NSPR but but not in gfx for some reason. ;-)

Comment 9

[Jesse Ruderman]

Instead of looking for NaN specifically, maybe you could invert the tests, using the fact that all comparisons with NaN return false to your advantage:

-   if (aCurrentScale < CURRENT_SCALE_MIN)
+  if (!(aCurrentScale >= CURRENT_SCALE_MIN))
       aCurrentScale = CURRENT_SCALE_MIN;

-   else if (aCurrentScale > CURRENT_SCALE_MAX)
+  if (!(aCurrentScale <= CURRENT_SCALE_MAX))
       aCurrentScale = CURRENT_SCALE_MAX;

Comment 10

[Jonathan Watt [:jwatt]]

I see that nsCanvasRenderingContext2D uses JSDOUBLE_IS_FINITE to validate the floats passed in to it.

To my surprise there are no other DOM interfaces with float as an input, so it seems that only SVG is a problem.

Jesse, it's a cleaver idea, but I'd rather keep the code self documenting and clear that we need to check the validity of the float passed in. Else you can be sure that somewhere (there are many places in the SVG code we'll need to do this) someone will screw up the logic sooner or later.

Comment 11

[Boris Zbarsky [PTO through Aug 25, :bzbarsky, bz on IRC]]

Right.  Floats can also be infinite, not just NaN.

And yes, there are no other DOM float APIs.  XML doesn't really have much of a concept of "float" associated with it, unlike graphics.  ;)

Comment 12

[Jesse Ruderman]

I still see this bug on trunk, with all three assertions.

Comment 13

[Martin Mokrejs]

Attachment: seamonkey-gdb-run.txt

Still happens with TRUNK during startup:

Build identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b2pre) Gecko/2007112921 SeaMonkey/2.0a1pre

Comment 14

[Jesse Ruderman]

If you're seeing this assertion on startup, you might be hitting a different bug.

Comment 15

[Martin Mokrejs]

Unfortunately seamonkey terminates under gdb after a while due to some timeout between gdb commands. So, I cannot run the testcase now (or at least I think so). ;)

Comment 16

[Jonathan Watt [:jwatt]]

Attachment: patch that should be good enough for 1.9

This is a version of an old patch I had hanging around tidied up to use the JS macro. We should probably have one centralized method for doing checking finiteness, but for now this should do for 1.9. We might also be better off creating a non-virtual ellipse function that expects float* (it might reduce the amount of code generated for each call point). Also I'm not totally convinced that this check shouldn't happen in a single place in XPConnect before it calls methods it passes floats to.

Comment 17

[Jonathan Watt [:jwatt]]

roc: for reviewing, FLOAT_IS_FINITE is defined in mozilla/layout/svg/base/src/nsSVGUtils.h (end of patch) the rest is just checks calling that.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Can we copy the definition of JSDOUBLE_IS_FINITE to nsMathUtils, call it NS_FLOAT_IS_FINITE and use it from there?

How about an NS_ENSURE_FINITE(x) macro that does "if (!FLOAT_IS_FINITE) return NS_ERROR_ILLEGAL_VALUE;"?

Hmm, I wonder if it would make more sense for XPConnect to verify that floats are finite, and define a new IDL type for floats that includes NaNs and infinities? Would be less code, that's for sure.

Comment 19

[Jonathan Watt [:jwatt]]

roc: can we do that tidying after 1.9, and focus on other things for now? Besides that, copying the macro means divergence as changes are made to add processor X or whatever, and having a return statement for each check means more code at each call point I'd think.

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I doubt it will lead to more generated code.

I'd really like to change this now before we land a big patch which we then have to fix later. This is not going to be difficult.

If you don't want to duplicate the macro, put NS_ENSURE_FINITE in nsContentUtils.h for now and we can move it later without trouble if we want to.

Comment 21

[Jonathan Watt [:jwatt]]

Okay.

It occurred to me that the rules for IEEE floats should allow us to check a number is finite by doing:

  bool isFinite(float f) { return f - f == 0; }

since |infinity - infinity == NaN| and any comparison where one of the operands is NaN will result in false. At least in content land |float| is always an IEEE float, right?

Comment 22

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 23

[Jonathan Watt [:jwatt]]

Err, ignore the PR_BEGIN_MACRO/PR_END_MACRO lines.

If the |f - f == 0| thing is okay I'll move the code into nsMathUtils.h as requested. If not I'll leave it in nsContentUtils.h and use JSDOUBLE_IS_FINITE.

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

f - f == 0 sounds good to me.

Why make rv a macro parameter? Isn't NS_ERROR_ILLEGAL_VALUE always what you want?

Comment 25

[Jonathan Watt [:jwatt]]

So we don't have to rewrite all macros call points if we want to use that macro in a function that returns void, or if we want a different value somewhere else. If we're putting it in nsContentUtils.h/nsMathUtils.h instead of nsSVGUtils.h then I assume that's because we think it might be useful somewhere else at some point. It also matches the NS_ENSURE_XXX macros which people are familiar with, and people see what's returned at the call point without looking up the macro.

Comment 26

[Jonathan Watt [:jwatt]]

Attachment: patch - macros moved to nsMathUtils.h

Dunno if you can give sr as well roc or if I should get another reviewer.

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

> So we don't have to rewrite all macros call points if we want to use that macro
> in a function that returns void, or if we want a different value somewhere
> else.

We could write a new macro in that case. But OK.

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+static NS_HIDDEN_(PRBool) NS_FloatIsFinite(float f) { return f - f == 0.0f; }
+static NS_HIDDEN_(PRBool) NS_FloatIsFinite(double f) { return f - f == 0.0; }

These should be "inline" otherwise you'll get a huge number of warnings.

But let's think about the XPConnect alternative. Brendan, if you're listening, what do you think about having XPConnect throw when non-finite values are passed to DOM interfaces?

Comment 29

[Brendan Eich [:brendan]]

Have you tested to make sure that a long-standing MSVC bug found early in JS's lifetime, where NaN == 0, is not going to bite f - f == 0? See around line 3735 in jsinterp.c, e.g. (grep for "MSVC miscompiles").

If you want to use jsnum.h, it's exported. Feel free -- or copy, as there's a extremely small chance we'll evolve to support some new platform.

I don't think changing XPConnect to throw is a good idea this late in the game. It may be strictly better for all code, but would we find out in time? JS supports non-finite values and possibly some web or intranet XUL app out there depends on them getting through an XPConnected API.

/be

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #29)
> Have you tested to make sure that a long-standing MSVC bug found early in JS's
> lifetime, where NaN == 0, is not going to bite f - f == 0? See around line 3735
> in jsinterp.c, e.g. (grep for "MSVC miscompiles").

Interesting, hopefully jwatt can test that.

> If you want to use jsnum.h, it's exported. Feel free -- or copy, as there's a
> extremely small chance we'll evolve to support some new platform.

We probably wouldn't want to use that from, say, gfx/thebes.

> I don't think changing XPConnect to throw is a good idea this late in the game.
> It may be strictly better for all code, but would we find out in time? JS
> supports non-finite values and possibly some web or intranet XUL app out there
> depends on them getting through an XPConnected API.

Yeah OK.

Comment 31

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Something to think about when we rev XPConnect, though.

Comment 32

[Jonathan Watt [:jwatt]]

Good catch on the |static -> inline|, I forgot to change that.

So the |NaN == 0| bug is no longer present in VC8, but we also have VC7.1 listed as supported on the build prerequisites page. I'll need to find someone to test that.

Comment 33

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

roc: can you file that bug on xpconnect, so it's not lost?

jwatt: can you add a configure test for NaN == 0?

Comment 34

[Jonathan Watt [:jwatt]]

Shaver: will do.

I haven't managed to actually test if the NaN bug is present in VC7.1, but the VS 2003 docs contain a page under the Lexical Conventions section which claims it follows the IEEE rules:

  http://msdn2.microsoft.com/en-us/library/w22adx1s(VS.71).aspx

The Lexical Conventions section in the VC6 docs has no such page on the other hand:

  http://msdn2.microsoft.com/en-us/library/aa314452(VS.60).aspx

Comment 35

[Jonathan Watt [:jwatt]]

Attachment: patch - with configure check

I had a few problems writing the configure test. Initially my test wouldn't run, which turned out to be because we set SKIP_COMPILER_CHECKS in configure.in if _WIN32_MSVC is set. (Yikes!?) After moving my test outside the normal compiler checks section of configure.in, it ran, but failed. It turned out this was because $LIBS is passed to cl on the command line, but cl interprets the libs to be source files that it should compile. I guess we really haven't designed configure.in to run compiler checks for MSVC. :-(

Anyways, rather than fight, I've temporarily undefined LIBS for MSVC for this test (they're not needed anyway). I've also added a static assertion to nsMathUtils.h.

Comment 36

[Jonathan Watt [:jwatt]]

Oops, no, I removed the static assertion. I just forgot to remove that sentence before hitting submit. :-)

Comment 37

[Jonathan Watt [:jwatt]]

Other MSDN docs of interest are the doc on the /fp option (which states that the IEEE floating point behavior is the default) and the doc on the the float_control pragma:

  http://msdn2.microsoft.com/en-us/library/e7s85ffb(VS.80).aspx
  http://msdn2.microsoft.com/en-us/library/45ec64h6(VS.80).aspx

Neither of these seem to exist in VC7.1 though, only in VC8.

Comment 38

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Filed bug 421825.

Comment 39

[Jonathan Watt [:jwatt]]

We should block on this since we have a patch in hand.

Comment 40

[Jonathan Watt [:jwatt]]

Comment on attachment 308195 [details] [diff] [review]
patch - with configure check

Can you review the configure test, bsmedberg? Note comment 35.

Comment 41

[Robert Longson [:longsonr]]

(In reply to comment #34)

> I haven't managed to actually test if the NaN bug is present in VC7.1, but the
> VS 2003 docs contain a page under the Lexical Conventions section which claims
> it follows the IEEE rules:
> 

FWIW Visual C++ 7.1 works i.e. it returns 0 from the configure check.

Comment 42

[Jonathan Watt [:jwatt]]

Thanks for the confirmation.

Comment 43

[Jonathan Watt [:jwatt]]

Attachment: new configure check

luser pointed out that AC_TRY_RUN checks are undesirable since they break cross compilation. This patch changes the configure test to fall back to static assertions if we're doing a cross compile. See the comments inline.

I tried a bunch of other ways to get a compile time NaN (including the following) without success:

// VC8 resolves this to zero:
#define NAN ((FLT_MAX+1)-(FLT_MAX+1))

// Not constant, won't compile:
#define NAN (std::numeric_limits<float>::quiet_NaN())

Comment 44

[Brendan Eich [:brendan]]

How about (0.0 / 0.0)?

/be

Comment 45

[Robert Longson [:longsonr]]

Do you need to make sure that /fp:precise (or for VC7.1 /Op) is used when compiling per http://msdn2.microsoft.com/en-us/library/e7s85ffb(VS.80).aspx ?

Given your definition of NAN for the static assert, VC7.1 fails the first, third and final two tests without /Op and fails all but the last test with /Op

(0.0/0.0) fails completely on VC7.1 The compiler claims we are no longer dealing with constant expressions.

Comment 46

[Jonathan Watt [:jwatt]]

Brendan: yeah, as Robert says, VC7.1/VC8 spit out "error C2057: expected constant expression" for (0.0/0.0) and similar. (So it's even weirder that the float(...) method works). Anyway, we have a solution for this point, so let's not worry about it. I was just saying there's a reason I did it the way I did.

Robert: I'm not sure that the static assertion case failing even with /Op is a problem, since we only do the static assert if we're cross compiling. Does anyone do cross compiling with VC7.1?

Your comment 41 seems to suggest that it's also not necessary do define /Op for the runtime checks either. If that's not correct, I guess we'd be alright defining /Op system wide for VC7.1 (it's not the official compiler) since that's really the default and what you get when you use VC8 (the official compiler). In fact it may be necessary even if we take the JSDOUBLE_IS_FINITE route since the JS code defines that (and needs it for the macro to work?):

http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/js/src/Makefile.in&rev=3.119&mark=299,301#295

The VC7.1 /Op doc: http://msdn2.microsoft.com/en-us/library/aa984742(VS.71).aspx

Comment 47

[Jonathan Watt [:jwatt]]

I.e. /fp:precise is the default for VC8 as noted in comment 37.

Comment 48

[Robert Longson [:longsonr]]

vc7.1 fails on runtime checks if you use /O1 or /O2 unless you explicitly set /Op too.

Comment 49

[Jonathan Watt [:jwatt]]

Those aren't default options though, right, so that change would be exactly the sort of thing the runtime check is designed to catch. I guess it may be an argument for making VC7.1 users' lives easier by setting /Op for the entire build though.

Comment 50

[Jonathan Watt [:jwatt]]

Comment on attachment 308411 [details] [diff] [review]
new configure check

The TryServer happily builds to completion on all platforms with this configure.in patch.

Comment 51

[Jonathan Watt [:jwatt]]

Setting priority to P1 since we may want this change in a beta.

Comment 52

[Robert Longson [:longsonr]]

(In reply to comment #49)
> Those aren't default options though, right, so that change would be exactly the
> sort of thing the runtime check is designed to catch. I guess it may be an
> argument for making VC7.1 users' lives easier by setting /Op for the entire
> build though.
> 

Presumably if you did 

ac_add_options --enable-optimize

your build would not work unless --enable-optimise does /Op for vc7.1

I haven't checked whether it does or not.

Comment 53

[David Bradley]

You need to be careful and not cause the compiler to use less than precise logic for floating point as this will cause the JS double to string conversion to have problems. Or make sure the JS C file that contains that logic is compiled with that option set

Comment 54

[Jonathan Watt [:jwatt]]

We're talking about turning /Op on, not off, so that shouldn't be an issue.

Comment 55

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 308195 [details] [diff] [review]
patch - with configure check

fine with me, but I leave the configure part to Benjamin

Comment 56

[Jonathan Watt [:jwatt]]

Cool, thanks.

longsonr: digging into the MS docs further it seems that you actually can turn the FP optimization off in VC7.1 using a pragma (strange that the page on /Op doesn't mention this).

http://msdn2.microsoft.com/en-us/library/chh3fb0k(VS.71).aspx

Can you check compilation with the following:

#pragma optimize("p", off)

I'm not quite sure from the doc whether that should be 'off' or 'on' actually. :-) Note that it has to come before the function it applies to so, in the static case, it should go as the last line in the includes/defines block.

Comment 57

[Robert Longson [:longsonr]]

#pragma optimize("p", on)

works. off does not.

Comment 58

[Jonathan Watt [:jwatt]]

Attachment: configure checks

Thanks for testing. In that case I think we should do this.

Comment 59

[Benjamin Smedberg]

Comment on attachment 308591 [details] [diff] [review]
configure checks

1) whatever else below, the configure check would need to be in the SKIP_COMPILER_CHECKS block

2) Since we know what MSVC does, we should just hardcode the results in that case, rather than having this unholy hybrid of AC_TRY_RUN and AC_TRY_COMPILE

3) I don't want a new AC_TRY_RUN, period. If we need something like that we should just have a runtime assertion for the conditions being tested.

4) You can do MOZ_OPTIMIZE_FLAGS += -Op even if MOZ_OPTIMIZE_FLAGS aren't previously set, and avoid the extra ifdef

5) -Op shouldn't be in MOZ_OPTIMIZE_FLAGS, it should be in the default flags (so probably OS_CFLAGS)

Comment 60

[Jonathan Watt [:jwatt]]

(In reply to comment #59)
> (From update of attachment 308591 [details] [diff] [review])
> 1) whatever else below, the configure check would need to be in the
> SKIP_COMPILER_CHECKS block

As noted in comment 35, SKIP_COMPILER_CHECKS is set if we're compiling with MSVC, period. Do you want to skip the check for MSVC?

> 2) Since we know what MSVC does, we should just hardcode the results in that
> case, rather than having this unholy hybrid of AC_TRY_RUN and AC_TRY_COMPILE

The check-with-fallback-check is not just for MSVC. As explained in the comment in the patch, there's a good reason for it that may not be unique to MSVC: we can't rely on floating point numbers to be handled in the same way at compile time as they are handled during run time. Using AC_TRY_RUN when we can with a fallback to AC_TRY_COMPILE when cross compiling provides a more robust check (and thus seemed to be the correct thing to do). I'm not sure why you would have such a strong reaction against the combination.

> 3) I don't want a new AC_TRY_RUN, period. If we need something like that we
> should just have a runtime assertion for the conditions being tested.

You mean a check that is run when mozilla starts up, and which calls abort() if it fails? Where would you suggest putting such a check? Do we really want to ship checking code?

> 4) You can do MOZ_OPTIMIZE_FLAGS += -Op even if MOZ_OPTIMIZE_FLAGS aren't
> previously set, and avoid the extra ifdef

Great, I wasn't sure. Thanks.

> 5) -Op shouldn't be in MOZ_OPTIMIZE_FLAGS, it should be in the default flags
> (so probably OS_CFLAGS)

Okay. I put it there since it's a -O option, but I don't have a preference.

Comment 61

[Benjamin Smedberg]

> As noted in comment 35, SKIP_COMPILER_CHECKS is set if we're compiling with
> MSVC, period. Do you want to skip the check for MSVC?

SKIP_COMPILER_CHECKS is also used so that we can configure l10n repackaging without a compiler. So yes, or another alternative needs to be found...

> (and thus seemed to be the correct thing to do). I'm not sure why you would
> have such a strong reaction against the combination.

Becuase it causes configure to have different results when direct-compiling and cross-compiling. Also because we're working towards a new configuration system and would like to avoid reimplementing AC_TRY_RUN.

> You mean a check that is run when mozilla starts up, and which calls abort() if
> it fails? Where would you suggest putting such a check? Do we really want to
> ship checking code?

That, or a debug-only assertion. You could put it in pretty much any run-once initialization function, so I'd suggest nsLayoutModule.cpp:Initialize.

Comment 62

[Jonathan Watt [:jwatt]]

Okay. A debug-only assertion won't work since it's the turning on of optimization in release builds that is most likely to cause problems. I'll see what I can come up with.

Comment 63

[Jonathan Watt [:jwatt]]

Section 10.7 of the GCC 4.3.0 manual says:

-------------------------------------------------------------------------------
On 68000 and x86 systems, for instance, you can get paradoxical results if you
test the precise values of floating point numbers. For example, you can find
that a floating point value which is not a NaN is not equal to itself. This
results from the fact that the floating point registers hold a few more bits of
precision than fit in a double in memory. Compiled code moves values between
memory and floating point registers at its convenience, and moving them into
memory truncates them.

You can partially avoid this problem by using the '-ffloat-store' option (see
Section 3.10 [Optimize Options], page 77).
-------------------------------------------------------------------------------

http://gcc.gnu.org/onlinedocs/gcc-4.3.0/gcc/Disappointments.html#index-floating-point-precision-2897

In NS_FloatIsFinite our trick is to subtract the float from itself and then compare the result to zero (rather than directly compare the float to itself). I'm not sure if that means we're safe or not. I think it probably does, but if I'm wrong NS_FloatIsFinite would intermittently return spurious false negatives. Clearly we don't want scripts to fail randomly when false negatives cause us to throw an exception. roc, you're good at this low level stuff. What do you think?

Comment 64

[Robert O'Callahan (:roc) (email my personal email if necessary)]

> I'm not sure if that means we're safe or not.

Good catch! I think we're actually not safe, although in practice it would be very surprising if we got hit since the optimizer should get the value in a register and then use the register twice instead of using a load for one of the values.

Anyway, the only architecture that we support that supports extra hidden extra precision, as far as I know, is x86 using the x87 floating point instructions, which is 32-bit Windows and Linux/Unix. For those platforms we could use inline assembler to ensure we get the right code.

I think for now we should just use JSDOUBLE_IS_FINITE.

Comment 65

[Jonathan Watt [:jwatt]]

Yeah, if there's any doubt then we should play it safe. Thanks for the info.

Comment 66

[Damon Sicore (:damons)]

OK, So, comment #51 says we *may* want this in a beta.  Does the patch *require* a beta vector due to risk?  I'm going through each of the eight remaining SVG P1s which block Beta 5 and asking this question.  Eight SVG beta blockers seems like a lot to block the beta.  If we don't think this requires a beta cycle, we should move it to the P2 list and only focus on P1s.  Thoughts?

Comment 67

[Jonathan Watt [:jwatt]]

We're doing the JSDOUBLE_IS_FINITE thing now, so we can take this after the beta.

Comment 68

[Jonathan Watt [:jwatt]]

Attachment: patch - back to JSDOUBLE_IS_FINITE

I've also added to content/Makefile.in the compiler flags necessary to make JSDOUBLE_IS_FINITE work for VC7.1 users, as found in js/src/Makefile.in.

Comment 69

[Jonathan Watt [:jwatt]]

Checked in.