Skip to content

Commit 4a099e8

Browse files
jrhodnikJohn Hodnikclaude
authored and
amaitland
committed
Core - OnSelectClientCertificate own the copied certificate vector (#5281)
* Fix deferred client certificate selection reading freed memory CefCertificateCallbackWrapper held the offered certificate list as `const X509CertificateList&`, bound to a stack local built in ClientAdapter::OnSelectClientCertificate. Once that handler returned the list was destroyed, so calling Select() at any later point walked freed memory and threw inside the thumbprint-matching loop, taking the host process down with it. CEF explicitly permits answering later. cef_request_handler.h says to return true and call Select "either in this method or at a later time", so a wrapper that outlives the handler has to own the list it selects from. It now holds a heap-allocated copy, freed in the finalizer. A ref class cannot contain a std::vector by value, hence the pointer. Copying the vector copies the reference-counted CefX509Certificate pointers, and those references are what keep the certificates alive. This is the remaining half of #2948. The comment above the caller reads "Create a copy of the vector in an attempt to fix #2948", and the copy is indeed made - but it is then bound by reference, so it dies at the same instant the original would have. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Remove the redundant certificate vector copy in ClientAdapter The wrapper takes its own copy in its constructor, so the local copy added by a51cdd3 in ClientAdapter::OnSelectClientCertificate is now pure redundancy - two copies where one is needed. Pass `certificates` straight through instead. Also refreshes the comment on _certificateList, which described the caller's stack local that this removes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: John Hodnik <jhodnik@activu.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent d24556a commit 4a099e8

2 files changed

Lines changed: 18 additions & 9 deletions

File tree

‎CefSharp.Core.Runtime/Internals/CefCertificateCallbackWrapper.h‎

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -19,18 +19,29 @@ namespace CefSharp
1919
{
2020
private:
2121
MCefRefPtr<CefSelectClientCertificateCallback> _callback;
22-
const CefRequestHandler::X509CertificateList& _certificateList;
22+
// Owned copy of the certificates Chromium offered, not a reference to the caller's list.
23+
// That list belongs to CEF for the duration of ClientAdapter::OnSelectClientCertificate,
24+
// and this wrapper deliberately outlives that call - CEF allows Select to be called
25+
// "either in this method or at a later time" - so a reference would dangle the moment
26+
// the handler returns and a deferred Select would read freed memory.
27+
// A ref class cannot hold a std::vector by value, hence the pointer. Copying the vector
28+
// copies the reference-counted CefX509Certificate pointers, and those references are
29+
// what keep the certificates themselves alive.
30+
CefRequestHandler::X509CertificateList* _certificateList;
2331

2432
public:
2533
CefCertificateCallbackWrapper(CefRefPtr<CefSelectClientCertificateCallback>& callback, const CefRequestHandler::X509CertificateList& certificates)
26-
: _callback(callback), _certificateList(certificates)
34+
: _callback(callback), _certificateList(new CefRequestHandler::X509CertificateList(certificates))
2735
{
2836

2937
}
3038

3139
!CefCertificateCallbackWrapper()
3240
{
3341
_callback = nullptr;
42+
43+
delete _certificateList;
44+
_certificateList = nullptr;
3445
}
3546

3647
~CefCertificateCallbackWrapper()
@@ -53,8 +64,8 @@ namespace CefSharp
5364
auto certThumbprint = cert->Thumbprint;
5465

5566
std::vector<CefRefPtr<CefX509Certificate>>::const_iterator it =
56-
_certificateList.begin();
57-
for (; it != _certificateList.end(); ++it)
67+
_certificateList->begin();
68+
for (; it != _certificateList->end(); ++it)
5869
{
5970
auto bytes((*it)->GetDEREncoded());
6071
auto byteSize = bytes->GetSize();

‎CefSharp.Core.Runtime/Internals/ClientAdapter.cpp‎

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -764,8 +764,6 @@ namespace CefSharp
764764
auto browserWrapper = GetBrowserWrapper(browser->GetIdentifier(), browser->IsPopup());
765765

766766
auto list = gcnew X509Certificate2Collection();
767-
// Create a copy of the vector in an attempt to fix #2948
768-
CefRequestHandler::X509CertificateList certs;
769767

770768
std::vector<CefRefPtr<CefX509Certificate> >::const_iterator it =
771769
certificates.begin();
@@ -780,11 +778,11 @@ namespace CefSharp
780778
bytes->GetData(static_cast<void*>(src), byteSize, 0);
781779
auto cert = gcnew X509Certificate2(bufferByte);
782780
list->Add(cert);
783-
784-
certs.push_back(*it);
785781
}
786782

787-
auto callbackWrapper = gcnew CefCertificateCallbackWrapper(callback, certs);
783+
// Passed straight through. The wrapper takes its own reference to each certificate, so
784+
// there is no need to copy the vector here.
785+
auto callbackWrapper = gcnew CefCertificateCallbackWrapper(callback, certificates);
788786

789787
return handler->OnSelectClientCertificate(
790788
_browserControl, browserWrapper, isProxy,

0 commit comments

Comments
 (0)