Bug 191498 - [Curl] implement CertificateInfo::summaryInfo
Summary: [Curl] implement CertificateInfo::summaryInfo
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: Curl, InRadar
Depends on: 191447
Blocks:
  Show dependency treegraph
 
Reported: 2018-11-09 16:30 PST by Devin Rousso
Modified: 2019-08-05 00:35 PDT (History)
10 users (show)

See Also:


Attachments
Patch (23.21 KB, patch)
2019-07-08 21:06 PDT, Takashi Komori
no flags Details | Formatted Diff | Diff
Patch (22.72 KB, patch)
2019-07-10 18:46 PDT, Takashi Komori
no flags Details | Formatted Diff | Diff
Patch (28.66 KB, patch)
2019-07-19 01:50 PDT, Takashi Komori
no flags Details | Formatted Diff | Diff
Patch (28.46 KB, patch)
2019-07-29 00:57 PDT, Takashi Komori
no flags Details | Formatted Diff | Diff
Patch (28.23 KB, patch)
2019-07-31 20:39 PDT, Takashi Komori
no flags Details | Formatted Diff | Diff
Patch (28.12 KB, patch)
2019-08-04 22:41 PDT, Takashi Komori
no flags Details | Formatted Diff | Diff
Patch for landing (28.13 KB, patch)
2019-08-04 23:42 PDT, Takashi Komori
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2018-11-09 16:30:06 PST
Used by WebInspector when showing secure certificate info per-request <https://webkit.org/b/191447>.
Comment 1 Takashi Komori 2019-07-08 21:06:15 PDT
Created attachment 373699 [details]
Patch
Comment 2 Fujii Hironori 2019-07-08 21:41:05 PDT
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?
Comment 3 Fujii Hironori 2019-07-08 21:51:37 PDT
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>
Comment 4 Takashi Komori 2019-07-08 23:25:20 PDT
(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?
Comment 5 Fujii Hironori 2019-07-08 23:45:00 PDT
(In reply to Takashi Komori from comment #4)

Got it. Np.
Comment 6 Fujii Hironori 2019-07-09 00:09:12 PDT
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?
Comment 7 Fujii Hironori 2019-07-09 00:15:04 PDT
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
Comment 8 Fujii Hironori 2019-07-09 00:17:33 PDT
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.
Comment 9 Fujii Hironori 2019-07-09 01:00:55 PDT
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".
Comment 10 Takashi Komori 2019-07-10 18:46:52 PDT
Created attachment 373889 [details]
Patch
Comment 11 Takashi Komori 2019-07-10 18:55:16 PDT
(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.
Comment 12 Takashi Komori 2019-07-10 18:56:12 PDT
(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.
Comment 13 Takashi Komori 2019-07-10 19:24:37 PDT
(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.
Comment 14 Takashi Komori 2019-07-10 19:27:36 PDT
(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&.
Comment 15 Takashi Komori 2019-07-10 19:28:16 PDT
(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?
Comment 16 Takashi Komori 2019-07-10 20:23:11 PDT
(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
Comment 17 Fujii Hironori 2019-07-11 00:24:22 PDT
(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
Comment 18 Fujii Hironori 2019-07-11 00:31:59 PDT
(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.
Comment 19 Fujii Hironori 2019-07-11 03:44:13 PDT
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?
Comment 20 Fujii Hironori 2019-07-11 04:09:21 PDT
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.
Comment 21 Fujii Hironori 2019-07-11 06:55:26 PDT
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);
}
Comment 22 Alex Christensen 2019-07-12 11:50:07 PDT
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
Comment 23 Takashi Komori 2019-07-19 01:50:39 PDT
Created attachment 374451 [details]
Patch
Comment 24 Takashi Komori 2019-07-19 01:51:41 PDT
(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.
Comment 25 Takashi Komori 2019-07-19 01:52:07 PDT
(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.
Comment 26 Takashi Komori 2019-07-19 02:02:15 PDT
(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!
Comment 27 Takashi Komori 2019-07-19 02:13:40 PDT
(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.
Comment 28 Build Bot 2019-07-19 03:48:39 PDT
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
Comment 29 Alex Christensen 2019-07-19 09:08:29 PDT
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?
Comment 30 Takashi Komori 2019-07-22 20:16:04 PDT
(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?
Comment 31 Alex Christensen 2019-07-23 12:55:44 PDT
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
Comment 32 Takashi Komori 2019-07-29 00:57:45 PDT
Created attachment 375068 [details]
Patch
Comment 33 Takashi Komori 2019-07-29 01:04:31 PDT
(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.
Comment 34 Fujii Hironori 2019-07-30 22:04:02 PDT
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);
Comment 35 Takashi Komori 2019-07-31 20:39:24 PDT
Created attachment 375286 [details]
Patch
Comment 36 Takashi Komori 2019-07-31 20:41:27 PDT
(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.
Comment 37 Build Bot 2019-08-01 00:27:33 PDT
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
Comment 38 Alex Christensen 2019-08-01 07:04:49 PDT
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.
Comment 39 Takashi Komori 2019-08-02 03:47:20 PDT
(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
Comment 40 Takashi Komori 2019-08-04 22:41:21 PDT
Created attachment 375520 [details]
Patch
Comment 41 Takashi Komori 2019-08-04 22:43:27 PDT
(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.
Comment 42 Takashi Komori 2019-08-04 23:42:00 PDT
Created attachment 375522 [details]
Patch for landing
Comment 43 WebKit Commit Bot 2019-08-05 00:34:30 PDT
Comment on attachment 375522 [details]
Patch for landing

Clearing flags on attachment: 375522

Committed r248268: <https://trac.webkit.org/changeset/248268>
Comment 44 WebKit Commit Bot 2019-08-05 00:34:32 PDT
All reviewed patches have been landed.  Closing bug.
Comment 45 Radar WebKit Bug Importer 2019-08-05 00:35:22 PDT
<rdar://problem/53931826>