1589930 - Crash in [@ txFormatNumberFunctionCall::evaluate]

Status: RESOLVED FIXED

(Core :: XSLT, defect, P2)

Description

[Julian Seward [:jseward]]

This bug is for crash report bp-03bb4abb-2336-4fb6-9d04-e6c5f0191019.

This crash appears 8 times in 2 different instances in the Windows nightly
20191017093524.

Top 10 frames of crashing thread:

0 xul.dll nsresult txFormatNumberFunctionCall::evaluate dom/xslt/xslt/txFormatNumberFunctionCall.cpp
1 xul.dll Expr::evaluateToString dom/xslt/xpath/txExpr.cpp:20
2 xul.dll nsresult txCoreFunctionCall::evaluate dom/xslt/xpath/txCoreFunctionCall.cpp:423
3 xul.dll nsresult txValueOf::execute dom/xslt/xslt/txInstructions.cpp:767
4 xul.dll txXSLTProcessor::execute dom/xslt/xslt/txXSLTProcessor.cpp:37
5 xul.dll nsresult txMozillaXSLTProcessor::TransformToDoc dom/xslt/xslt/txMozillaXSLTProcessor.cpp:566
6 xul.dll nsresult nsTransformBlockerEvent::Run dom/xslt/xslt/txMozillaXSLTProcessor.cpp:458
7 xul.dll nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1225
8 xul.dll NS_ProcessNextEvent xpcom/threads/nsThreadUtils.cpp:486
9 xul.dll mozilla::ipc::MessagePump::Run ipc/glue/MessagePump.cpp:88

Comment 1

[Boris Zbarsky [:bzbarsky]]

The crash is due to someone getting the value of a CheckedInt that fails the mIsValid check at https://searchfox.org/mozilla-central/rev/d7537a9cd2974efa45141d974e5fc7c727f1a78f/mfbt/CheckedInt.h#535-537

Looking at the code in txFormatNumberFunctionCall::evaluate, the only uses of CheckedInt are:

  CheckedUint32 resPos = CheckedUint32(res.Length()) - 1;
...
#define CHECKED_SET_CHAR(c)                                       \
  if (!resPos.isValid() || !res.SetCharAt(c, resPos--.value())) { \
...
#define CHECKED_TRUNCATE()             \
  if (!resPos.isValid()) {             \
    ReportInvalidArg(aContext);        \
    return NS_ERROR_XPATH_INVALID_ARG; \
  }                                    \
  res.Truncate(resPos--.value());

and the switch to CheckedUint32 happened in bug 1527277.

The code is not quite making sense to me: just because resPos is valid doesn't mean resPos-- will be, if resPos is 0! Am I just missing something?

Comment 2

[Boris Zbarsky [:bzbarsky]]

In particular, what happens if res.Length() == 1 or we reach those macros multiple times and walk off the beginning of the string? Can that happen?

Comment 3

[Bob Clary [:bc] (inactive)]

using http://grandraid-reunion.livetrail.run/coureur.php?rech=32 bughunter reproduces

Assertion failure: mIsValid (Invalid checked integer (division by zero or integer overflow)), at z:/build/build/src/obj-firefox/dist/include/mozilla/CheckedInt.h:537
#01: txCoreFunctionCall::evaluate(txIEvalContext *,txAExprResult * *) [dom/xslt/xpath/txCoreFunctionCall.cpp:424]
#02: txValueOf::execute(txExecutionState &) [dom/xslt/xslt/txInstructions.cpp:768]
#03: txXSLTProcessor::execute(txExecutionState &) [dom/xslt/xslt/txXSLTProcessor.cpp:38]
#04: txMozillaXSLTProcessor::TransformToDoc(mozilla::dom::Document * *,bool) [dom/xslt/xslt/txMozillaXSLTProcessor.cpp:569]

and an opt crash as well.

Comment 7

[Boris Zbarsky [:bzbarsky]]

using http://grandraid-reunion.livetrail.run/coureur.php?rech=32 bughunter reproduces

Bob, thank you, that is extremely helpful!

On that site, we are hitting the assertion on this line inside txFormatNumberFunctionCall::evaluate:

350         res.Insert((char16_t)(1 + format->mZeroDigit), resPos.value() + 1);

The specific value we are working with is 99.980000000000018 (probably meant to be 99.98) and the formatStr we have is "0.0". Pretty minimal testcase coming up, but the analysis is, I think, as follows:

1. When we reach the resPos initialization location, res has length 4, and contains U+0000, U+0000, U+56E1, and U+1528 in that order. We initialize resPos to 3.
2. bufIntDigits is 2, buflen is 16, and i is 2. buf seems to contain "9998", which seems sensible.
3. We end up doing a carry, reset digit from the 9 we got out of buf to 0, and do the CHECKED_SET_CHAR at https://searchfox.org/mozilla-central/rev/55aa17110091deef24b913d033ccaf58f9c6d337/dom/xslt/xslt/txFormatNumberFunctionCall.cpp#309 which sets resPos to 2 and sets res[3] to u'0' aka U+0030.
4. We break out of the loop down to bufIntDigits, since now i == 1.
5. hasFraction is true, so we throw the decimal separator into res, setting res[2] to u'.' aka U+002E. Now resPos is 1.
6. Now we do the loop for (i = 0; i < intDigits; ++i), and intDigits is 2. We go through the loop one time, find a digit of 9 and carry it. We set it via CHECKED_SET_CHAR on line 340, which sets res[1] to u'0' and resPos to 0.
7. We go through the loop the second time (with i == 1 now). We again find a 9 do the carry, end up setting res[0] to u'0' and decrement resPos, which sends it invalid, since it underflows.
8. We exit the intDigits loop and carry is true. i == 2 and groupSize == 12, so we land at https://searchfox.org/mozilla-central/rev/55aa17110091deef24b913d033ccaf58f9c6d337/dom/xslt/xslt/txFormatNumberFunctionCall.cpp#350 and try to insert a 1 at resPos.value() + 1, but of course resPos is invalid at this point and that's where we crash. If we were not using a checked int here, we would underflow and then overflow and try to set at 0, which would actually do something somewhat sane: insert a '1' into res, which would produce "100.0".

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: XSL for testcase

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase

Comment 10

[Peter Van der Beken [:peterv]]

This function is such a mess :-(.

Comment 11

[Peter Van der Beken [:peterv]]

Attachment: Bug 1589930 - Crash in [@ txFormatNumberFunctionCall::evaluate]. r?bzbarsky!

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a8bbd72111b3
Crash in [@ txFormatNumberFunctionCall::evaluate]. r=bzbarsky

Comment 13

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/a8bbd72111b3

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Is this something we should consider for uplift or can this fix ride Fx73 to release? It grafts cleanly to supported branches as-landed.

Comment 16

[Peter Van der Beken [:peterv]]

Comment on attachment 9112859 [details]
Bug 1589930 - Crash in [@ txFormatNumberFunctionCall::evaluate]. r?bzbarsky!

Beta/Release Uplift Approval Request

• User impact if declined: Crash.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Regressing bug 1597081 added checks on over/undeflow of an index into a string, but had a logic error that caused underflow (which then triggered the checks). This just corrects the logic error, but the over/undeflow checks remain so should be pretty safe.
• String changes made/needed:

Comment 17

[Julien Cristau [:jcristau]]

Comment on attachment 9112859 [details]
Bug 1589930 - Crash in [@ txFormatNumberFunctionCall::evaluate]. r?bzbarsky!

crash fix, approved for 72.0b5

Comment 18

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/releases/mozilla-beta/rev/b3c25efdc5e7