Core - OnSelectClientCertificate own the copied certificate vector - #5281
Conversation
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 cefsharp#2948. The comment above the caller reads "Create a copy of the vector in an attempt to fix cefsharp#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>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthrough
ChangesCertificate callback lifetime
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: 🟡 Moderate · up to The PR fixes deferred client-certificate selection by retaining the offered certificates after the request handler returns. However, repeated or concurrent Select calls are not clearly constrained, so native callback cleanup could race and cause instability or memory-safety problems; this should be addressed or explicitly accepted before merging. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Title checkExplanation The title clearly identifies the main change: CefSharp now owns a copied certificate vector for OnSelectClientCertificate. The wording is slightly ungrammatical but remains specific and understandable. Full details: Linked Issues checkExplanation The changes address issue
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ Build CefSharp 151.3.160-CI5592 completed (commit a1db8af689 by @) |
amaitland
left a comment
amaitland
left a comment
There was a problem hiding this comment.
Thanks for the PR, see comment lineline.
| MCefRefPtr<CefSelectClientCertificateCallback> _callback; | ||
| const CefRequestHandler::X509CertificateList& _certificateList; | ||
| // Owned copy of the certificates Chromium offered, not a reference to the caller's. | ||
| // ClientAdapter::OnSelectClientCertificate builds that list as a stack local, so a |
There was a problem hiding this comment.
On the surface fixing that instead seems like a better long term solution.
At a minimum we should remove the extra copy
Thoughts?
There was a problem hiding this comment.
Had a slightly more detailed look at the existing code, I think having the callback wrapper create the CefRequestHandler::X509CertificateList that it in tern owns seems reasonable, so we just need to cleanup ClientAdapter::OnSelectClientCertificate
There was a problem hiding this comment.
Makes sense — done. ClientAdapter::OnSelectClientCertificate no longer builds its own copy, it just passes certificates straight through, and the wrapper owns the copy as before.
Also updated the comment on _certificateList, since it described the caller's stack local that this removes.
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>
|
✅ Build CefSharp 151.3.160-CI5599 completed (commit cecf76fe7e by @) |
…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>
3 participants
Fixes: #2948
Summary:
ISelectClientCertificateCallback.Selectstill throws when called afterOnSelectClientCertificatehas returned, so the deferred form documented by CEF ("Return true and callCefSelectClientCertificateCallback::Selecteither in this method or at a later time") cannot be usedCefCertificateCallbackWrapper.his unchanged on masterCefCertificateCallbackWrapperstores it asconst X509CertificateList&and the copy is a local inClientAdapter::OnSelectClientCertificate, so it is destroyed when the handler returns, exactly as the original wasInvokeOnUiThreadIfRequiredso the certificate is selected on the handler's own thread, is the same observation from the other directionChanges: [specify the structures changed]
CefCertificateCallbackWrapperconst CefRequestHandler::X509CertificateList&BrowserSettings,CefSettingsBase,RequestContextSettingsandWindowInfoSelectiterates via the pointerCefX509Certificatepointers, which is what keeps the certificates alive. A ref class cannot hold astd::vector(or a smart pointer) by value, which is why the field was a reference to begin withHow Has This Been Tested?
Built the NETCore packages for x86 and x64 from the v144.0.270 tag with this change, on VS2022 / Windows 10, and loaded servers that require a client certificate and servers that merely request one.
Deferred path, the failing case: held the callback, returned
truefromOnSelectClientCertificate, then calledSelectafterwards from another thread.Also re-checked the two paths that already worked, both unchanged: calling
Selectinside the handler, and callingSelect(nullptr)to continue without a certificate.Screenshots (if appropriate):
Types of changes
Checklist:
One point worth a maintainer's opinion: with deferral working,
Selectbecomes reachable from a thread other than the CEF UI thread. The header's "or at a later time" wording reads as intended, and it works in practice, but ifSelectis meant to be UI-thread-only then that is worth documenting.Summary by CodeRabbit