FF2b2 topcrash [@ nsZipArchive::FindNext] [@ _shexp_match] enumerating entries of a closed nsIZipReader

VERIFIED FIXED in mozilla1.8.1

Status

()

--
critical
VERIFIED FIXED
13 years ago
7 years ago

People

(Reporter: asqueella, Assigned: asqueella)

Tracking

(4 keywords)

1.8 Branch
mozilla1.8.1
crash, topcrash, verified1.8.0.9, verified1.8.1
Points:
---
Bug Flags:
blocking1.8.1 +
blocking1.8.0.9 +

Firefox Tracking Flags

(Not tracked)

Details

(crash signature)

Attachments

(4 attachments, 1 obsolete attachment)

(Assignee)

Description

13 years ago
Trying to enumerate entries of a zipreader that was initialized, opened, and then closed, crashes with this stack:

jar50.dll!_shexp_match(char * str=0xfeeefeee, char * expr=0x03ca1118, int case_insensitive=0)  Line 184
jar50.dll!NS_WildCardMatch(char * str=0xfeeefeee, char * xp=0x03d29ea8, int case_insensitive=0)  Line 292
jar50.dll!nsZipArchive::FindNext(nsZipFind * aFind=0x03e50a90, nsZipItem * * aResult=0x03e50afc)  Line 819
jar50.dll!nsJAREnumerator::HasMoreElements(int * aResult=0x0012b548)  Line 1000

Here's the code that can be used to reproduce the crash:

function showZipContents()
{
   var file;

   // Open a file
   const CFilePicker = "@mozilla.org/filepicker;1";
   const nsIFilePicker = Components.interfaces.nsIFilePicker;

   var filepicker = Components.classes[CFilePicker].createInstance(nsIFilePicker);
   filepicker.init(window, "Select a File", nsIFilePicker.modeOpen);

   var res = filepicker.show();
   if(res != nsIFilePicker.returnOK)
      return;

   file = filepicker.file;

   // Show contents of zip file
   var zipreader = Components.classes["@mozilla.org/libjar/zip-reader;1"]
                             .createInstance(Components.interfaces.nsIZipReader);
   zipreader.init(file);
   zipreader.open();
   zipreader.close();
   var entries = zipreader.findEntries('*.*');
   while(entries.hasMoreElements()) {
      var entry = entries.getNext();
      entry.QueryInterface(Components.interfaces.nsIZipEntry);
      alert(entry.name);
   }

   //zipreader.close();
}
showZipContents();
Created attachment 217839 [details]
testcase (for 1.8 branch)

Testcase based on code that uses enhanced privileges to trigger the crash.

Comment 2

13 years ago
Created attachment 222318 [details]
Slightly modified testcase for updated nsIZipReader interface (trunk)

With bug 214672 Further optimization and correctness improvements of libj...
this is now fixed that it no longer crashes. 
Note, nsIZipReader got a few changes:
Init(file)/Open got merged into Open(file)
FindEntries now returns a StringEnumaration with the names of the found entries.
See modified testscript for the changes.

So, this bug can be closed now.
Except if people want to have this also fixed in the branches?
(In reply to comment #2)
> Except if people want to have this also fixed in the branches?

Not me, thanks for fixing!

(Assignee)

Comment 4

13 years ago
Not me either, since 214672 apparently can't be taken on the branch. Thank you.

(I'm not resolving this one just because I didn't get to re-testing it yet.)

Comment 5

13 years ago
bug 214672 could be taken to the branch, but it is a 'risky' patch, changing the public nsIZipReader interface, so my guess is that the branch drivers won't really want to do that one.
(Assignee)

Comment 6

12 years ago
I finally re-tested using 20060626 build, and it is indeed fixed (by checkin in bug 214672).
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED

Comment 7

12 years ago
Reopening. This is still a topcrasher in Firefox 2 beta 2. I imagine it's due to some extension (like Forecastfox) abusing things like this testcase does.

Alfred, Jeff, maybe one of you guys can look into this. I tried, but couldn't quite figure out what was going on. 
Status: RESOLVED → REOPENED
Flags: blocking1.8.1?
Keywords: topcrash
OS: Windows XP → All
Hardware: PC → All
Resolution: FIXED → ---
Version: Trunk → 1.8 Branch
(Assignee)

Updated

12 years ago
Summary: crash [@ _shexp_match] enumerating entries of a closed nsIZipReader → FF2b2 topcrash [@ _shexp_match] enumerating entries of a closed nsIZipReader
(Assignee)

Comment 8

12 years ago
Er, meant to add nsZipArchive::FindNext to the summary (that's how the crash shows up in talkback, according to ispiked). There are 30+ crashes with that signature, making it #9 on the list for b2.
Summary: FF2b2 topcrash [@ _shexp_match] enumerating entries of a closed nsIZipReader → FF2b2 topcrash [@ nsZipArchive::FindNext] [@ _shexp_match] enumerating entries of a closed nsIZipReader

Comment 9

12 years ago
As noted in comments 4 and 5, the patch of bug 214672 was only applied to the trunk, and not to *any* bracnhes, so also not to the FF2 branch.

Two options:
1. Apply the bug 214672 patch also to the FF2 branch.
2. Device a special patch for just this issues for FF2.

Comment 10

12 years ago
I had in mind option 2.

Updated

12 years ago
Flags: blocking1.8.1? → blocking1.8.1+
Darin, any idea which part of the patch mentioned above would have fixed this, and whether we could get than onto the 1.8 branch?

Comment 12

12 years ago
One simple patch would be to check for mFd in 'findEntries'
727 //---------------------------------------------
728 // nsZipArchive::FindInit
729 //---------------------------------------------
730 nsZipFind* nsZipArchive::FindInit(const char * aPattern)
731 {

And possibly the FindNext also wants to check for mFd.
767 //---------------------------------------------
768 // nsZipArchive::FindNext
769 //---------------------------------------------
770 PRInt32 nsZipArchive::FindNext(nsZipFind* aFind, nsZipItem** aResult)
771 {

Note, the crash probably caused by the FindNext crashing on invalid entries in mFiles, which is probably caused by a freed ZipReader object, whilst the underlying nsZipArchive object is still referred to from nsZipFind...

May be the nsJarEnumerator needs to addref the nsJar so that the nsJar doesn't get destroyed before the nsJarEnumerator is destroyed?

Comment 13

12 years ago
So, to make the FindInit and FindNext more foolproof, they should both do:

  // If there is no mFd then the zipArchive has been closed.
  if (!mFd) return ZIP_ERR_PARAM;

I don't have access to the branch (only to trunk), but I am sure that someelse can wrap this up in a patch.

Comment 14

12 years ago
Alfred, that patch won't work because |mFd| is only used in STANDALONE mode. What do we need to check in non-STANDALONE mode to make sure we still have the archive? Something involving |mFiles|?
(Assignee)

Comment 15

12 years ago
Created attachment 237487 [details] [diff] [review]
patch

The crash happens because memory belonging to a freed entry is accessed. This happens because in the non-standalone case mFiles' items are not nulled out.

Simply nulling mFiles out fixes the crash in the testcase for me. Note that this is also done on trunk.

One could argue we should throw an exception when a closed zipreader is accessed, but it is not required to fix the crash and can be done separately.
Assignee: nobody → asqueella
Status: REOPENED → ASSIGNED
Attachment #237487 - Flags: superreview?(darin)
Attachment #237487 - Flags: review?(alfredkayser)
(Assignee)

Updated

12 years ago
Whiteboard: [branch only]
Target Milestone: --- → mozilla1.8.1
(Assignee)

Updated

12 years ago
Attachment #217839 - Attachment description: testcase → testcase (for 1.8 branch)
(Assignee)

Updated

12 years ago
Attachment #222318 - Attachment description: Slightly modified testcase for updated nsIZipReader interface → Slightly modified testcase for updated nsIZipReader interface (trunk)
Attachment #222318 - Attachment mime type: text/plain → text/html

Comment 16

12 years ago
Comment on attachment 237487 [details] [diff] [review]
patch

Fine by me.
(Although in the trunk code the following is used to clear the array:
577   for (int i = 0; i < ZIP_TABSIZE; i++) {
578     mFiles[i] = 0;
579   }
)
I am not sure what is preferred)
Attachment #237487 - Flags: review?(alfredkayser) → review+

Comment 17

12 years ago
Comment on attachment 237487 [details] [diff] [review]
patch

looks good, sr=darin

(I too prefer consistency in this module, but memset is better than a for-loop.)
Attachment #237487 - Flags: superreview?(darin) → superreview+
(Assignee)

Comment 18

12 years ago
> (I too prefer consistency in this module, but memset is better than a
> for-loop.)
> 
That module already uses memset to null out mFiles, so I didn't make it any more inconsistent.
(Assignee)

Comment 19

12 years ago
Comment on attachment 237487 [details] [diff] [review]
patch

Asking for approval. This is a patch for branch-only bug that should fix a topcrash. Pretty safe - one line change shouldn't affect the users who don't abuse nsIZipArchive's API. FWIW, code with similar effect has been on trunk for a while.
Attachment #237487 - Flags: approval1.8.1?
(Assignee)

Comment 20

12 years ago
Created attachment 238041 [details]
test for davel to clean up

This is an xpcshell-based test that davel volunteered to clean up and check into the tree :)
(Assignee)

Comment 21

12 years ago
Dave, https://bugzilla.mozilla.org/attachment.cgi?id=238041 has the zipped up test you asked for. It is supposed to be in modules/libjar/test, and you need to add the test dir itself to the libjar/Makefile.in:

ifdef ENABLE_TESTS
TOOL_DIRS  += test
endif

Comment 22

12 years ago
Comment on attachment 237487 [details] [diff] [review]
patch

a=schrep for 18drivers for topcrash fix.
Attachment #237487 - Flags: approval1.8.1? → approval1.8.1+
(Assignee)

Updated

12 years ago
Whiteboard: [branch only] → [checkin needed (1.8 branch)]

Comment 23

12 years ago
Nickolay - we are approaching code freeze for 1.8.1 - can you land on that branch and mark fixed1.8.1.  
Comment on attachment 238041 [details]
test for davel to clean up

in test/Makefile.in, replace "$(INSTALL) $(srcdir)/test_bug333423.zip ." with "$(ZIP) -q test_bug333423.zip Makefile" and it seems to work. I'll test this on trunk, and check it in on trunk, if that is ok.
mozilla/modules/libjar/nsZipArchive.cpp 	1.78.20.3
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago12 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
Whiteboard: [checkin needed (1.8 branch)]
Created attachment 238251 [details]
new tests for trunk (zip instead of cvs diff)

Here's the "final" version - do I have to add the new Makefile to allmakefiles?
Attachment #238041 - Attachment is obsolete: true
Attachment #238251 - Flags: review?

Updated

12 years ago
Attachment #238251 - Flags: review? → review?(asqueella+bzwatch)

Updated

12 years ago
Attachment #238251 - Flags: review?(asqueella+bzwatch) → review?(asqueella)
(Assignee)

Comment 27

12 years ago
Comment on attachment 238251 [details]
new tests for trunk (zip instead of cvs diff)

Not a formal review, but I looked and tested the code on windows and it works fine.
Attachment #238251 - Flags: review?(asqueella) → review+

Updated

12 years ago
Attachment #238251 - Flags: review?(darin)
Attachment #238251 - Flags: review?(asqueella)
Attachment #238251 - Flags: review+
(Assignee)

Updated

12 years ago
Attachment #238251 - Flags: review?(asqueella) → review+
Comment on attachment 238251 [details]
new tests for trunk (zip instead of cvs diff)

fixing collision
Flags: blocking1.8.0.8?
> do I have to add the new Makefile to allmakefiles?

probably a good idea although not strictly needed.
oh... guess it is needed for make distclean to work correctly.

Comment 31

12 years ago
Comment on attachment 238251 [details]
new tests for trunk (zip instead of cvs diff)

sr=darin
Attachment #238251 - Flags: review?(darin) → review+
checked in test case to trunk

Checking in Makefile.in;
/cvsroot/mozilla/modules/libjar/Makefile.in,v  <--  Makefile.in
new revision: 1.52; previous revision: 1.51
done
RCS file: /cvsroot/mozilla/modules/libjar/test/Makefile.in,v
done
Checking in test/Makefile.in;
/cvsroot/mozilla/modules/libjar/test/Makefile.in,v  <--  Makefile.in
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/modules/libjar/test/unit/test_bug333423.js,v
done
Checking in test/unit/test_bug333423.js;
/cvsroot/mozilla/modules/libjar/test/unit/test_bug333423.js,v  <--  test_bug333423.js
initial revision: 1.1
done
Checking in allmakefiles.sh;
/cvsroot/mozilla/allmakefiles.sh,v  <--  allmakefiles.sh
new revision: 1.642; previous revision: 1.641
done

Comment 33

12 years ago
*** Bug 353019 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 34

12 years ago
Comment on attachment 237487 [details] [diff] [review]
patch

Asking for 1.8.0.8 approval. This has been on 1.8 branch for a while; rather safe one-line patch that should fix the #23 topcrash on 1.5.0.6.
Attachment #237487 - Flags: approval1.8.0.8?
(Assignee)

Comment 35

12 years ago
With the help of ispiked we found out that McAfee Site Advisor extension is triggering the crash with the following code:
 var found = zipReader.findEntries(relPath);
 zipReader.close();
 return found?found.hasMoreElements():false;

I couldn't find a way to report a bug to mcafee though :-/
Restoring lost blocking flag
Flags: blocking1.8.0.9?

Comment 37

12 years ago
Is this patch applied to 1.5.0.7? I don't get segfaults using the official mozilla binary release but if I compile 1.5.0.7 myself (via gentoo portage) I do get segfaults when running firefox with siteadvisor installed.

Applying https://bugzilla.mozilla.org/attachment.cgi?id=237487 and then compiling prevents the segfaults.

I'm not sure why the binary release doesn't segfault for me though.

Regards,
Sam
(Assignee)

Comment 38

12 years ago
> Is this patch applied to 1.5.0.7?
No.
Flags: blocking1.8.0.9? → blocking1.8.0.9+
Comment on attachment 237487 [details] [diff] [review]
patch

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #237487 - Flags: approval1.8.0.9? → approval1.8.0.9+

Comment 40

12 years ago
McAffee has a new version of siteadvisor upcoming that has fixed what was triggering this bug from their end FWIW.
(Assignee)

Updated

12 years ago
Whiteboard: [checkin needed - 1.8.0 branch]
mozilla/modules/libjar/nsZipArchive.cpp 	1.78.28.1
Keywords: fixed1.8.0.9
Whiteboard: [checkin needed - 1.8.0 branch]
Confirm verified fixed on Vista, Windows 2000, XP x64 and Fedora FC6 with :
Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.8.1.1pre) Gecko/20061127 BonEcho/2.0.0.1pre
Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.8.0.9pre) Gecko/20061127 Firefox/1.5.0.9pre

Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.8.1.1pre) Gecko/20061128 BonEcho/2.0.0.1pre
Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.8.0.9pre) Gecko/20061127 Firefox/1.5.0.9pre

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.1pre) Gecko/20061128 BonEcho/2.0.0.1pre
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.0.9pre) Gecko/20061128 Firefox/1.5.0.9pre

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8.1.1pre) Gecko/20061127 BonEcho/2.0.0.1pre
Status: RESOLVED → VERIFIED
Keywords: fixed1.8.0.9, fixed1.8.1 → verified1.8.0.9, verified1.8.1
Crash Signature: [@ nsZipArchive::FindNext] [@ _shexp_match]
You need to log in before you can comment on or make changes to this bug.