Fix segfault in AyonApi::serialCorePost on failed requests - #53
Open
isohedronpipeline wants to merge 5 commits into
Open
Fix segfault in AyonApi::serialCorePost on failed requests#53isohedronpipeline wants to merge 5 commits into
AyonApi::serialCorePost on failed requests#53isohedronpipeline wants to merge 5 commits into
Conversation
…onflict Release/resolve develop main conflict
Release PR
Release 0.1.3 prep
Release prep
serialCorePost dereferences the httplib::Result from Post() without checking whether the request actually succeeded. cpp-httplib returns a null Result rather than throwing when a request fails to connect or complete, so any failed POST segfaults on a null-pointer read instead of failing gracefully like the sibling GET path does. Fixes ynput#52
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.
Fix segfault in
AyonApi::serialCorePoston failed requestsFixes #52
Summary
AyonApi::serialCorePostdereferences thehttplib::Resultreturned bym_ayonServer->Post(...)without checking whether the request actuallysucceeded:
cpp-httplib does not throw a C++ exception when a request fails to
connect or complete (connection refused, TLS failure, timeout, etc.) - it
returns a null/empty
Result. The existingcatch (const httplib::Error&)block a few lines down never fires for this case, so any failed POST
segfaults immediately on a null-pointer dereference.
The sibling
GET-based code path (used bygetSiteRoots()) alreadyguards against exactly this with an explicit null check and a clean
"response is null"log line.serialCorePostis missing theequivalent guard.
Impact
Reproduced as a 100%-reproducible Houdini crash: any transient
connectivity issue between the client and the AYON server during a
POST-based
resolvePath()call hard-crashes the host DCC (confirmed inHoudini 21) instead of failing gracefully like the
GETpath does. Sincethe failure only needs to happen once during any
serialCorePostcall,this affects any studio hitting a network blip, VPN hiccup, server
restart, or misconfiguration during a resolve - not something specific to
one environment.
What this PR changes
1. Null-check before dereferencing the response (the core fix):
2. Smarter handling specifically for
httplib::Error::SSLServerVerificationWhile diagnosing this, we found
cpp-httplib'sSSLClient::load_certs()has its own latent issue: it wraps CA-cert loading in a
std::call_once,but the
bool retit returns is a fresh local variable on every call - soif certificate loading genuinely fails once, that failure is silently
reported as success on every subsequent call for that client object's
entire lifetime (since the
call_oncebody never runs again). Combinedwith
AyonApiholding one long-lived, keep-alivehttplib::Clientforthe whole session, a single bad first attempt can permanently poison
verification for that object.
Rather than patch vendored
cpp-httplib, we detect this specific errorand validate whether retrying is actually worthwhile before doing so:
This avoids two failure modes: wasting the full retry budget when the
cert configuration is genuinely broken (fails fast with one clear log
line instead), and getting permanently stuck retrying against an already
provably-poisoned client object when the cert config is actually fine (a
fresh client gets a genuine second chance).
3. New
m_caCertPathmemberNeeded to support the rebuild above - the winning cert path (from
whichever of the constructor/
setSSL()branches determined it) is nowrecorded on the instance instead of being used once and discarded.
4. CMakeLists.txt default fix
option(USE_OPENSSL3 "Build against OpenSSL 3.x" OFF)- this vendoredcpp-httplibhard-errors at compile time on any OpenSSL version below3.0.0, so
OFFcannot currently produce a working build under anyconfiguration. Flipped the default to
ONto match reality; anyonebuilding this repo standalone (via
AyonBuild.py, not throughayon-usd-resolver'sbuild_resolver.py, which already passes-DUSE_OPENSSL3=ONexplicitly) previously hit a confusing compile errorwith no indication the fix was a one-line CMake flag.
Testing
AyonBuild.py --runStageGRP CleanBuild- compilescleanly with all changes.
ayon-usd-resolverplugin against Houdini 21 with thispatched
ayon-cpp-apias its submodule and verified in a live Houdinisession: the previously 100%-reproducible crash no longer occurs, and a
deliberately-triggered
SSLServerVerificationfailure now retriescleanly instead of crashing.
genuinely invalid cert path (fails fast) from a valid one (retries)
using targeted manual tests against both cases.