Make Curl CA path ownership safe#1464
Merged
bmehta001 merged 9 commits intoJun 9, 2026
Merged
Conversation
Split the Curl CA path fix into a smaller review slice. Store CA path strings with stable ownership so Curl options do not keep pointers to transient or externally-owned string data. Files changed: lib/http/HttpClient_Curl.hpp Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves the safety of libcurl option configuration by ensuring the CA bundle path passed to CURLOPT_CAINFO has stable ownership for the lifetime of a CurlHttpOperation.
Changes:
- Persist the
sslCaInfostring insideCurlHttpOperation(m_sslCaInfo) instead of relying on a potentially transient caller-owned string. - Update
CURLOPT_CAINFOsetup to use the ownedm_sslCaInfobuffer.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
bmehta001
force-pushed
the
bhamehta/curl-ca-path-ownership
branch
from
ec189ee to
1367e82
Compare
Explain why CurlHttpOperation owns the CA path string but intentionally does not copy the request body. The SDK upload path keeps the request alive until Send completes, and copying the body would duplicate every upload payload. Files changed: lib/http/HttpClient_Curl.hpp Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Explain that request headers are copied into the owned curl_slist during construction, so CurlHttpOperation does not need to keep the original header map alive for the operation lifetime. Files changed: lib/http/HttpClient_Curl.hpp Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
baijumeswani
reviewed
Jun 5, 2026
Co-authored-by: Baiju Meswani <baijumeswani@gmail.com>
Apply the reviewed header-loop cleanup and modernize a few local internals in HttpClient_Curl.hpp without changing behavior. This preserves the existing request-body lifetime model while reducing unnecessary casts/null usage and avoiding loss of the old response buffer if realloc fails. Files changed: lib/http/HttpClient_Curl.hpp Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
baijumeswani
approved these changes
Jun 6, 2026
baijumeswani
left a comment
baijumeswani
left a comment
Contributor
There was a problem hiding this comment.
looks good. copilot had some suggestions. not sure if they need to be addressed.
Remove the defaulted temporary arguments for requestHeaders and requestBody so a caller can no longer bind a temporary to the requestBody reference member, which Send() reads after construction. Production already passes owned values (HttpClient_Curl.cpp); update the unit tests to pass fixture-lifetime headers/body instead of temporaries. Addresses the Copilot review note about a potentially dangling requestBody reference. Files changed: - lib/http/HttpClient_Curl.hpp - tests/unittests/HttpClientCurlTests.cpp Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The constructor comment claimed both requestHeaders and requestBody must outlive the operation, but requestHeaders is only copied into the curl_slist during construction; only requestBody is stored by reference and read by Send(). Update the comment so it no longer misleads callers. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
baijumeswani
approved these changes
Jun 9, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is split out of #1416 to keep the review surface small and focused.
Changes:
Validation performed locally:
git diff --check