Closed Bug 1527277 Opened 7 months ago Closed 7 months ago

format-number(): Assertion failure: aNewLength <= base_string_type::mLength (Truncate cannot make string longer), at /builds/worker/workspace/build/src/obj-firefox/dist/include/nsTSubstring.h:850

Categories

(Core :: XSLT, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla67
Tracking Status
firefox-esr60 --- wontfix
firefox65 --- wontfix
firefox66 --- wontfix
firefox67 --- fixed

People

(Reporter: nicolas.gregoire, Assigned: erahm)

References

()

Details

(Keywords: regression, Whiteboard: [reporter-external] [client-bounty-form] [verif?])

Attachments

(4 files)

During XSLT processing, the format-number()function could trigger a MOZ_RELEASE_ASSERT assertion. I don't think there's a security impact, given that MOZ_RELEASE_ASSERT will also trigger in non-debug builds.

Stylesheet

<xsl:stylesheet xmlns:xsl="http://www.w3.org/1999/XSL/Transform" version="1.0">
<xsl:decimal-format grouping-separator="." />
<xsl:template match="/">
<xsl:value-of select="format-number(0.1, '.#')"/>
</xsl:template>
</xsl:stylesheet>

XML document

<e />

ASan logs

Assertion failure: aNewLength <= base_string_type::mLength (Truncate cannot make string longer), at /builds/worker/workspace/build/src/obj-firefox/dist/include/nsTSubstring.h:850
AddressSanitizer:DEADLYSIGNAL

==31947==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000001 (pc 0x7fd2e8bfd08a bp 0x7ffc2252c530 sp 0x7ffc2252ba00 T0)
==31947==The signal is caused by a WRITE memory access.
==31947==Hint: address points to the zero page.
#0 0x7fd2e8bfd089 in Truncate /builds/worker/workspace/build/src/obj-firefox/dist/include/nsTSubstring.h:849:5
#1 0x7fd2e8bfd089 in txFormatNumberFunctionCall::evaluate(txIEvalContext*, txAExprResult**) /builds/worker/workspace/build/src/dom/xslt/xslt/txFormatNumberFunctionCall.cpp:312
#2 0x7fd2e8be1651 in txXPathOptimizer::optimize(Expr*, Expr**) /builds/worker/workspace/build/src/dom/xslt/xpath/txXPathOptimizer.cpp:61:19
#3 0x7fd2e8bbfd78 in txExprParser::createExprInternal(nsTSubstring<char16_t> const&, unsigned int, txIParseContext*, Expr**) /builds/worker/workspace/build/src/dom/xslt/xpath/txExprParser.cpp:168:18
#4 0x7fd2e8c746a0 in createExpr /builds/worker/workspace/build/src/dom/xslt/xpath/txExprParser.h:33:12
#5 0x7fd2e8c746a0 in getExprAttr(txStylesheetAttr*, int, nsAtom*, bool, txStylesheetCompilerState&, nsAutoPtr<Expr>&) /builds/worker/workspace/build/src/dom/xslt/xslt/txStylesheetCompileHandlers.cpp:152
#6 0x7fd2e8c8015b in txFnStartValueOf(int, nsAtom*, nsAtom*, txStylesheetAttr*, int, txStylesheetCompilerState&) /builds/worker/workspace/build/src/dom/xslt/xslt/txStylesheetCompileHandlers.cpp:2011:8
#7 0x7fd2e8c4e8b1 in txStylesheetCompiler::startElementInternal(int, nsAtom*, nsAtom*, txStylesheetAttr*, int) /builds/worker/workspace/build/src/dom/xslt/xslt/txStylesheetCompiler.cpp:259:10
#8 0x7fd2e8c4ff39 in txStylesheetCompiler::startElement(char16_t const*, char16_t const**, int) /builds/worker/workspace/build/src/dom/xslt/xslt/txStylesheetCompiler.cpp:153:10
#9 0x7fd2e8c1059a in HandleStartElement /builds/worker/workspace/build/src/dom/xslt/xslt/txMozillaStylesheetCompiler.cpp:117:28
#10 0x7fd2e8c1059a in non-virtual thunk to txStylesheetSink::HandleStartElement(char16_t const*, char16_t const**, unsigned int, unsigned int, unsigned int) /builds/worker/workspace/build/src/dom/xslt/xslt/txMozillaStylesheetCompiler.cpp
#11 0x7fd2e314beb3 in nsExpatDriver::HandleStartElement(char16_t const*, char16_t const**) /builds/worker/workspace/build/src/parser/htmlparser/nsExpatDriver.cpp:285:26
#12 0x7fd2eaf8e7ca in doContent /builds/worker/workspace/build/src/parser/expat/lib/xmlparse.c:2886:11
#13 0x7fd2eaf837af in contentProcessor /builds/worker/workspace/build/src/parser/expat/lib/xmlparse.c:2514:27
#14 0x7fd2eaf837af in doProlog /builds/worker/workspace/build/src/parser/expat/lib/xmlparse.c:4571
#15 0x7fd2eaf7a5a4 in prologProcessor /builds/worker/workspace/build/src/parser/expat/lib/xmlparse.c:4297:10
#16 0x7fd2eaf7a5a4 in prologInitProcessor /builds/worker/workspace/build/src/parser/expat/lib/xmlparse.c:4106
#17 0x7fd2eaf7850e in MOZ_XML_Parse /builds/worker/workspace/build/src/parser/expat/lib/xmlparse.c:1887:17
#18 0x7fd2e315195d in nsExpatDriver::ParseBuffer(char16_t const*, unsigned int, bool, unsigned int*) /builds/worker/workspace/build/src/parser/htmlparser/nsExpatDriver.cpp:796:16
#19 0x7fd2e31524f0 in nsExpatDriver::ConsumeToken(nsScanner&, bool&) /builds/worker/workspace/build/src/parser/htmlparser/nsExpatDriver.cpp:891:5
#20 0x7fd2e315e408 in nsParser::Tokenize(bool) /builds/worker/workspace/build/src/parser/htmlparser/nsParser.cpp:1413:25
#21 0x7fd2e315a302 in nsParser::ResumeParse(bool, bool, bool) /builds/worker/workspace/build/src/parser/htmlparser/nsParser.cpp:963:45
#22 0x7fd2e315f63e in nsParser::OnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned long, unsigned int) /builds/worker/workspace/build/src/parser/htmlparser/nsParser.cpp:1319:12
#23 0x7fd2e8c1139e in txStylesheetSink::OnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned long, unsigned int) /builds/worker/workspace/build/src/dom/xslt/xslt/txMozillaStylesheetCompiler.cpp:219:21
#24 0x7fd2e1bc5e79 in nsCORSListenerProxy::OnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned long, unsigned int) /builds/worker/workspace/build/src/netwerk/protocol/http/nsCORSListenerProxy.cpp:640:20
#25 0x7fd2e1b43b44 in mozilla::net::HttpChannelChild::DoOnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned long, unsigned int) /builds/worker/workspace/build/src/netwerk/protocol/http/HttpChannelChild.cpp:964:18
#26 0x7fd2e1b4daae in mozilla::net::HttpChannelChild::OnTransportAndData(nsresult const&, nsresult const&, unsigned long const&, unsigned int const&, nsTString<char> const&) /builds/worker/workspace/build/src/netwerk/protocol/http/HttpChannelChild.cpp:854:3
#27 0x7fd2e1d8c03e in mozilla::net::ChannelEventQueue::FlushQueue() /builds/worker/workspace/build/src/netwerk/ipc/ChannelEventQueue.cpp:90:12
#28 0x7fd2e1d9f2ca in CompleteResume /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/net/ChannelEventQueue.h:293:5
#29 0x7fd2e1d9f2ca in mozilla::net::ChannelEventQueue::ResumeInternal()::CompleteResumeRunnable::Run() /builds/worker/workspace/build/src/netwerk/ipc/ChannelEventQueue.cpp:148
#30 0x7fd2e1024551 in mozilla::SchedulerGroup::Runnable::Run() /builds/worker/workspace/build/src/xpcom/threads/SchedulerGroup.cpp:299:32
#31 0x7fd2e1054746 in nsThread::ProcessNextEvent(bool, bool*) /builds/worker/workspace/build/src/xpcom/threads/nsThread.cpp:1162:14
#32 0x7fd2e105a968 in NS_ProcessNextEvent(nsIThread*, bool) /builds/worker/workspace/build/src/xpcom/threads/nsThreadUtils.cpp:474:10
#33 0x7fd2e2003fba in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /builds/worker/workspace/build/src/ipc/glue/MessagePump.cpp:88:21
#34 0x7fd2e1f4dd0f in RunInternal /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:315:10
#35 0x7fd2e1f4dd0f in RunHandler /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:308
#36 0x7fd2e1f4dd0f in MessageLoop::Run() /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:290
#37 0x7fd2e9187569 in nsBaseAppShell::Run() /builds/worker/workspace/build/src/widget/nsBaseAppShell.cpp:137:27
#38 0x7fd2ed4ba4ef in XRE_RunAppShell() /builds/worker/workspace/build/src/toolkit/xre/nsEmbedFunctions.cpp:908:20
#39 0x7fd2e1f4dd0f in RunInternal /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:315:10
#40 0x7fd2e1f4dd0f in RunHandler /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:308
#41 0x7fd2e1f4dd0f in MessageLoop::Run() /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:290
#42 0x7fd2ed4b9e94 in XRE_InitChildProcess(int, char**, XREChildData const*) /builds/worker/workspace/build/src/toolkit/xre/nsEmbedFunctions.cpp:746:34
#43 0x5565eacb73d4 in content_process_main /builds/worker/workspace/build/src/browser/app/../../ipc/contentproc/plugin-container.cpp:49:28
#44 0x5565eacb73d4 in main /builds/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:265
#45 0x7fd3004f2b96 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21b96)
#46 0x5565eabdcaa8 in _start (/work/firefox/firefox+0x2aaa8)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV /builds/worker/workspace/build/src/obj-firefox/dist/include/nsTSubstring.h:849:5 in Truncate
==31947==ABORTING

Flags: sec-bounty?
Group: firefox-core-security → dom-core-security
Component: Security → XSLT
Product: Firefox → Core

This implies resPos == 0, we decrement it and due to unsigned overflow we're looking at UINT32_MAX. This will always be larger than the string buffer size so it will trigger the release assertion in Truncate.

So yeah it's probably not a sec issue strictly speaking, but we should probably handle this better than crashing.

Group: dom-core-security
Priority: -- → P2

Sylvestre, why is this marked as a regression? I didn't see a comment about which versions is this not seen in.

Flags: needinfo?(sledru)

In general, we mark crashes as regression because the product wasn't crashing before.

Flags: needinfo?(sledru)

Eric, since you already looked into this (comment 1), I'm wondering if you are (or will be) working on it?

Flags: needinfo?(erahm)

(In reply to Neha Kochar [:neha] from comment #4)

Eric, since you already looked into this (comment 1), I'm wondering if you are (or will be) working on it?

I'll see if there's a quick fix I'll take care of it. This code is pretty much incomprehensible so I might need to defer to peterv.

Flags: needinfo?(erahm)

Add a common function for reporting an invalid argument.

Switch to managing the buffer lifetime with a UniquePtr. This will make handling errors simpler in the next patch.

(In reply to Eric Rahm [:erahm] (ni? for phab reviews) from comment #1)

This implies resPos == 0, we decrement it and due to unsigned overflow we're looking at UINT32_MAX. This will always be larger than the string buffer size so it will trigger the release assertion in Truncate.

This was almost what was happening. It turns out res.Length() == 0 and then we set resPos = res.Length() - 1.

Hi Eric,
I'm seeing this is still marked with regressionwindow-wanted. Do you still need the regression range or you've got down the root of the problem?
In case the regression range is still needed, could you help me out with some steps to reproduce the issue? (so I can start searching for a regression range)

Flags: needinfo?(erahm)

(In reply to Cristian Baica [:cbaica], Release Desktop QA from comment #11)

Hi Eric,
I'm seeing this is still marked with regressionwindow-wanted. Do you still need the regression range or you've got down the root of the problem?
In case the regression range is still needed, could you help me out with some steps to reproduce the issue? (so I can start searching for a regression range)

I don't think we really need one, but if you want to repro you can use the test from patch 4. My guess is you'll just narrow this down to when we added the assertion in bug 1487341.

Flags: needinfo?(erahm)

Peter, can you take a look at patches 3 and 4?

Flags: needinfo?(peterv)

Eric, sorry for the review delay on this one. Peter was caught up with other stuff for Fission M1 target completion. Assuring that this is on his radar and he'll review in the next week.

See Also: → 1532989
Flags: needinfo?(peterv)
Pushed by erahm@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/62aba7d6d779
Part 1: Refactor error reporting logic. r=peterv
https://hg.mozilla.org/integration/autoland/rev/7ce2309548da
Part 2: Use unique pointer to manage buffer lifetime. r=peterv
https://hg.mozilla.org/integration/autoland/rev/b4fef176bc8f
Part 3: Validate usage of string iterator. r=peterv
https://hg.mozilla.org/integration/autoland/rev/60142f1fcb4c
Part 4: Add crashtest. r=peterv

Unfortunately this bug does not qualify for a security bounty because we think this is an unexploitable crash.

Flags: sec-bounty? → sec-bounty-

No problem, that was somewhat expected. As I didn't know the exact impact of this crash, I set the 'sec' flag in order to create a restricted ticket.

You need to log in before you can comment on or make changes to this bug.