Fix a renderer crash while processing FTP directory listing.

Fix a renderer crash while processing FTP directory listing.

The problem was caused by a broken assumption about current_parser_.
After adding OnEndOfInput to FtpDirectoryListingParser interface,
it was possible that during processing of input we ended up
with just one parser, which returned error when OnEndOfInput was called.

In that case, we should just reset current_parser_ to NULL to avoid
a use-after-free error.

TEST=net_unittests
BUG=47528
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=50930

[eroman, 2010-06-25 21:00:45]

lgtm

[wtc, 2010-06-26 17:44:37]

LGTM.

http://codereview.chromium.org/2846037/diff/1/2
File net/data/ftp/dir-listing-ls-17 (right):

http://codereview.chromium.org/2846037/diff/1/2#newcode1
net/data/ftp/dir-listing-ls-17:1: ftpd-BSD: .: Permission denied
Just curious: could you explain why this input causes the
parser's OnEndOfInput to fail?

http://codereview.chromium.org/2846037/diff/1/4
File net/ftp/ftp_directory_listing_buffer.cc (right):

http://codereview.chromium.org/2846037/diff/1/4#newcode119
net/ftp/ftp_directory_listing_buffer.cc:119: return ERR_FAILED;
Should we return ERR_UNRECOGNIZED_FTP_DIRECTORY_LISTING_FORMAT
instead?

http://codereview.chromium.org/2846037/diff/1/4#newcode146
net/ftp/ftp_directory_listing_buffer.cc:146: delete *i;
You can also fix this bug by adding the following code
before deleting |*i|:
      if (current_parser_ == *i)
        current_parser_ = NULL;

Your fix is fine.  I just wanted to check my understanding
of the code.

[Paweł Hajdan Jr., 2010-06-28 16:58:48]

http://codereview.chromium.org/2846037/diff/1/2
File net/data/ftp/dir-listing-ls-17 (right):

http://codereview.chromium.org/2846037/diff/1/2#newcode1
net/data/ftp/dir-listing-ls-17:1: ftpd-BSD: .: Permission denied
On 2010/06/26 17:44:37, wtc wrote:
> Just curious: could you explain why this input causes the
> parser's OnEndOfInput to fail?

Good question. My interpretation is that initially current_parser_ becomes VMS.
That's because other parsers are more strict at the beginning, but VMS gives
even less predictable listing headers, so we accept everything as a potential
header. But then VMS has a very specific footer. We use it to verify that it was
really a VMS listing, and in this case it wasn't.

http://codereview.chromium.org/2846037/diff/1/4
File net/ftp/ftp_directory_listing_buffer.cc (right):

http://codereview.chromium.org/2846037/diff/1/4#newcode119
net/ftp/ftp_directory_listing_buffer.cc:119: return ERR_FAILED;
On 2010/06/26 17:44:37, wtc wrote:
> Should we return ERR_UNRECOGNIZED_FTP_DIRECTORY_LISTING_FORMAT
> instead?

Indeed, getting rid of ERR_FAILED would be good. I'm going to prepare a
follow-up CL. I'd just rather add ERR_INVALID_FTP_DIRECTORY_LISTING_FORMAT so
the code is more specific. When current_parser_ is non-null, it indicates it
"thinks" it recognized the format.

http://codereview.chromium.org/2846037/diff/1/4#newcode146
net/ftp/ftp_directory_listing_buffer.cc:146: delete *i;
On 2010/06/26 17:44:37, wtc wrote:
> You can also fix this bug by adding the following code
> before deleting |*i|:
>       if (current_parser_ == *i)
>         current_parser_ = NULL;
> 
> Your fix is fine.  I just wanted to check my understanding
> of the code.

Sounds good to me.

[Paweł Hajdan Jr., 2010-06-28 16:59:11]

Oh, by the way, what do you think about merging it to the 375 branch?

[wtc, 2010-06-30 15:07:53]

Merging this CL to the 375 branch (for 5.0) is fine because it
fixes a crash and has a low risk.

http://codereview.chromium.org/2846037/diff/1/2
File net/data/ftp/dir-listing-ls-17 (right):

http://codereview.chromium.org/2846037/diff/1/2#newcode1
net/data/ftp/dir-listing-ls-17:1: ftpd-BSD: .: Permission denied
Thanks for the explanation.

Sounds like you didn't construct this test case.  Is this test case
based on a real FTP directory listing?

http://codereview.chromium.org/2846037/diff/1/4
File net/ftp/ftp_directory_listing_buffer.cc (right):

http://codereview.chromium.org/2846037/diff/1/4#newcode119
net/ftp/ftp_directory_listing_buffer.cc:119: return ERR_FAILED;
Adding ERR_INVALID_FTP_DIRECTORY_LISTING_FORMAT is fine.

[Paweł Hajdan Jr., 2010-06-30 15:19:53]

http://codereview.chromium.org/2846037/diff/1/2
File net/data/ftp/dir-listing-ls-17 (right):

http://codereview.chromium.org/2846037/diff/1/2#newcode1
net/data/ftp/dir-listing-ls-17:1: ftpd-BSD: .: Permission denied
On 2010/06/30 15:07:53, wtc wrote:
> Sounds like you didn't construct this test case.  Is this test case
> based on a real FTP directory listing?

Yes, that's the exact listing from the bug.