Closed Bug 541828 Opened 10 years ago Closed 10 years ago

Firefox 3.6 Crash Report [@ nsZipArchive::BuildFileList() ]

Categories

(Core :: Networking: JAR, defect, critical)

x86
Windows XP
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla1.9.3a1
Tracking Status
blocking1.9.2 --- -
status1.9.2 --- .7-fixed

People

(Reporter: chofmann, Assigned: taras.mozilla)

References

Details

(Keywords: crash, verified1.9.2, Whiteboard: [crashkill])

Crash Data

Attachments

(1 file, 3 obsolete files)

#41 topcrash in some early 3.6 final reports.

stack looks like

http://crash-stats.mozilla.com/report/index/26bdd961-f973-4736-946a-679de2100122

Frame  	Module  	Signature [Expand]  	Source
0 	xul.dll 	nsZipArchive::BuildFileList 	modules/libjar/nsZipArchive.cpp:542
1 	xul.dll 	nsZipArchive::OpenArchive 	modules/libjar/nsZipArchive.cpp:244
2 	xul.dll 	nsJAR::Open 	modules/libjar/nsJAR.cpp:176
3 	xul.dll 	OpenAndValidateArchive 	xpinstall/src/nsXPInstallManager.cpp:713
4 	xul.dll 	nsXPInstallManager::InstallItems 	xpinstall/src/nsXPInstallManager.cpp:791
5 	xul.dll 	nsXPInstallManager::DownloadNext 	xpinstall/src/nsXPInstallManager.cpp:927
6 	xul.dll 	nsXPInstallManager::OnStopRequest 	xpinstall/src/nsXPInstallManager.cpp:1221
7 	xul.dll 	nsStreamListenerTee::OnStopRequest 	netwerk/base/src/nsStreamListenerTee.cpp:72
8 	xul.dll 	nsHttpChannel::OnStopRequest 	netwerk/protocol/http/src/nsHttpChannel.cpp:5279
9 	xul.dll 	nsInputStreamPump::OnStateStop 	netwerk/base/src/nsInputStreamPump.cpp:576
10 	xul.dll 	nsInputStreamPump::OnInputStreamReady 	netwerk/base/src/nsInputStreamPump.cpp:401
11 	xul.dll 	nsOutputStreamReadyEvent::Run 	xpcom/io/nsStreamUtils.cpp:191
12 	xul.dll 	nsThread::ProcessNextEvent 	xpcom/threads/nsThread.cpp:527
13 	xul.dll 	nsBaseAppShell::Run 	widget/src/xpwidgets/nsBaseAppShell.cpp:170
14 	xul.dll 	nsAppStartup::Run 	toolkit/components/startup/src/nsAppStartup.cpp:182
15 	nspr4.dll 	PR_GetEnv 	
16 	firefox.exe 	wmain 	toolkit/xre/nsWindowsWMain.cpp:120
17 	firefox.exe 	__tmainCRTStartup 	obj-firefox/memory/jemalloc/crtsrc/crtexe.c:591
18 	kernel32.dll 	BaseProcessStart

more reports at http://crash-stats.mozilla.com/report/list?range_value=2&range_unit=weeks&signature=nsZipArchive%3A%3ABuildFileList%28%29&version=Firefox%3A3.6

source line at the top of the stack is around code that changed in this batch of checking on 1.9. in early dec.

modules/libjar/nsZipArchive.cpp
diff
browse
annotate	ab17a0719dfb
2009-12-03 18:48 -0800	Taras Glek - Bug 531231: Incorrect mmap cleanup r=alfredkayser
diff
browse
annotate	92475137135c
2009-11-07 16:19 +0100	Taras Glek - Bug 525755 - crash [@ nsZipArchive::BuildFileList] using jar: with the file protocol without a '/' for the root of the filesystem. r=alfredkayser
diff
browse
annotate	c7f1f9689d08
2009-10-21 11:58 +0200	Alfred Kayser - [Bug 523065] libjar: use malloc instead of calloc for zlib. r=tglek
diff
browse
annotate	ebe315caa591
2009-10-17 17:54 +0200	Alfred Kayser - Bug 511754 - make nsZipItems point at ZipCentral references to mmapped jar area r=tglek
diff
browse
annotate	8ac6cac34614
2009-10-09 14:30 +0200	Alfred Kayser - Bug 521227 - Remove the no longer needed zliballoc chunk optimizations. r=tglek
diff
browse
annotate	8bc357ede524
2009-12-03 18:39 -0800	Alfred Kaiser - Bug 510874: Replace ZIP error codes with the NS_ERROR_ codes. r=tglek
diff
browse
annotate	0ee34502b793
2009-12-03 18:39 -0800	Alfred Kaiser - Bug 510844: Remove memcpy()s for compressed jar reading r=tglek
diff
browse
annotate	6a25259a0d0c
2009-12-03 18:39 -0800	Alfred Kaiser - Bug 510247: Buffer under run on JAR files r=tglek
It seems that we need some more checking before using 'offset_central_dir':
PRUint32 centralOffset = xtolong(((ZipEnd *)buf)->offset_central_dir);
539
540 //-- Read the central directory headers
541 buf = startp + centralOffset; 
542 PRUint32 sig = xtolong(buf);  <-- This is where it crashes...
Severity: normal → critical
It seems that the crash is caused by a malformed zip file.
Can someone that is having these crashes try to pin down which zip file this is?
Assignee: nobody → alfredkayser
Status: NEW → ASSIGNED
Attachment #423312 - Flags: review?(tglek)
Attachment #423312 - Flags: review?(tglek) → review+
Pushed to trunk: http://hg.mozilla.org/mozilla-central/rev/4be473b81462

Note, this needs to be pushed to 1.9.2, but that requires some flags?
Fixed on trunk means RESOLVED FIXED.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
(In reply to comment #4) 
> Note, this needs to be pushed to 1.9.2, but that requires some flags?

yes, you need to request "approval1.9.2.1" on the attachment.
Attachment #423312 - Flags: approval1.9.2.1?
This checkin introduced a new build warning on mozilla-central:

http://office.smedbergs.us:8080/warning?id=30734
> modules/libjar/nsZipArchive.cpp:541:
>          comparison between signed and unsigned integer expressions

The problem is, "endp - buf" is signed, & "sizeof(PRUint32))" is unsigned.

Luckily, the "sizeof(PRUint32))" value will trivially fit in a signed integer without overflowing.  So we can easily fix this warning by converting the "sizeof" result to a PRInt32.
This followup fixes the added warning, mentioned in comment 7.
Attachment #423616 - Flags: review?(tglek)
If we understand the crash this well, shouldn't this bug have included an automated test?
Attachment #423616 - Flags: review?(tglek) → review+
As soon as somebody can reproduce a zipfile that cause this crash, a testcase could be created for it.
I looked at some urls associated with the crash.  no obvious zipfiles standout.

maybe some kind of corruption gets introduced in the download of these addons or installers?

   1 https://addons.mozilla.org/sk/firefox/addon/5890
   1 https://addons.mozilla.org/ru/firefox/recommended
   1 https://addons.mozilla.org/ru/firefox/browse/type:1/cat:93
   1 https://addons.mozilla.org/ru/firefox/browse/type:1/cat:22/sort:popular
   1 https://addons.mozilla.org/fr/firefox/search?q=clear&cat=all&advancedsearch=1&as=1&appid=1&lver=3.6&atype=0&pp=20&pid=5&sort=&lup=
   1 https://addons.mozilla.org/fr/firefox/addon/3006
   1 http://get.adobe.com/flashplayer/thankyou/xpi/?installer=Flash_Player_10_for_Windows_-_Other_Browsers&a=McAfee_Security_Scan

http://toolbar.aol.com/mail/welcome.adp?sj=1&postinstall&lang=en&locale=US&source=tb50-ff-aolmailtb&hp=0&ds=1

http://search.conduit.com/?ctid=CT1142338&SearchSource=13

http://r.mail.ru/cln5826/binsputnik.mail.ru/firefox/sputnik.firefox.xpi

http://fpdownload.adobe.com/get/flashplayer/current/install_flash_player.exe

there are also what's new urls being reported.  stuff like

   3 http://www.mozilla.com/en-US/firefox/3.6/whatsnew/
   2 http://www.mozilla.com/it/firefox/3.6/whatsnew/
   1 http://www.mozilla.com/sv-SE/firefox/3.6/whatsnew/
   1 http://www.mozilla.com/en-GB/firefox/3.6/whatsnew/
   1 http://www.mozilla.com/de/firefox/3.6/whatsnew/
   1 http://www.mozilla.com/pt-BR/firefox/3.6/whatsnew/
   1 http://www.mozilla.com/hu/firefox/3.6/whatsnew/
   1 http://www.mozilla.com/et/firefox/3.6/whatsnew/

but maybe these are just side effects of a start up or session restore activity. the number of crash reports with these urls would be extremely low relative to the number of people hitting the page.

first thought was that possible compressed personas contentmight have also contributed to the increase in the crashes when the whats new pages changed in the final release, but the whats new page urls are seen prior to the final release date.
blocking1.9.2: ? → -
Comment on attachment 423312 [details] [diff] [review]
V1: Check for out of range value of CentralOffset

a=beltzner for 1.9.2.2
Attachment #423312 - Flags: approval1.9.2.2? → approval1.9.2.2+
Need checkin for 1.9.2
Keywords: checkin-needed
Duplicate of this bug: 552195
Keywords: checkin-needed
Alfred, we never got a such a zip file that caused the crash right ? So it might very hard to verify this bug as fixed.
Attached file Corrupt zip file that causes the crash (obsolete) —
Here's a simple zip file that causes the crash on my end--it's actually just a text file with the contents "hi there.", masquerading as a zip file.
This is not fixed in 1.9.2.

If I use the attachment in comment 18 via "jar:file://C:/corrupt.zip!/" on the release candidate for 3.6.2 on XP, I get a crash.

See http://crash-stats.mozilla.com/report/index/bp-85411df3-1f23-4ce8-b18d-0da222100322 

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.2) Gecko/20100316 Firefox/3.6.2 (.NET CLR 3.5.30729)
This corrupt.zip probably causes a crash on another point in the zip reader.
What happens is that the file is smaller than ZIPEND_SIZE, so that the first loop crashes...

So another check is needed before:
529   for (buf = endp - ZIPEND_SIZE; xtolong(buf) != ENDSIG; buf--)
530   {
531     if (buf == startp) {
like so:
    if (endp - buf < ZIPEND_SIZE)
       return NS_ERROR_FILE_CORRUPTED;

Currently I don't have live tree available, so may be someone else can make a quick patch here.
Probably also good to change:
531     if (buf == startp) {
into:
531     if (buf <= startp) {
for extra security
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to comment #19)
> This is not fixed in 1.9.2.
> 
> If I use the attachment in comment 18 via "jar:file://C:/corrupt.zip!/" on the
> release candidate for 3.6.2 on XP, I get a crash.

yeah, still ranking around #73 in early firefox 3.6.2 crash data.
Attached patch fixSplinter Review
Fix with testcase
Assignee: alfredkayser → tglek
Attachment #423312 - Attachment is obsolete: true
Attachment #423616 - Attachment is obsolete: true
Attachment #433952 - Attachment is obsolete: true
Attachment #435272 - Flags: review?(alfredkayser)
Attachment #435272 - Flags: review?(alfredkayser) → review+
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/ebe3a63d03d3
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Attachment #435272 - Flags: approval1.9.2.4?
Comment on attachment 435272 [details] [diff] [review]
fix

a=LegNeato for 1.9.2.5. Please ONLY land this on mozilla-1.9.2 default, as we
are still working on 1.9.2.4 on the relbranch
Attachment #435272 - Flags: approval1.9.2.4? → approval1.9.2.5+
(In reply to comment #25)
> http://hg.mozilla.org/releases/mozilla-1.9.2/rev/b105f3fbfa81

This landing is on GECKO1924_20100413_RELBRANCH not default. It wasn't included in 3.6.4 build4 but must be backed out in case a build5 is necessary. You should also set status1.9.2 to '.5-fixed' after landing on default.
Since I thought that was important to fix sooner rather than later, I backed it out of the relbranch:
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/66cf7462b72b
and relanded it on the default branch:
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/604742fd52d5
Attachment #435272 - Flags: approval1.9.2.5+ → approval1.9.2.6+
Verified fixed for 1.9.2.7 using the zip file from comment 18 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.7) Gecko/20100713 Firefox/3.6.7 (.NET CLR 3.5.30729). This file still crashes 1.9.2.6 so it looks good.
Keywords: verified1.9.2
Crash Signature: [@ nsZipArchive::BuildFileList() ]
You need to log in before you can comment on or make changes to this bug.