Closed Bug 1589930 Opened 3 months ago Closed 2 months ago

Crash in [@ txFormatNumberFunctionCall::evaluate]

Categories

(Core :: XSLT, defect, P2, critical)

Unspecified
Windows 10
defect

Tracking

()

RESOLVED FIXED
mozilla73
Tracking Status
firefox-esr68 --- wontfix
firefox71 --- wontfix
firefox72 --- fixed
firefox73 --- fixed

People

(Reporter: jseward, Assigned: peterv)

References

Details

(Keywords: crash)

Crash Data

Attachments

(3 files)

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

Flags: needinfo?(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?

Flags: needinfo?(bzbarsky) → needinfo?(peterv)

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?

Component: DOM: Core & HTML → XSLT

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.

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".
Attachment #9103068 - Attachment mime type: text/xsl → text/plain
Attachment #9103068 - Attachment mime type: text/plain → text/xsl

This function is such a mess :-(.

Assignee: nobody → peterv
Status: NEW → ASSIGNED
Flags: needinfo?(peterv)
Priority: -- → P2
Pushed by pvanderbeken@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a8bbd72111b3
Crash in [@ txFormatNumberFunctionCall::evaluate]. r=bzbarsky
Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla73
Duplicate of this bug: 1597081

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

Flags: needinfo?(peterv)
Flags: in-testsuite+

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:
Flags: needinfo?(peterv)
Attachment #9112859 - Flags: approval-mozilla-beta?

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

crash fix, approved for 72.0b5

Attachment #9112859 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.