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

Status: VERIFIED FIXED

(Core :: Networking, defect)

Description

[Nickolay_Ponomarev]

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();

Comment 1

[Martijn Wargers (dead)]

Attachment: testcase (for 1.8 branch)

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

Comment 2

[Alfred Kayser]

Attachment: 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?

Comment 3

[Martijn Wargers (dead)]

(In reply to comment #2)
> Except if people want to have this also fixed in the branches?

Not me, thanks for fixing!

Comment 4

[Nickolay_Ponomarev]

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

[Alfred Kayser]

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.

Comment 6

[Nickolay_Ponomarev]

I finally re-tested using 20060626 build, and it is indeed fixed (by checkin in bug 214672).

Comment 7

[Adam Guthrie]

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. 

Comment 8

[Nickolay_Ponomarev]

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.

Comment 9

[Alfred Kayser]

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

[Adam Guthrie]

I had in mind option 2.

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

[Alfred Kayser]

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

[Alfred Kayser]

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

[Adam Guthrie]

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|?

Comment 15

[Nickolay_Ponomarev]

Attachment: 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.

Comment 16

[Alfred Kayser]

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)

Comment 17

[Darin Fisher]

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.)

Comment 18

[Nickolay_Ponomarev]

> (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 19

[Nickolay_Ponomarev]

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.

Comment 20

[Nickolay_Ponomarev]

Attachment: test for davel to clean up

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

Comment 21

[Nickolay_Ponomarev]

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

[Mike Schroepfer]

Comment on attachment 237487 [details] [diff] [review]
patch

a=schrep for 18drivers for topcrash fix.

Comment 23

[Mike Schroepfer]

Nickolay - we are approaching code freeze for 1.8.1 - can you land on that branch and mark fixed1.8.1.  

Comment 24

[Dave Liebreich [:davel]]

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.

Comment 25

[:Gavin Sharp [email: [e-mail]]]

mozilla/modules/libjar/nsZipArchive.cpp 	1.78.20.3

Comment 26

[Dave Liebreich [:davel]]

Attachment: new tests for trunk (zip instead of cvs diff)

Here's the "final" version - do I have to add the new Makefile to allmakefiles?

Comment 27

[Nickolay_Ponomarev]

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.

Comment 28

[Dave Liebreich [:davel]]

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

fixing collision

Comment 29

[Christian :Biesinger (don't email me, ping me on IRC)]

> do I have to add the new Makefile to allmakefiles?

probably a good idea although not strictly needed.

Comment 30

[Christian :Biesinger (don't email me, ping me on IRC)]

oh... guess it is needed for make distclean to work correctly.

Comment 31

[Darin Fisher]

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

sr=darin

Comment 32

[Dave Liebreich [:davel]]

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

[Adam Guthrie]

*** Bug 353019 has been marked as a duplicate of this bug. ***

Comment 34

[Nickolay_Ponomarev]

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.

Comment 35

[Nickolay_Ponomarev]

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 :-/

Comment 36

[Daniel Veditz [:dveditz]]

Restoring lost blocking flag

Comment 37

[Sam Schinke]

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

Comment 38

[Nickolay_Ponomarev]

> Is this patch applied to 1.5.0.7?
No.

Comment 39

[Daniel Veditz [:dveditz]]

Comment on attachment 237487 [details] [diff] [review]
patch

approved for 1.8.0 branch, a=dveditz for drivers

Comment 40

[Sam Schinke]

McAffee has a new version of siteadvisor upcoming that has fixed what was triggering this bug from their end FWIW.

Comment 41

[:Gavin Sharp [email: [e-mail]]]

mozilla/modules/libjar/nsZipArchive.cpp 	1.78.28.1

Comment 42

[Carsten Book [:Tomcat]]

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