Crash when serializing security info to disk cache [@ memcpy - nsStringInputStream::SetData]

VERIFIED FIXED in mozilla1.9.3a1

Status

()

P2
critical
VERIFIED FIXED
11 years ago
8 years ago

People

(Reporter: syskin2, Assigned: bzbarsky)

Tracking

({crash, fixed1.9.0.15, regression})

Trunk
mozilla1.9.3a1
x86
Windows XP
crash, fixed1.9.0.15, regression
Points:
---
Bug Flags:
blocking1.9.2 +

Firefox Tracking Flags

(status1.9.2 beta1-fixed, status1.9.1 .4-fixed)

Details

(crash signature, URL)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

11 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3pre) Gecko/2007122105 Minefield/3.0b3pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3pre) Gecko/2007122105 Minefield/3.0b3pre

I've been hitting this new crasher when using my bank website.

Reproducible: Sometimes

Steps to Reproduce:
1. Set browser.cache.disk_cache_ssl = true
2. Visit https://www3.netbank.commbank.com.au/netbank/bankmain
3. Keep highlighting the url and pressing enter to reaload the page
Actual Results:  
bp-c6c0970f-aeb7-11dc-9747-001a4bd43e5c
bp-6bf0588a-adda-11dc-adfb-001a4bd46e84
..and friends

Expected Results:  
No crash :)

I could not crash under debugger. I tried other https websites and they don't crash - luckily the login page of my bank is enough (although navigation later on crashes more easily, I think).

Updated

11 years ago
Blocks: 262116
Keywords: crash, regression
Version: unspecified → Trunk

Comment 1

11 years ago
0  	msvcr80.dll@0x151ac  	
1 	nsStringInputStream::SetData(char const*, int) 	mozilla/xpcom/io/nsStringStream.cpp:189
2 	NS_NewByteInputStream(nsIInputStream**, char const*, int, nsAssignmentType) 	mozilla/xpcom/io/nsStringStream.cpp:366
3 	NS_NewCStringInputStream(nsIInputStream**, nsACString_internal const&) 	mozilla/xpcom/io/nsStringStream.cpp:412
4 	NS_DeserializeObject(nsACString_internal const&, nsISupports**) 	mozilla/netwerk/base/src/nsSerializationHelper.cpp:82
5 	nsDiskCacheEntry::CreateCacheEntry(nsCacheDevice*) 	mozilla/netwerk/cache/src/nsDiskCacheEntry.cpp:90
Summary: Crash when serializing security info to disk cache → Crash when serializing security info to disk cache [@ memcpy - nsStringInputStream::SetData]

Comment 2

10 years ago
Bug 458340 may be related.

Comment 3

9 years ago
I am seeing a similar crash too at one of our customer sites:

00 msvcr90!TrailUpVec+0x18
01 xpcom_core!nsStringInputStream::SetData+0x42
02 xpcom_core!NS_NewByteInputStream+0x6c
03 xpcom_core!NS_NewCStringInputStream+0x1a
04 necko!NS_DeserializeObject+0x54
05 necko!nsDiskCacheEntry::CreateCacheEntry+0xce
06 necko!nsDiskCacheDevice::FindEntry+0x83
07 necko!nsCacheService::SearchCacheDevices+0x61
08 necko!nsCacheService::ActivateEntry+0x84
09 necko!nsCacheService::ProcessRequest+0x2d
0a necko!nsCacheService::OpenCacheEntry+0x60
0b necko!nsCacheSession::OpenCacheEntry+0x19
0c necko!nsHttpChannel::OpenCacheEntry+0x1ce
0d necko!nsHttpChannel::Connect+0x106
0e necko!nsHttpChannel::AsyncOpen+0x11e
0f gklayout!nsScriptLoader::ProcessScriptElement+0x8fe
10 gklayout!nsScriptElement::MaybeProcessScript+0x6d
11 gklayout!nsHTMLScriptElement::MaybeProcessScript+0x1e
12 gklayout!nsHTMLScriptElement::DoneAddingChildren+0xc
13 gklayout!HTMLContentSink::ProcessSCRIPTEndTag+0x59
14 gklayout!SinkContext::CloseContainer+0x134
15 gklayout!HTMLContentSink::CloseContainer+0x2f
16 gkparser!CNavDTD::CloseContainer+0xa1
17 gkparser!CNavDTD::HandleEndToken+0x27c
18 gkparser!CNavDTD::HandleToken+0x2df
19 gkparser!CNavDTD::BuildModel+0x177
1a gkparser!nsParser::BuildModel+0x6c
1b gkparser!nsParser::ResumeParse+0xbe
1c gkparser!nsParser::ContinueInterruptedParsing+0x31
1d gklayout!nsContentSink::ContinueInterruptedParsingIfEnabled+0x1d
1e gklayout!nsRunnableMethod<nsJSChannel>::Run+0xd
1f xpcom_core!nsThread::ProcessNextEvent+0xc3
20 xpcom_core!NS_ProcessNextEvent_P+0x20
21 gkwidget!nsBaseAppShell::Run+0x2a
22 tkitcmps!nsAppStartup::Run+0x1e
23 xul!XRE_main+0x1360
24 HsEngine!NS_internal_main+0xc1
25 HsEngine!wmain+0xf3
26 HsEngine!__tmainCRTStartup+0x10f
27 kernel32!BaseProcessStart+0x23
Whiteboard: [needs stack evaluated for usefulness - cache]
(Assignee)

Comment 4

9 years ago
So... NS_DeserializeObject (introduced in bug 262116) is just broken.  In particular, it computes |size| to be _sufficient_ to hold the base64-decoded data, then decodes the data, then uses |size| as the correct datasize.  It's only the correct datasize if the original data had a size that's 0 mod 3.  Otherwise, it's too big.  The correct way to compute the datasize can be found in nsDataChannel::OpenContentStream, say (or various other consumers of PL_Base64Decode).  It's too bad NSPR's API here sucks so much.

In any case, we end up passing a length that's bigger than the actual allocated size of the buffer (but only when the original data size was 1 mod 3, due to the null that NSPR allocates for us).  Then the string input stream tries to make a copy and memcpys that one extra byte that's outside the buffer.  That's presumably when the crash (or UMR in bug 458340) would hit.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [needs stack evaluated for usefulness - cache]
(Assignee)

Updated

9 years ago
Duplicate of this bug: 458340
(Assignee)

Comment 6

9 years ago
Created attachment 400945 [details] [diff] [review]
Fix
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #400945 - Flags: review?(jduell.mcbugs)
(Assignee)

Comment 7

9 years ago
Created attachment 400946 [details] [diff] [review]
Actally looking for the right char too
Attachment #400945 - Attachment is obsolete: true
Attachment #400946 - Flags: review?(jduell.mcbugs)
Attachment #400945 - Flags: review?(jduell.mcbugs)
(Assignee)

Comment 8

9 years ago
Pushed http://hg.mozilla.org/mozilla-central/rev/d1f1ad39e10e
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
(Assignee)

Comment 9

9 years ago
Comment on attachment 400946 [details] [diff] [review]
Actally looking for the right char too

We should fix this on branches...
Attachment #400946 - Flags: approval1.9.2?
Attachment #400946 - Flags: approval1.9.1.4?
Attachment #400946 - Flags: approval1.9.0.16?
Attachment #400946 - Flags: approval1.9.0.15?
Comment on attachment 400946 [details] [diff] [review]
Actally looking for the right char too

>+  size = (size * 3) / 4;

If the string length is > 0x55555555 then this overflows to a small number. Does something upstream from here ensure that the object won't be that big?
I don't know for sure, but if |size| ends up too small (possibly only if some of the objects are serializing data under web page control like URIs), then we'll just fail to deserialize the object (since the stream will end before we have all the data we expect to get).  Presumably the upstream code can already deal with that, since deserialization failure can happen for various reasons...
Comment on attachment 400946 [details] [diff] [review]
Actally looking for the right char too

Approved for 1.9.1.4 and 1.9.0.15, a=dveditz

code-freeze is Sept 22. If this misses we'll push it to the next cycle.
Attachment #400946 - Flags: approval1.9.1.4?
Attachment #400946 - Flags: approval1.9.1.4+
Attachment #400946 - Flags: approval1.9.0.16?
Attachment #400946 - Flags: approval1.9.0.15?
Attachment #400946 - Flags: approval1.9.0.15+
(Assignee)

Updated

9 years ago
Flags: blocking1.9.2?
Pushed http://hg.mozilla.org/releases/mozilla-1.9.1/rev/14cd5d57b639

Checking in netwerk/base/src/nsSerializationHelper.cpp;
/cvsroot/mozilla/netwerk/base/src/nsSerializationHelper.cpp,v  <--  nsSerializationHelper.cpp
new revision: 1.2; previous revision: 1.1
done
status1.9.1: --- → .4-fixed
Keywords: fixed1.9.0.15
Flags: blocking1.9.2? → blocking1.9.2+
Priority: -- → P2
I can't get this to crash in 1.9.0.14 at all.
Al, this bug never had much of a reproducible testcase.  You might have better luck under Valgrind (see bug 458340, a dup of this bug).

Comment 16

9 years ago
Comment on attachment 400946 [details] [diff] [review]
Actally looking for the right char too

blocking doesn't need approval
Attachment #400946 - Flags: approval1.9.2? → approval1.9.2+
(In reply to comment #15)
> Al, this bug never had much of a reproducible testcase.  You might have better
> luck under Valgrind (see bug 458340, a dup of this bug).

Jesse, I'm not familiar with Valgrind. So it's kinda hard for me to verify. Would it be easy a fast for you to run a check? You have reported bug 458340 which has been duped. Thanks.
Severity: major → critical
Target Milestone: --- → mozilla1.9.3a1

Comment 19

9 years ago
Bug 458340 was a one-off occurrence, so I can't really retest that.  I tested the steps in comment 0 and didn't see any cache-related warnings from Valgrind.  V.
Status: RESOLVED → VERIFIED
Crash Signature: [@ memcpy - nsStringInputStream::SetData]
You need to log in before you can comment on or make changes to this bug.