Bug 191498 – [Curl] implement CertificateInfo::summaryInfo

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Devin Rousso]

Used by WebInspector when showing secure certificate info per-request <https://webkit.org/b/191447>.

[Takashi Komori]

Created attachment 373699 [details]
Patch

[Fujii Hironori]

Comment on attachment 373699 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=373699&action=review

> Source/WebCore/platform/network/curl/CertificateInfoCurl.cpp:28
> +#include "OpenSSLHelper.h"

See https://webkit.org/code-style-guidelines/#include-config-h

> Source/WebCore/platform/network/curl/CertificateInfoCurl.cpp:168
> +                char buf[256] { 0 };

Remove '{ 0 }'. clang-cl reports a warning.
char buf[16];

> Source/WebCore/platform/network/curl/CertificateInfoCurl.cpp:169
> +                snprintf(buf, sizeof(buf), "%d.%d.%d.%d", data[0], data[1], data[2], data[3]);

I think WTF::makeString is better than snprintf. See also Bug 199452.

> Source/WebCore/platform/network/curl/CertificateInfoCurl.cpp:174
> +                for (int i = 0; i < 8; i++) {

Move 'buf' here.
char buf[5];

> Source/WebCore/platform/network/curl/OpenSSLHelper.h:181
> +class Ptr {

Can you use std::unique_ptr? You can specify a custom deleter. 
https://en.cppreference.com/w/cpp/memory/unique_ptr

> LayoutTests/platform/wincairo/TestExpectations:1021
> +webkit.org/b/191498 http/tests/inspector/network/getSerializedCertificate.html [ Pass ]

Can you just remove these two lines?

[Fujii Hironori]

Comment on attachment 373699 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=373699&action=review

> Source/WebCore/platform/network/curl/CertificateInfoCurl.cpp:122
> +    struct tm time { 0 };

clang-cl dislikes this. Replace '{ 0 }' with '{ }'.

> Source/WebCore/platform/network/curl/OpenSSLHelper.h:31
> +#include <wtf/vector.h>

This is can be compiled on Windows. Because it is using case-insensitive file system.
#include <wtf/Optional.h>
#include <wtf/Vector.h>

[Takashi Komori]

(In reply to Fujii Hironori from comment #2)
> > LayoutTests/platform/wincairo/TestExpectations:1021
> > +webkit.org/b/191498 http/tests/inspector/network/getSerializedCertificate.html [ Pass ]
> 
> Can you just remove these two lines?

As wincairo/TestExpectations skips whole http/tests/inspector/ directory first, we have to specify newly passed tests as [ Pass ].
If possible, we want specify "http/tests/inspector/network [ Pass ]" but we can't do that because not all inspector/network tests don't pass now on wincairo.

Is there any better way?

[Fujii Hironori]

(In reply to Takashi Komori from comment #4)

Got it. Np.

[Fujii Hironori]

Comment on attachment 373699 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=373699&action=review

> Source/WebCore/platform/network/curl/CertificateInfoCurl.cpp:143
> +        time.tm_sec = (data[8] - '0') * 10 + (data[9] - '0');

Is it safe to ignore fractional second and timezone parts?
What about using ASN1_TIME_to_tm?

[Fujii Hironori]

Comment on attachment 373699 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=373699&action=review

> Source/WebCore/platform/network/curl/CertificateInfo.h:76
> +        auto certificateChain = certificateInfo.certificateChain();

This is a common C++ pit-hole. The type of certificateInfo.certificateChain() is "const Vector<Certificate>&", but type of auto drops & and "const Vector<Certificate>".
Then, this sentence copies the Vector.
auto& certificateChain = certificateInfo.certificateChain();

> Source/WebCore/platform/network/curl/CertificateInfo.h:80
> +        for (auto certificate : certificateChain) {

auto& certificate

[Fujii Hironori]

Comment on attachment 373699 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=373699&action=review

>> Source/WebCore/platform/network/curl/CertificateInfo.h:76
>> +        auto certificateChain = certificateInfo.certificateChain();
> 
> This is a common C++ pit-hole. The type of certificateInfo.certificateChain() is "const Vector<Certificate>&", but type of auto drops & and "const Vector<Certificate>".
> Then, this sentence copies the Vector.
> auto& certificateChain = certificateInfo.certificateChain();

Not copying, but just ref-ing. However we should avoid such unnecessary ref-ing.

[Fujii Hironori]

Comment on attachment 373699 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=373699&action=review

> Source/WebCore/platform/network/curl/CertificateInfo.h:102
> +            WebCore::CertificateInfo::Certificate certificate(certSize);

Before allocating the buffer certificate, you should check the decoder remaining data size.
decoder.bufferIsLargeEnoughToContain<char>(certSize)
You can see other decoder example by grepping "bufferIsLargeEnoughToContain".

[Takashi Komori]

Created attachment 373889 [details]
Patch

[Takashi Komori]

(In reply to Fujii Hironori from comment #2)
> Comment on attachment 373699 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=373699&action=review
> 
> > Source/WebCore/platform/network/curl/CertificateInfoCurl.cpp:28
> > +#include "OpenSSLHelper.h"
> 
> See https://webkit.org/code-style-guidelines/#include-config-h
> 
> > Source/WebCore/platform/network/curl/CertificateInfoCurl.cpp:168
> > +                char buf[256] { 0 };
> 
> Remove '{ 0 }'. clang-cl reports a warning.
> char buf[16];
> 
> > Source/WebCore/platform/network/curl/CertificateInfoCurl.cpp:169
> > +                snprintf(buf, sizeof(buf), "%d.%d.%d.%d", data[0], data[1], data[2], data[3]);
> 
> I think WTF::makeString is better than snprintf. See also Bug 199452.
> 
> > Source/WebCore/platform/network/curl/CertificateInfoCurl.cpp:174
> > +                for (int i = 0; i < 8; i++) {
> 
> Move 'buf' here.
> char buf[5];

Stop using char buffer and snprintf.


> > Source/WebCore/platform/network/curl/OpenSSLHelper.h:181
> > +class Ptr {
> 
> Can you use std::unique_ptr? You can specify a custom deleter. 
> https://en.cppreference.com/w/cpp/memory/unique_ptr
> 

Fixed.

[Takashi Komori]

(In reply to Fujii Hironori from comment #3)
> Comment on attachment 373699 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=373699&action=review
> 
> > Source/WebCore/platform/network/curl/CertificateInfoCurl.cpp:122
> > +    struct tm time { 0 };
> 
> clang-cl dislikes this. Replace '{ 0 }' with '{ }'.

Fixed.

 
> > Source/WebCore/platform/network/curl/OpenSSLHelper.h:31
> > +#include <wtf/vector.h>
> 
> This is can be compiled on Windows. Because it is using case-insensitive
> file system.
> #include <wtf/Optional.h>
> #include <wtf/Vector.h>

Fiexed.

[Takashi Komori]

(In reply to Fujii Hironori from comment #6)
> Comment on attachment 373699 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=373699&action=review
> 
> > Source/WebCore/platform/network/curl/CertificateInfoCurl.cpp:143
> > +        time.tm_sec = (data[8] - '0') * 10 + (data[9] - '0');
> 
> Is it safe to ignore fractional second and timezone parts?
> What about using ASN1_TIME_to_tm?

Unfortunately libressl doesn't seem to have the function.

[Takashi Komori]

(In reply to Fujii Hironori from comment #8)
> Comment on attachment 373699 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=373699&action=review
> 
> >> Source/WebCore/platform/network/curl/CertificateInfo.h:76
> >> +        auto certificateChain = certificateInfo.certificateChain();
> > 
> > This is a common C++ pit-hole. The type of certificateInfo.certificateChain() is "const Vector<Certificate>&", but type of auto drops & and "const Vector<Certificate>".
> > Then, this sentence copies the Vector.
> > auto& certificateChain = certificateInfo.certificateChain();
> 
> Not copying, but just ref-ing. However we should avoid such unnecessary
> ref-ing.

Changed to auto&.

[Takashi Komori]

(In reply to Fujii Hironori from comment #9)
> Comment on attachment 373699 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=373699&action=review
> 
> > Source/WebCore/platform/network/curl/CertificateInfo.h:102
> > +            WebCore::CertificateInfo::Certificate certificate(certSize);
> 
> Before allocating the buffer certificate, you should check the decoder
> remaining data size.
> decoder.bufferIsLargeEnoughToContain<char>(certSize)
> You can see other decoder example by grepping "bufferIsLargeEnoughToContain".

decoder.decode() and decoder.decodeFixedLengthData() call bufferIsLargeEnoughToContain internally.
Do we have to call that before allocating Certificate again?

[Takashi Komori]

(In reply to Fujii Hironori from comment #6)
> Comment on attachment 373699 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=373699&action=review
> 
> > Source/WebCore/platform/network/curl/CertificateInfoCurl.cpp:143
> > +        time.tm_sec = (data[8] - '0') * 10 + (data[9] - '0');
> 
> Is it safe to ignore fractional second and timezone parts?


According to RFC, fractional second must not be included.

https://tools.ietf.org/html/rfc5280#section-4.1.2.5.2

[Fujii Hironori]

(In reply to Takashi Komori from comment #15)
> decoder.decode() and decoder.decodeFixedLengthData() call
> bufferIsLargeEnoughToContain internally.
> Do we have to call that before allocating Certificate again?

I don't know the reason.
Since this part is security sensitive, it think we should follow other code.
See Bug 43647, and 
https://github.com/WebKit/webkit/blob/71c68d0d57514f7cc2514d8d167468e5cc7e3a73/Source/WebKit/Platform/IPC/ArgumentCoders.cpp#L99

[Fujii Hironori]

(In reply to Fujii Hironori from comment #17)
> I don't know the reason.

I guess it can crash UI process if the compromised web process sends very large size.

[Fujii Hironori]

Comment on attachment 373889 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=373889&action=review

> Source/WebCore/platform/network/curl/CertificateInfoCurl.cpp:73
> +    if (commonNameEntryData < 0)

The type of return value of X509_NAME_ENTRY_get_data is ASN1_STRING pointer. Why do you compare a pointer with <0?

[Fujii Hironori]

Comment on attachment 373889 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=373889&action=review

> Source/WebCore/platform/network/curl/CertificateInfoCurl.cpp:76
> +    auto commonName = OpenSSL::createPtr<uint8_t>();

Looks so bad. The previous your code looks better: OpenSSL::Ptr commonName;
You can specify a lambda as deleter of std::unique_ptr. Google "lambda deleter std::unique_ptr".
You can improve more.

[Fujii Hironori]

Comment on attachment 373889 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=373889&action=review

>> Source/WebCore/platform/network/curl/CertificateInfoCurl.cpp:76
>> +    auto commonName = OpenSSL::createPtr<uint8_t>();
> 
> Looks so bad. The previous your code looks better: OpenSSL::Ptr commonName;
> You can specify a lambda as deleter of std::unique_ptr. Google "lambda deleter std::unique_ptr".
> You can improve more.

Sorry, I didn't understand the problem.
Now I understand why we shouldn't use std::unique_ptr for this purpose.

What about the following code?:

static auto toASN1String(const X509_NAME* name)
{
    auto deleter = [](unsigned char* ptr) {
                       if (ptr)
                           free(ptr);
    };
    unsigned char* buffer;
    ASN1_STRING_to_UTF8(&buffer, name);
    return std::unique_ptr<unsigned char[], decltype(deleter)>(buffer, deleter);
}

[Alex Christensen]

Comment on attachment 373889 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=373889&action=review

> Source/WebCore/ChangeLog:9
> +        This patch makes WebInspector show summary of certificates.

This sounds good.

> Source/WebCore/platform/network/curl/CertificateInfoCurl.cpp:104
> +static Optional<Seconds> convertASN1TimeToSeconds(const ASN1_TIME* ans1Time)

I think things like this ought to go in OpenSSLHelper because it's specific to OpenSSL, not libCURL.  Also, there's a lot of custom parsing code here.  It would be good to minimize that.

> Source/WebCore/platform/network/curl/OpenSSLHelper.h:41
> +    X509Ref(::X509* x509, bool isOwner = true)

I think this design is lending itself to strange lifetime management bugs without a good reason...

> Source/WebCore/platform/network/curl/OpenSSLHelper.h:107
> +    X509Ref item(unsigned i) { return X509Ref { sk_X509_value(m_certs, i), false}; }

... If this instead returned an X509* we wouldn't need this "sometimes an X509Ref points to something that something else manages" strangeness.

> Source/WebCore/platform/network/curl/OpenSSLHelper.h:181
> +using Ptr = std::unique_ptr<T*, WTF::Function<void(T**)>>;

I don't think this is a good name.  I think each type should have its own pointer type.  This is what boringssl did, and what I did in https://trac.webkit.org/changeset/244568/webkit

[Takashi Komori]

Created attachment 374451 [details]
Patch

[Takashi Komori]

(In reply to Fujii Hironori from comment #17)
> (In reply to Takashi Komori from comment #15)
> > decoder.decode() and decoder.decodeFixedLengthData() call
> > bufferIsLargeEnoughToContain internally.
> > Do we have to call that before allocating Certificate again?
> 
> I don't know the reason.
> Since this part is security sensitive, it think we should follow other code.
> See Bug 43647, and 
> https://github.com/WebKit/webkit/blob/
> 71c68d0d57514f7cc2514d8d167468e5cc7e3a73/Source/WebKit/Platform/IPC/
> ArgumentCoders.cpp#L99

Fixed.

[Takashi Komori]

(In reply to Fujii Hironori from comment #19)
> Comment on attachment 373889 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=373889&action=review
> 
> > Source/WebCore/platform/network/curl/CertificateInfoCurl.cpp:73
> > +    if (commonNameEntryData < 0)
> 
> The type of return value of X509_NAME_ENTRY_get_data is ASN1_STRING pointer.
> Why do you compare a pointer with <0?

Fixed.

[Takashi Komori]

(In reply to Alex Christensen from comment #22)
> Comment on attachment 373889 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=373889&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        This patch makes WebInspector show summary of certificates.
> 
> This sounds good.
> 
> > Source/WebCore/platform/network/curl/CertificateInfoCurl.cpp:104
> > +static Optional<Seconds> convertASN1TimeToSeconds(const ASN1_TIME* ans1Time)
> 
> I think things like this ought to go in OpenSSLHelper because it's specific
> to OpenSSL, not libCURL.  Also, there's a lot of custom parsing code here. 
> It would be good to minimize that.
> 

Thank you for your comments.
We moved it to OpenSSLHelper.cpp. 

> > Source/WebCore/platform/network/curl/OpenSSLHelper.h:41
> > +    X509Ref(::X509* x509, bool isOwner = true)
> 
> I think this design is lending itself to strange lifetime management bugs
> without a good reason...

Changed X509Ref more simpler.

> > Source/WebCore/platform/network/curl/OpenSSLHelper.h:107
> > +    X509Ref item(unsigned i) { return X509Ref { sk_X509_value(m_certs, i), false}; }
> 
> ... If this instead returned an X509* we wouldn't need this "sometimes an
> X509Ref points to something that something else manages" strangeness.

Changed to return X509*

> > Source/WebCore/platform/network/curl/OpenSSLHelper.h:181
> > +using Ptr = std::unique_ptr<T*, WTF::Function<void(T**)>>;
> 
> I don't think this is a good name.  I think each type should have its own
> pointer type.  This is what boringssl did, and what I did in
> https://trac.webkit.org/changeset/244568/webkit

We reconsidered names of unique pointers and changed.
Thank you for your advice! It was great help!

[Takashi Komori]

(In reply to Fujii Hironori from comment #20)
> Comment on attachment 373889 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=373889&action=review
> 
> > Source/WebCore/platform/network/curl/CertificateInfoCurl.cpp:76
> > +    auto commonName = OpenSSL::createPtr<uint8_t>();
> 
> Looks so bad. The previous your code looks better: OpenSSL::Ptr commonName;
> You can specify a lambda as deleter of std::unique_ptr. Google "lambda
> deleter std::unique_ptr".
> You can improve more.

By reconsidering how to use unique pointer, converting function became simpler.

[EWS Watchlist]

Comment on attachment 374451 [details]
Patch

Attachment 374451 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/12771851

New failing tests:
mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla-baseline
apiTests

[Alex Christensen]

Comment on attachment 374451 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=374451&action=review

> Source/WTF/wtf/persistence/PersistentCoders.h:153
> -        encoder.encodeFixedLengthData(reinterpret_cast<const uint8_t*>(vector.data()), vector.size() * sizeof(T), alignof(T));
> +        encoder.encodeFixedLengthData(reinterpret_cast<const uint8_t*>(vector.data()), vector.size() * sizeof(T));

Why does this no longer match ArgumentCoders.h? Why did you make this change?

> Source/WebCore/platform/network/curl/CertificateInfo.h:84
> +    static bool decode(Decoder& decoder, WebCore::CertificateInfo& certificateInfo)

Please use modern decoding that returns an Optional<WebCore::CertificateInfo>. Decoders that return a bool prevent us from removing default constructors in decodable classes.

> Source/WebCore/platform/network/curl/OpenSSLHelper.cpp:56
> +        if (*ptr)
> +            OPENSSL_free(*ptr);

How do we know there will only be one thing needing OPENSSL_free here?

[Takashi Komori]

(In reply to Alex Christensen from comment #29)
> Comment on attachment 374451 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=374451&action=review
> 
> > Source/WTF/wtf/persistence/PersistentCoders.h:153
> > -        encoder.encodeFixedLengthData(reinterpret_cast<const uint8_t*>(vector.data()), vector.size() * sizeof(T), alignof(T));
> > +        encoder.encodeFixedLengthData(reinterpret_cast<const uint8_t*>(vector.data()), vector.size() * sizeof(T));
> 
> Why does this no longer match ArgumentCoders.h? Why did you make this change?

ArgumentCoders.h and PresistentCoders.h are alike but they are unrelated.
Because they are separated by namespace, changing PersistentCoders doesn't affect ArgumentCoders.

This change aimed to use Persistence::VectorCoder to simplify code.
Before the change, argument number of Persistence::Encoder::encodeFixedLengthData was wrong but this template function didn't raise compile errors as it is not reffered anywhere and didn't raise compile errors.

> > Source/WebCore/platform/network/curl/CertificateInfo.h:84
> > +    static bool decode(Decoder& decoder, WebCore::CertificateInfo& certificateInfo)
> 
> Please use modern decoding that returns an
> Optional<WebCore::CertificateInfo>. Decoders that return a bool prevent us
> from removing default constructors in decodable classes.

Other ports use PersistentDecoders for encoding/decoding certificates too and replacing coder affects other ports.
Should we replace PersistentCoders by ArgumentCoders or something modern coder?
 
> > Source/WebCore/platform/network/curl/OpenSSLHelper.cpp:56
> > +        if (*ptr)
> > +            OPENSSL_free(*ptr);
> 
> How do we know there will only be one thing needing OPENSSL_free here?

Do you mean we should consider not only buffer we got from ASN1_STRING_to_UTF8other but also other type objects we have to free with OPENSSL_free?

[Alex Christensen]

Comment on attachment 374451 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=374451&action=review

>>> Source/WebCore/platform/network/curl/OpenSSLHelper.cpp:56
>>> +            OPENSSL_free(*ptr);
>> 
>> How do we know there will only be one thing needing OPENSSL_free here?
> 
> Do you mean we should consider not only buffer we got from ASN1_STRING_to_UTF8other but also other type objects we have to free with OPENSSL_free?

If this is only to be used for that type, it should not be in a deleter<unsigned char**>, but rather in a class type that can only be used for a buffer you got from ASN1_STRING_to_UTF8other

[Takashi Komori]

Created attachment 375068 [details]
Patch

[Takashi Komori]

(In reply to Alex Christensen from comment #31)
> Comment on attachment 374451 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=374451&action=review
> 
> >>> Source/WebCore/platform/network/curl/OpenSSLHelper.cpp:56
> >>> +            OPENSSL_free(*ptr);
> >> 
> >> How do we know there will only be one thing needing OPENSSL_free here?
> > 
> > Do you mean we should consider not only buffer we got from ASN1_STRING_to_UTF8other but also other type objects we have to free with OPENSSL_free?
> 
> If this is only to be used for that type, it should not be in a
> deleter<unsigned char**>, but rather in a class type that can only be used
> for a buffer you got from ASN1_STRING_to_UTF8other

Stopped using deleter<unsigned char**> because we can free buffer locally in convert function.

Stopped using deleter<BUF_MEM> and add getting data functions.

[Fujii Hironori]

Comment on attachment 375068 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=375068&action=review

> Source/WebCore/platform/network/curl/OpenSSLHelper.cpp:46
> +using X509Ref = std::unique_ptr<X509, deleter<X509>>;

I don't think X509Ref is a good name because this is not ref-counted object and this can be nullptr.
I think you don't need to type aliasing becuase this type is used only once.

> Source/WebCore/platform/network/curl/OpenSSLHelper.cpp:138
> +        if (auto x509 = ::PEM_read_bio_X509(m_bio, nullptr, 0, nullptr))

return X509Ref(::PEM_read_bio_X509(m_bio, nullptr, 0, nullptr));

> Source/WebCore/platform/network/curl/OpenSSLHelper.cpp:184
> +    return WTFMove(result);

You should remove WTFMove.
return result;
https://lists.webkit.org/pipermail/webkit-dev/2019-March/030548.html

> Source/WebCore/platform/network/curl/OpenSSLHelper.cpp:205
> +    auto commonName = toString(commonNameEntryData);

How about simply returning the return value of toString?
return toString(commonNameEntryData);

[Takashi Komori]

Created attachment 375286 [details]
Patch

[Takashi Komori]

(In reply to Fujii Hironori from comment #34)
> Comment on attachment 375068 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=375068&action=review
> 
> > Source/WebCore/platform/network/curl/OpenSSLHelper.cpp:46
> > +using X509Ref = std::unique_ptr<X509, deleter<X509>>;
> 
> I don't think X509Ref is a good name because this is not ref-counted object
> and this can be nullptr.
> I think you don't need to type aliasing becuase this type is used only once.

Removed type aliasing.

> > Source/WebCore/platform/network/curl/OpenSSLHelper.cpp:138
> > +        if (auto x509 = ::PEM_read_bio_X509(m_bio, nullptr, 0, nullptr))
> 
> return X509Ref(::PEM_read_bio_X509(m_bio, nullptr, 0, nullptr));

Fixed.
 
> > Source/WebCore/platform/network/curl/OpenSSLHelper.cpp:184
> > +    return WTFMove(result);
> 
> You should remove WTFMove.
> return result;
> https://lists.webkit.org/pipermail/webkit-dev/2019-March/030548.html

Fixed.

> > Source/WebCore/platform/network/curl/OpenSSLHelper.cpp:205
> > +    auto commonName = toString(commonNameEntryData);
> 
> How about simply returning the return value of toString?
> return toString(commonNameEntryData);

Fixed.

[EWS Watchlist]

Comment on attachment 375286 [details]
Patch

Attachment 375286 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/12845399

New failing tests:
mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla-llint

[Alex Christensen]

Comment on attachment 375286 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=375286&action=review

r=me with a few nits.
ews failure is unrelated

> Source/WebCore/platform/network/curl/CertificateInfo.h:-79
> -    static bool decode(Decoder&, WebCore::CertificateInfo&)

Please use modern decoding that returns an Optional<WebCore::CertificateInfo>. Decoders that return a bool prevent us from removing default constructors in decodable classes.

> Source/WebCore/platform/network/curl/OpenSSLHelper.cpp:171
> +static Optional<String> toString(const ASN1_STRING* name)

These can just return a String, which has a null state.

[Takashi Komori]

(In reply to Alex Christensen from comment #38)
> > Source/WebCore/platform/network/curl/CertificateInfo.h:-79
> > -    static bool decode(Decoder&, WebCore::CertificateInfo&)
> 
> Please use modern decoding that returns an
> Optional<WebCore::CertificateInfo>. Decoders that return a bool prevent us
> from removing default constructors in decodable classes.

Changing decoder's style affects other ports so we will fix it in this ticket.
https://bugs.webkit.org/show_bug.cgi?id=200387

[Takashi Komori]

Created attachment 375520 [details]
Patch

[Takashi Komori]

(In reply to Alex Christensen from comment #38)
> Comment on attachment 375286 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=375286&action=review
>
> > Source/WebCore/platform/network/curl/OpenSSLHelper.cpp:171
> > +static Optional<String> toString(const ASN1_STRING* name)
> 
> These can just return a String, which has a null state.

Changed to return String.

[Takashi Komori]

Created attachment 375522 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 375522 [details]
Patch for landing

Clearing flags on attachment: 375522

Committed r248268: <https://trac.webkit.org/changeset/248268>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/53931826>