514872 - nsIZipReader.findEntries (and consequently, jar directory browsing) broken after bug 332173

Status: RESOLVED FIXED

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

Description

[:Gijs (he/him)]

STR:

1. Download (or find on your harddrive) a random jar/zip file. Let "file:///foo/bar.zip" be the path where it is saved.
2. In the location bar, type and hit enter on: jar:file///foo/bar.zip!/


Expected result:
A listing of the files and directories in the jar/zip file.

Actual result:
An empty list.

The last working nightly is:
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9.2a1pre) Gecko/20090729 Minefield/3.6a1pre
Built from http://hg.mozilla.org/mozilla-central/rev/55fd6820c596


The first non-working nightly is:
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9.2a1pre) Gecko/20090730 Minefield/3.6a1pre
Built from http://hg.mozilla.org/mozilla-central/rev/36a36a9c942b

Looking at http://hg.mozilla.org/mozilla-central/shortlog/36a36a9c942b, the obvious culprit is bug 332173. I can't see the bug and don't have the time to decipher the patch to figure out why this broke. However, no .idl files changed, so I presume this is all still supposed to work, and that it not working is a bug...

I would set this bug as depending on bug 332173, but I don't have the privileges to do so.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

IINM, The trunk is running different code than the patch I wrote.  
It's a templatized derivative of my patch produced by Jeff Walden, so 
I'm adding him to the cc list here.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

> 2. In the location bar, type and hit enter on: jar:file///foo/bar.zip!/
IINM, the correct syntax would be                jar:file:///foo/bar.zip!/
                                                         ^

I am able to observe this empty listing issue on Windows with 
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a2pre) Gecko/20090817 Namoroka/3.6a2pre 
and a URL such as
jar:file:///c:/tmp/sslTraces.zip!/
which produces a non-empty listing in older browsers.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Interestingly, this is just a problem with the directory listing, not with 
accessing files in the jar's contents.  For example, the URL
jar:file:///c:/tmp/sslTraces.zip!/sslTraceDES.txt
shows the contents of that file, even though 
jar:file:///c:/tmp/sslTraces.zip!/
shows an empty index.

Comment 4

[:Gijs (he/him)]

(In reply to comment #2)
> > 2. In the location bar, type and hit enter on: jar:file///foo/bar.zip!/
> IINM, the correct syntax would be                jar:file:///foo/bar.zip!/
>                                                          ^

Uhm, yes, d'oh. I forgot a colon.


(In reply to comment #3)
> Interestingly, this is just a problem with the directory listing, not with 
> accessing files in the jar's contents.  For example, the URL
> jar:file:///c:/tmp/sslTraces.zip!/sslTraceDES.txt
> shows the contents of that file, even though 
> jar:file:///c:/tmp/sslTraces.zip!/
> shows an empty index.


Right. I actually discovered the issue because a user reported that Chrome List was broken on trunk (http://code.google.com/p/chromelist/issues/detail?id=39). There it was clear that using findEntries to find entries on a per-directory basis somehow no longer worked. I then found the regression range, and then realized that this probably broke jar directory browsing - and it had! :-)

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

At http://mxr.mozilla.org/mozilla-central/source/modules/libjar/nsJARInputStream.cpp#119
we read:

119     // We can get aDir's contents as strings via FindEntries
120     // with the following pattern (see nsIZipReader.findEntries docs)
121     // assuming dirName is properly escaped:
122     //
123     //   dirName + "?*~" + dirName + "?*/?*"

The pattern being passed to ns_WildCardMatch<char> is "?*~?*/?*" 
and the file names (in my test case) are simple file names.  
The return value is the NOMATCH value.  This is unexpected.  

I wonder if the author mistakenly (but understandably) thought that the 
function takes patterns that are regular expressions rather than file name
globbing expressions.  But regardless, that pattern should work.

Unfortunately, the debugger is thoroughly confused by the templatized code,
and is unable to step through it, or even correctly disassemble it. 
So, I think I will try walking through the non-templatized version of the 
code to see how it behaves.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

I found the bug.  IINM, this bug is only in the templatized derivative of my 
patch, and not in my original code.  The bug only occurs with patterns that
contain the '~' character.  My original code looks like this:

358     if (!strchr(xp, '~'))
359         return _shexp_match(str, xp, case_insensitive, 0);
360 
361     exp = PORT_Strdup(xp);
362     if(!exp)
363         return NOMATCH;
364 
365     x = _scan_and_copy(exp, '~', '\0', NULL);

The templatized code translated that, in static function ns_WildCardMatch, as

421     if (!nsCharTraits<T>::find(xp, nsCharTraits<T>::length(xp), T('~')))
422         return _shexp_match(str, xp, case_insensitive, 0);
423 
424     expr = (T *) NS_Alloc((nsCharTraits<T>::length(xp) + 1) * sizeof(T));
425     if(!expr)
426         return NOMATCH;
427 
428     x = ::_scan_and_copy(expr, T('~'), T('\0'), static_cast<T*>(NULL));

The problem (IINM) is that the NS_Alloc at line 424 does not duplicate the 
functionality of the original PORT_Strdup call.  It does not fill the newly
allocated buffer with a copy of the original pattern string "xp".  
That's the problem.

Comment 7

[Jeff Walden [:Waldo]]

Oh gag.  I hate this code.  Rubber-stamp r=me to add this line to fix the problem, if you can whip up the patch (I'll push if you want), which probably should go in branches as well:

memcpy(expr, xp, (nsCharTraits<T>::length(xp) + 1) * sizeof(T));

Comment 8

[(dormant account)]

isn't this bug a dupe of 509267

Comment 9

[:Gijs (he/him)]

(In reply to comment #8)
> isn't this bug a dupe of 509267

Yes and no. The jar directory browsing bit is a dupe. The findEntries bit isn't; the last few comments in bug 509267 seem to indicate it might be fixed by no longer using findEntries.

Comment 10

[Florian Quèze [:florian]]

I think this should block 1.9.2 because it is a serious regression for all consumers of the nsIZipReader API.

There are visible effects in at least Firefox (on the jar directory browsing, see bug 509267, with a potential other way to fix that specific issue), Instantbird (https://bugzilla.instantbird.org/show_bug.cgi?id=211) and Chrome List (http://code.google.com/p/chromelist/issues/detail?id=39).

If I understand comment 6 and comment 7 correctly, the fix is trivial.

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

Sorry, I cannot work on this bug any more at this time.

Comment 12

[:Gijs (he/him)]

Attachment: Patch per comment 7

OK, so this is, afaict, what comment #7 meant. I patched against 1.9.2 as I'm on OS X 10.4, and trunk no longer compiles on that, to my knowledge. It works on my box, and I verified it fixes Chrome List's issues. I put it up on try server (testing branch - which in hindsight perhaps wasn't the cleverest idea) as I figured changing core stuff might cause issues...

Does this need SR? I don't *think* so, but I could be wrong, as the rules changed semi-recently and I've never patched anything in XPCOM (or if I did it was a very long time ago). :-)

Comment 13

[:Gijs (he/him)]

(In reply to comment #12)
> I put it up on try
> server (testing branch - which in hindsight perhaps wasn't the cleverest idea)
> as I figured changing core stuff might cause issues...

Builds seem to have gone OK - except two of the unit test boxen are failing the same getBoundingClientRect tests, there's a random-looking reftest failure, and some test timeouts... I'm presuming that has nothing to do with this patch, but maybe I'm wrong? Logs:

http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1253828343.1253834486.18946.gz
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1253828083.1253834064.14394.gz
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1253828083.1253833776.11308.gz

That, and MacOSX Darwin 8.8.1 try talos dirty is very unhappy, but that seems to be the case for all the tryserver builds, so I'm 99% sure that isn't the patch's fault...

Comment 14

[Benjamin Smedberg]

Gijs, you should land this please.

Did this also regress on 1.9.0 and 1.9.1?

Comment 15

[[e-mail]]

Is there a particular reason we can't use NS_strdup or nsMemory::Clone?

Comment 16

[Daniel Veditz [:dveditz]]

On the 1.9.0 and 1.9.1 branches the libjar copy of nsWildcard is fine, but this fix is needed in the xpfe/components/filepicker copy.

Comment 17

[:Gijs (he/him)]

(In reply to comment #15)
> Is there a particular reason we can't use NS_strdup or nsMemory::Clone?

I don't know. Is there an advantage to doing so? If so, please submit the alternate patch for review... I am not at all an expert in this area of the code, the bug simply bit me and I was the first to put up a patch that I didn't even come up with myself...

(In reply to comment #14)
> Gijs, you should land this please.

Apologies; I have been in transit and adjusting to EST (where I am sort of on holiday). At the moment the tree is red. Barring a decision to use an alternate approach (comment #15), if someone gets to it before me, feel free to check in. If that doesn't happen, I will try to get to it tonight, and otherwise tomorrow morning (EST).

Comment 18

[:Gijs (he/him)]

(In reply to comment #17)
> (In reply to comment #14)
> > Gijs, you should land this please.
> 
> Apologies; I have been in transit and adjusting to EST (where I am sort of on
> holiday). At the moment the tree is red. Barring a decision to use an alternate
> approach (comment #15), if someone gets to it before me, feel free to check in.
> If that doesn't happen, I will try to get to it tonight, and otherwise tomorrow
> morning (EST).

http://hg.mozilla.org/mozilla-central/rev/3a091a5c93d9

Comment 19

[:Gijs (he/him)]

Oops, 1.9.2 checkin was: http://hg.mozilla.org/releases/mozilla-1.9.2/rev/f1e55c249e20

Comment 20

[:Gijs (he/him)]

Attachment: Patch for 1.9.0 filepicker copy

Comment 21

[:Gijs (he/him)]

Comment on attachment 404038 [details] [diff] [review]
Patch for 1.9.0 filepicker copy

Carrying over review, requesting approval per comment #16.

Comment 22

[Daniel Veditz [:dveditz]]

Comment on attachment 404038 [details] [diff] [review]
Patch for 1.9.0 filepicker copy

Approved for 1.9.1.4 and 1.9.0.15, a=dveditz for release-drivers

Comment 23

[:Gijs (he/him)]

Checking in mozilla/xpfe/components/filepicker/src/nsWildCard.cpp;
/cvsroot/mozilla/xpfe/components/filepicker/src/nsWildCard.cpp,v  <--  nsWildCard.cpp
new revision: 1.5; previous revision: 1.4
done

Comment 24

[:Gijs (he/him)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/fb0c1de78c61

Unless I missed something, we're done in this bug...

Comment 25

[Daniel Veditz [:dveditz]]

Looks like it to me, too. Thank-you!