Assert HttpNetWorkTransaction::GetResponseInfo() should never retun NULL when certificate error

Assert that HttpNetworkTransaction::GetResponseInfo() should
never return NULL when we get a certificate error.

R=wtc
BUG=11646
TEST=access https://www.cdep.ro/


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=16144

[wtc, 2009-05-13 20:34:59]

Hi Fumitoshi,

Thanks a lot for the changelist.  Browser process crashes
are a serious problem.  I am investigating the SSL handshake
error so that I know exactly what goes wrong.  So far I
found that the server requests SSL client authentication,
but Chromium doesn't support SSL client authentication, so
the server may have sent us an SSL alert message that is
reported by Windows Schannel as SEC_E_CERT_UNKNOWN, which
we map to ERR_CERT_INVALID.
http://msdn.microsoft.com/en-us/library/dd721886%28VS.85%29.aspx

I think we should fix this crash as follows:
1. Ensure that we never report a certificate error when
the SSL handshake fails.
2. If we get SEC_E_CERT_UNKNOWN during SSL handshake,
map it to an error other than ERR_CERT_INVALID, so that
it won't be misinterpreted as a (server) certificate
error.  (Here, it is the client cert that is invalid,
complained by the server.)

I will need to inspect the SSL packet trace to understand
more.  I will also test your patch to understand what it
does.

http://codereview.chromium.org/113330/diff/1/2
File net/http/http_cache.cc (right):

http://codereview.chromium.org/113330/diff/1/2#newcode942
Line 942: if (response)
I think it's better to keep the assumption that if we get
a certificate error, then there is a certificate in ssl_info.

[ukai, 2009-05-14 08:42:39]

On 2009/05/13 20:34:59, wtc wrote:
> Hi Fumitoshi,
> 
> Thanks a lot for the changelist.  Browser process crashes
> are a serious problem.  I am investigating the SSL handshake
> error so that I know exactly what goes wrong.  So far I
> found that the server requests SSL client authentication,
> but Chromium doesn't support SSL client authentication, so
> the server may have sent us an SSL alert message that is
> reported by Windows Schannel as SEC_E_CERT_UNKNOWN, which
> we map to ERR_CERT_INVALID.
> http://msdn.microsoft.com/en-us/library/dd721886%28VS.85%29.aspx
> 
> I think we should fix this crash as follows:
> 1. Ensure that we never report a certificate error when
> the SSL handshake fails.
> 2. If we get SEC_E_CERT_UNKNOWN during SSL handshake,
> map it to an error other than ERR_CERT_INVALID, so that
> it won't be misinterpreted as a (server) certificate
> error.  (Here, it is the client cert that is invalid,
> complained by the server.)

I agree.

> I will need to inspect the SSL packet trace to understand
> more.  I will also test your patch to understand what it
> does.

Thanks.  
It seems you addressed this issue by r16026.
Do you think we still need this change?
> 
> http://codereview.chromium.org/113330/diff/1/2
> File net/http/http_cache.cc (right):
> 
> http://codereview.chromium.org/113330/diff/1/2#newcode942
> Line 942: if (response)
> I think it's better to keep the assumption that if we get
> a certificate error, then there is a certificate in ssl_info.

[ukai, 2009-05-14 08:42:51]

http://codereview.chromium.org/113330/diff/1/2
File net/http/http_cache.cc (right):

http://codereview.chromium.org/113330/diff/1/2#newcode942
Line 942: if (response)
On 2009/05/13 20:34:59, wtc wrote:
> I think it's better to keep the assumption that if we get
> a certificate error, then there is a certificate in ssl_info.

Done.
I add CHECK(response) to make it easily find when our assumption is broken.

[wtc, 2009-05-14 18:15:01]

Hi Fumitoshi,

I think we can keep your change to http_cache.cc.
Please remove the other two files from this changelist

Remember to "Edit Issue" to update the description of
the CL.  Thanks!

http://codereview.chromium.org/113330/diff/1004/1005
File net/http/http_cache.cc (right):

http://codereview.chromium.org/113330/diff/1004/1005#newcode943
Line 943: // so GetResponseInfo() never returns NULL.
Nit: say
  so GetResponseInfo() should never return NULL here.

[rvargas (doing something else), 2009-05-14 21:05:36]

I don't see a lot of value in keeping the change. I mean, if we don't check the
value returned by GetResponseInfo we'll be crashing immediately with an access
violation through the null pointer, and that's a pretty common pattern in our
code, and it's also quite easy to spot from a crash report.

Maybe just a DCHECK?


On 2009/05/14 18:15:01, wtc wrote:
> Hi Fumitoshi,
> 
> I think we can keep your change to http_cache.cc.
> Please remove the other two files from this changelist
> 
> Remember to "Edit Issue" to update the description of
> the CL.  Thanks!
> 
> http://codereview.chromium.org/113330/diff/1004/1005
> File net/http/http_cache.cc (right):
> 
> http://codereview.chromium.org/113330/diff/1004/1005#newcode943
> Line 943: // so GetResponseInfo() never returns NULL.
> Nit: say
>   so GetResponseInfo() should never return NULL here.

[ukai, 2009-05-15 03:25:24]

Thanks for review.
I also updated the description.

http://codereview.chromium.org/113330/diff/1004/1005
File net/http/http_cache.cc (right):

http://codereview.chromium.org/113330/diff/1004/1005#newcode943
Line 943: // so GetResponseInfo() never returns NULL.
On 2009/05/14 18:15:01, wtc wrote:
> Nit: say
>   so GetResponseInfo() should never return NULL here.

Done.

http://codereview.chromium.org/113330/diff/1010/1011#newcode944
Line 944: CHECK(response);
Changed DCHECK here.

[wtc, 2009-05-15 03:41:11]

LGTM.

ukai: I changed "Check" to "Assert" in the CL's description.
"Check" is a little ambiguous.  "Assert" implies it's an
assertion (the DCHECK macro).

rvargas: I agree the CHECK/DCHECK isn't that useful.  The
value is in the comment, which will point us in the right
direction if we crash there again in the future.

[ukai, 2009-05-15 03:43:36]

On 2009/05/15 03:41:11, wtc wrote:
> LGTM.

Thanks! Submitted.
 
> ukai: I changed "Check" to "Assert" in the CL's description.
> "Check" is a little ambiguous.  "Assert" implies it's an
> assertion (the DCHECK macro).

Got it.
> 
> rvargas: I agree the CHECK/DCHECK isn't that useful.  The
> value is in the comment, which will point us in the right
> direction if we crash there again in the future.