Closed Bug 333423 Opened 15 years ago Closed 14 years ago

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

Categories

(Core :: Networking, defect)

1.8 Branch
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla1.8.1

People

(Reporter: asqueella, Assigned: asqueella)

References

Details

(4 keywords)

Crash Data

Attachments

(4 files, 1 obsolete file)

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();
Testcase based on code that uses enhanced privileges to trigger the crash.
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!

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.)
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.
I finally re-tested using 20060626 build, and it is indeed fixed (by checkin in bug 214672).
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
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
Summary: crash [@ _shexp_match] enumerating entries of a closed nsIZipReader → FF2b2 topcrash [@ _shexp_match] enumerating entries of a closed nsIZipReader
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
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.
I had in mind option 2.
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?
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?
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.
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|?
Attached patch patchSplinter Review
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)
Whiteboard: [branch only]
Target Milestone: --- → mozilla1.8.1
Attachment #217839 - Attachment description: testcase → testcase (for 1.8 branch)
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 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 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+
> (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.
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?
Attached file test for davel to clean up (obsolete) —
This is an xpcshell-based test that davel volunteered to clean up and check into the tree :)
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 on attachment 237487 [details] [diff] [review]
patch

a=schrep for 18drivers for topcrash fix.
Attachment #237487 - Flags: approval1.8.1? → approval1.8.1+
Whiteboard: [branch only] → [checkin needed (1.8 branch)]
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
Closed: 15 years ago14 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
Whiteboard: [checkin needed (1.8 branch)]
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?
Attachment #238251 - Flags: review? → review?(asqueella+bzwatch)
Attachment #238251 - Flags: review?(asqueella+bzwatch) → review?(asqueella)
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+
Attachment #238251 - Flags: review?(darin)
Attachment #238251 - Flags: review?(asqueella)
Attachment #238251 - Flags: review+
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 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
*** Bug 353019 has been marked as a duplicate of this bug. ***
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?
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?
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
> 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+
McAffee has a new version of siteadvisor upcoming that has fixed what was triggering this bug from their end FWIW.
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
Crash Signature: [@ nsZipArchive::FindNext] [@ _shexp_match]
You need to log in before you can comment on or make changes to this bug.