[webkit-reviews] review denied: [Bug 119436] [curl] Improve detecting and handling of SSL related errors : [Attachment 217023] proposed patch

[bugzilla-daemon at webkit.org]

Brent Fulgham <bfulgham at webkit.org> has denied sipka at inf.u-szeged.hu's request
for review:
Bug 119436: [curl] Improve detecting and handling of SSL related errors
https://bugs.webkit.org/show_bug.cgi?id=119436

Attachment 217023: proposed patch
https://bugs.webkit.org/attachment.cgi?id=217023&action=review

------- Additional Comments from Brent Fulgham <bfulgham at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=217023&action=review


Looks very good.  Looking at this in more detail I had a few questions, as well
as a concern about initialization of m_sslErrors.

> Source/WebCore/platform/network/ResourceHandleInternal.h:182
> +	   unsigned m_sslErrors;

This will not get initialized on other platforms. Maybe this should be
protected by #if USE(CURL) or something?

> Source/WebCore/platform/network/curl/ResourceHandleCurl.cpp:121
> +    allowsAnyHTTPSCertificateHosts(host.lower());

This says the key is always lower-case...

> Source/WebCore/platform/network/curl/SSLHandle.cpp:40
> +void allowsAnyHTTPSCertificateHosts(const String& host)

... host always seems to be supplied as lower case.

> Source/WebCore/platform/network/curl/SSLHandle.cpp:54
> +	       it->value = cert;

What is the case-ness of 'cert'?  Is it always lower case? Mixed case?

> Source/WebCore/platform/network/curl/SSLHandle.cpp:57
> +	   if (it->value == cert)

This comparison assumes that the certificate state in allowedHosts is always
the same as the passed cert. Do we need case insensitive compare?

> Source/WebCore/platform/network/curl/SSLHandle.cpp:134
> +    long len = BIO_get_mem_data(bio, &data);

len is signed. If it can be negative (e.g., error case) the following code will
cause problems.

> Source/WebCore/platform/network/curl/SSLHandle.cpp:136
> +    certificate = data;

Do we care about the case provided by the certificate store?

> Source/WebCore/platform/network/curl/SSLHandle.cpp:165
> +    ok = sslIgnoreHTTPSCertificate(host.lower(), certificate);

So should we be using "certificate.lower()" here?