Summary: | [Curl] implement CertificateInfo::summaryInfo | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Devin Rousso <hi> | ||||||||||||||||
Component: | Platform | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | achristensen, Basuke.Suzuki, commit-queue, don.olmstead, ews-watchlist, galpeter, hi, Hironori.Fujii, takashi.komori, webkit-bug-importer | ||||||||||||||||
Priority: | P2 | Keywords: | Curl, InRadar | ||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||
Hardware: | All | ||||||||||||||||||
OS: | All | ||||||||||||||||||
Bug Depends on: | 191447 | ||||||||||||||||||
Bug Blocks: | |||||||||||||||||||
Attachments: |
|
Description
Devin Rousso
2018-11-09 16:30:06 PST
Created attachment 373699 [details]
Patch
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 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> (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? (In reply to Takashi Komori from comment #4) Got it. Np. 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 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 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 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". Created attachment 373889 [details]
Patch
(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. (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. (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. (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&. (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? (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 (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 (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 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 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 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 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 Created attachment 374451 [details]
Patch
(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. (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. (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! (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 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 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? (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 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 Created attachment 375068 [details]
Patch
(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 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); Created attachment 375286 [details]
Patch
(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 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 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. (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 Created attachment 375520 [details]
Patch
(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. Created attachment 375522 [details]
Patch for landing
Comment on attachment 375522 [details] Patch for landing Clearing flags on attachment: 375522 Committed r248268: <https://trac.webkit.org/changeset/248268> All reviewed patches have been landed. Closing bug. |