304414 - nsIChannel::open works on a folder returning a stream that can cause crash

Status: VERIFIED FIXED

(Core :: Networking, defect, P1)

Description

[Michael]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b3) Gecko/20050712 Firefox/1.0+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b3) Gecko/20050712 Firefox/1.0+

As per mozillazine discussion linked since the 2004-06-26 builds Firefox now
opens a channel for a folder that used to throw a NS_ERROR_NOT_IMPLEMENTED
exception. The result is that when the stream is read using the
stream.available() method Firefox crashes with a talkback ID of TB8303646X or
TB8304346K.

Reproducible: Always

Comment 1

[Michael]

Attachment: Testcase

Attched test case that shows the crash, thanks to mw. Works only when run
locally.

Comment 2

[Frank Wein [:mcsmurf]]

Just as a side note, input.available() returns 4294967295 (which is max for
unsigned int), then the code adds +1 for the \0 and then it trys to alloc 0 bytes.

Comment 3

[Martijn Wargers (dead)]

It doesn't crash with a 2004-06-25 build, but crashes with 2004-06-26 build, so
likely to be a regression from fixing bug 247607.

Comment 4

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

http://lxr.mozilla.org/seamonkey/source/netwerk/base/src/nsDirectoryIndexStream.cpp#285

Comment 5

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

I want to note that the directoryInputStream was always broken, and that checkin
just exposed the bug in that stream impl.

Comment 6

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

Attachment: unit test for this case

Comment 7

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

Attachment: patch

Comment 8

[Boris Zbarsky [:bzbarsky]]

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

>Index: base/src/nsDirectoryIndexStream.cpp
>+    // More files available?
>+    PRBool more = mPos < mArray.Count();
>+    if (more) {
>+        *aLength = 1; // A bit of a lie but good enough
>         return NS_OK;
>     }
>+
>+    // All done
>+    *aLength = 0;
>+    return NS_OK;

how about:

*aLength = (mPos < mArray.Count()) ? 1 : 0;
return NS_OK;

Instead of all that?

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 193191 [details] [diff] [review]
unit test for this case

>Index: unit/test_localstreams.js
>+      try {
>+        binstream.readByteArray(1);
>+        do_throw("Data readable when available indicated EOF!");

You don't want to catch whatever do_throw throws.

r=bzbarsky with that fixed.

Comment 10

[Brendan Eich [:brendan]]

(In reply to comment #8)
> how about:
> 
> *aLength = (mPos < mArray.Count()) ? 1 : 0;
> return NS_OK;

Or even *aLength = (mPos < mArray.count()); -- C and C++ guarantee 1 or 0 here.

/be

Comment 11

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

I'm not sure that "*aLength = (mPos < mArray.count());" is more readable than
bz's suggestion...
--.noitseggus s'zb naht elbadaer erom si 

Comment 12

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

Attachment: patch with bz's comment

low risk patch to make the directoryindexstream implement Available()
correctly, which fixes a crash when javascript tries to use this interface siht
esu

Comment 13

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

Attachment: unit test, v1.1

Comment 14

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

fixed on trunk
Checking in netwerk/base/src/nsDirectoryIndexStream.cpp;
/cvsroot/mozilla/netwerk/base/src/nsDirectoryIndexStream.cpp,v  <-- 
nsDirectoryIndexStream.cpp
new revision: 1.36; previous revision: 1.35
done


Checking in netwerk/test/Makefile.in;
/cvsroot/mozilla/netwerk/test/Makefile.in,v  <--  Makefile.in
new revision: 1.87; previous revision: 1.86
done
Checking in netwerk/test/TestCookie.cpp;
/cvsroot/mozilla/netwerk/test/TestCookie.cpp,v  <--  TestCookie.cpp
new revision: 1.4; previous revision: 1.3
done
Checking in netwerk/test/unit/test_cookie_header.js;
/cvsroot/mozilla/netwerk/test/unit/test_cookie_header.js,v  <-- 
test_cookie_header.js
new revision: 1.2; previous revision: 1.1
done
RCS file: /cvsroot/mozilla/netwerk/test/unit/test_localstreams.js,v
done
Checking in netwerk/test/unit/test_localstreams.js;
/cvsroot/mozilla/netwerk/test/unit/test_localstreams.js,v  <--  test_localstreams.js
initial revision: 1.1
done

Comment 15

[Martijn Wargers (dead)]

Verified, thanks Christian!

Comment 16

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

"patch with bz's comment" (attachment 193348 [details] [diff] [review]) checked in to MOZILLA_1_8_BRANCH

Checking in netwerk/base/src/nsDirectoryIndexStream.cpp;
/cvsroot/mozilla/netwerk/base/src/nsDirectoryIndexStream.cpp,v  <-- 
nsDirectoryIndexStream.cpp
new revision: 1.35.4.1; previous revision: 1.35
done

Comment 17

[Jay Patel [:jay]]

v.fixed on branch with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b5)
Gecko/20050928 Firefox/1.4, no crash with testcase (tested as attachment and
locally).