Closed Bug 369418 Opened 14 years ago Closed 12 years ago

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

Categories

(Core :: SVG, defect, P2)

Product:
Core
Component:
SVG
Type:
defect

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: jruderman, Assigned: jwatt)

Details

(Keywords: assertion, crash, testcase)

Attachments

(5 files, 4 obsolete files)

testcase
434 bytes, image/svg+xml
Details
seamonkey-gdb-run.txt
143.94 KB, text/plain
Details
patch - with configure check
58.71 KB, patch
roc
: review+
roc
: superreview+
Details | Diff | Splinter Review
configure checks
6.01 KB, patch
benjamin
: review-
Details | Diff | Splinter Review
patch - back to JSDOUBLE_IS_FINITE
53.85 KB, patch
roc
: review+
roc
: superreview+
Details | Diff | Splinter Review
Attached image 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.
Also, "Foo" and "Bar" seem to disappear as a result of removing the text node "Z", which doesn't seem right.
Flags: blocking1.9?
This actually crashes for me on WinXP.
Keywords: crash
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.
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.
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.
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
Thanks guys. I looked in NSPR but but not in gfx for some reason. ;-)
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;
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.
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.  ;)
I still see this bug on trunk, with all three assertions.
Flags: wanted1.9+
Flags: blocking1.9?
Flags: blocking1.9-
Attached file 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
If you're seeing this assertion on startup, you might be hitting a different bug.
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). ;)
Assignee: general → jwatt
Status: NEW → ASSIGNED
OS: Mac OS X → All
Hardware: PC → All
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.
Attachment #307192 - Flags: review?(roc)
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.
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.
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.
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.
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?
Attached patch patch (obsolete) — — Splinter Review
Attachment #307192 - Attachment is obsolete: true
Attachment #307192 - Flags: review?(roc)
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.
f - f == 0 sounds good to me.

Why make rv a macro parameter? Isn't NS_ERROR_ILLEGAL_VALUE always what you want?
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.
Dunno if you can give sr as well roc or if I should get another reviewer.
Attachment #307940 - Attachment is obsolete: true
Attachment #308049 - Flags: review?(roc)
> 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.
+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?
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
(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.

Something to think about when we rev XPConnect, though.
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.
roc: can you file that bug on xpconnect, so it's not lost?

jwatt: can you add a configure test for NaN == 0?
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
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.
Attachment #308049 - Attachment is obsolete: true
Attachment #308195 - Flags: review?(roc)
Attachment #308049 - Flags: review?(roc)
Oops, no, I removed the static assertion. I just forgot to remove that sentence before hitting submit. :-)
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.
We should block on this since we have a patch in hand.
Flags: tracking1.9- → tracking1.9+
Comment on attachment 308195 [details] [diff] [review]
patch - with configure check

Can you review the configure test, bsmedberg? Note comment 35.
Attachment #308195 - Flags: review?(benjamin)
(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.
Flags: tracking1.9+ → blocking1.9+
Thanks for the confirmation.
Attachment #308195 - Flags: review?(benjamin)
Attached patch new configure check (obsolete) — — Splinter Review 
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())
Attachment #308411 - Flags: review?
Attachment #308411 - Flags: review? → review?(benjamin)
How about (0.0 / 0.0)?

/be
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.


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
I.e. /fp:precise is the default for VC8 as noted in comment 37.
vc7.1 fails on runtime checks if you use /O1 or /O2 unless you explicitly set /Op too.
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 on attachment 308411 [details] [diff] [review]
new configure check

The TryServer happily builds to completion on all platforms with this configure.in patch.
Setting priority to P1 since we may want this change in a beta.
Priority: -- → P1
(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.


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
Priority: P1 → --
We're talking about turning /Op on, not off, so that shouldn't be an issue.
Priority: -- → P1
Comment on attachment 308195 [details] [diff] [review]
patch - with configure check

fine with me, but I leave the configure part to Benjamin
Attachment #308195 - Flags: superreview+
Attachment #308195 - Flags: review?(roc)
Attachment #308195 - Flags: review+
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.
#pragma optimize("p", on)

works. off does not.
Attached patch configure checks — — Splinter Review 
Thanks for testing. In that case I think we should do this.
Attachment #308411 - Attachment is obsolete: true
Attachment #308591 - Flags: review?(benjamin)
Attachment #308411 - Flags: review?(benjamin)
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)
Attachment #308591 - Flags: review?(benjamin) → review-
(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.
> 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.
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.
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?
> 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.
Yeah, if there's any doubt then we should play it safe. Thanks for the info.
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?
We're doing the JSDOUBLE_IS_FINITE thing now, so we can take this after the beta.
Priority: P1 → P2
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.
Attachment #314080 - Flags: superreview?(roc)
Attachment #314080 - Flags: review?(roc)
Attachment #314080 - Flags: superreview?(roc)
Attachment #314080 - Flags: superreview+
Attachment #314080 - Flags: review?(roc)
Attachment #314080 - Flags: review+
Checked in.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Summary: "ASSERTION: reflow state computed incorrect width" and more assertions (testcase uses foreignObject and currentScale) → Prevent script from injecting floating point infinity or NaN values into C++ land through DOM interfaces
You need to log in before you can comment on or make changes to this bug.