513832 - 'state' is not initialized, and is not preserved between calls to ParseFTPList()

Status: RESOLVED FIXED

(Core Graveyard :: Networking: FTP, defect)

Description

[Wan-Teh Chang]

Attachment: Proposed patch

ParseFTPList.h documents the 'state' parameter of
ParseFTPList() as follows:
http://mxr.mozilla.org/mozilla-central/source/netwerk/streamconv/converters/ParseFTPList.h#45

45 ** Arguments:
...
48 **   'state':      a structure used internally to track state between 
49 **                 lines. Needs to be bzero()'d at LIST begin.

But we're not doing that when we call ParseFTPList():
http://mxr.mozilla.org/mozilla-central/source/netwerk/streamconv/converters/nsFTPDirListingConv.cpp#286

287     while ( line && (eol = PL_strchr(line, nsCRT::LF)) ) {
            ... 
298         list_state state;
            ...
301         int type = ParseFTPList(line, &state, &result );

First, we pass a new stack variable 'state' every time
we call ParseFTPList(), so the state is not carried
over from call to call.  Second, we don't zero 'state'
before passing it to ParseFTPList().  This means we
read uninitialized memory at least once.

The attached patch fixes both problems.  Note that I
merely set the 'magic' field of 'state' to 0 rather
than zeroing the entire struct because of this code
in ParseFTPList():
http://mxr.mozilla.org/mozilla-central/source/netwerk/streamconv/converters/ParseFTPList.cpp#59

59   if (state->magic != ((void *)ParseFTPList))
60   {
61     memset( state, 0, sizeof(*state) );
62     state->magic = ((void *)ParseFTPList);
63   }

Please let me know if you'd prefer that I zero the
struct.

Comment 1

[Wan-Teh Chang]

I pushed the patch to mozilla-central in changeset 4518af8ff692:
http://hg.mozilla.org/mozilla-central/rev/4518af8ff692

Comment 2

[Mike Beltzner [:beltzner, not reading bugmail]]

Regression: Txul increase 3.50% on Vista Firefox

Previous results: 96.3684 from build 20090904112552 of revision 859f3f3927a5 at 2009-09-04 11:25:00 on talos-rev2-vista15 run # 0

New results: 99.7368 from build 20090904133335 of revision e936fa034d28 at 2009-09-04 13:33:00 on talos-rev2-vista02 run # 0

Suspected checkin range: from revision 859f3f3927a5 to revision e936fa034d28

--------------

Implicates this bug. :(

Comment 3

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

Comment on attachment 397772 [details] [diff] [review]
Proposed patch

Since bzero is not actually needed, I'd have updated the comment in ParseFTPList.h to say that only magic needs to be initialized.

Also... state still isn't kept around for the entire listing, only for the current OnDataAvailable chunk. Maybe it would be better to store it in a member variable?

Out of curiosity, did you find this by code inspection or did you notice an actual bug resulting from this?

Comment 4

[Wan-Teh Chang]

Mike: this code is only executed when you visit an FTP site
and the site returns a directory listing.  Does Txul measure
that?

Christian: I will create another patch to update the comment
in ParseFTPList.h, and see if we should make 'state' a member
variable.  I found this bug by code inspection.  I believe
valgrind and Purify should be able to catch the uninitialized
memory read on the first ParseFTPList() call.

Comment 5

[Wan-Teh Chang]

With the help of mcsmurf on IRC, I found the Txul graphs for
those two machines.  The graphs are very volatile, going up
and down.  After my checkin, both graphs went to a local
maximum.  Then they went down, and up, and back down again.
Now both graphs are at the lowest points in the 24-hour
period.

Since Txul measures XUL window open time:
https://wiki.mozilla.org/Performance:Tinderbox_Tests#Txul:_XUL_window_open_time
it's very unlikely that an FTP directory listing parser change
would affect Txul.