Core - OnSelectClientCertificate own the copied certificate vector - #5281
Core - OnSelectClientCertificate own the copied certificate vector#5281jrhodnik wants to merge 1 commit into
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 (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: ⚪ Minimal · up to This localized fix keeps the copied certificate list alive for deferred selection without changing existing caller behavior. No actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 @) |
2 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