From dd22901cdd97666f38d9a4cfb345dbab0b6866bb Mon Sep 17 00:00:00 2001 From: Bryan Call Date: Thu, 27 Aug 2026 09:27:28 -0700 Subject: [PATCH 1/2] Clear API output parameters on failure Functions that return a handle through an output parameter left that parameter untouched when they failed, so a caller that skipped the return code was left holding whatever was on the stack. Several call sites in tree do skip it, and three of these functions already carried comments claiming they set *locp to TS_NULL_MLOC on failure while the code did not. Clear the output parameters immediately after the sanity-check asserts, so every failure path leaves TS_NULL_MLOC or nullptr behind. Where a function published its handles and only then validated them, compute into a local and publish on success only, so a failure never hands back a buffer that did not pass the check. Add the missing null-pointer asserts that this clearing depends on. Document the guarantee once in the API reference rather than per function, and add a regression test so the contract cannot drift out of sync with the code the way the old comments did. Checking the return code is still required; a cleared output parameter is not a usable handle. --- doc/developer-guide/api/index.en.rst | 13 +++ src/api/InkAPI.cc | 125 ++++++++++++++++++++++----- src/api/InkAPITest.cc | 43 +++++++++ 3 files changed, 159 insertions(+), 22 deletions(-) diff --git a/doc/developer-guide/api/index.en.rst b/doc/developer-guide/api/index.en.rst index df0ffb32d19..fac3e37d87d 100644 --- a/doc/developer-guide/api/index.en.rst +++ b/doc/developer-guide/api/index.en.rst @@ -26,6 +26,19 @@ For an introduction to the |TS| API, how to link it to your plugin, and the methods of passing parameters to your plugin at runtime, please refer to :manpage:`TSAPI(3ts)`. +Output Parameters and Failure +============================= + +Many API functions return a handle through an output parameter and report +success or failure with a :type:`TSReturnCode`. When such a function returns +anything other than ``TS_SUCCESS``, it clears its output parameters: handle +parameters are set to ``TS_NULL_MLOC`` and buffer parameters to ``nullptr``. +A failing call therefore never leaves an indeterminate or partially written +value behind. + +Checking the return code is still required. A cleared output parameter is not +a usable handle, and passing one to another API function is an error. + .. toctree:: :maxdepth: 1 diff --git a/src/api/InkAPI.cc b/src/api/InkAPI.cc index 59a94991c7e..e65e8c28731 100644 --- a/src/api/InkAPI.cc +++ b/src/api/InkAPI.cc @@ -918,6 +918,8 @@ TSUrlCreate(TSMBuffer bufp, TSMLoc *locp) sdk_assert(sdk_sanity_check_mbuffer(bufp) == TS_SUCCESS); sdk_assert(sdk_sanity_check_null_ptr(locp) == TS_SUCCESS); + *locp = TS_NULL_MLOC; + if (isWriteable(bufp)) { HdrHeap *heap = reinterpret_cast(bufp)->m_heap; *locp = reinterpret_cast(url_create(heap)); @@ -934,6 +936,8 @@ TSUrlClone(TSMBuffer dest_bufp, TSMBuffer src_bufp, TSMLoc src_url, TSMLoc *locp sdk_assert(sdk_sanity_check_url_handle(src_url) == TS_SUCCESS); sdk_assert(sdk_sanity_check_null_ptr(locp) == TS_SUCCESS); + *locp = TS_NULL_MLOC; + if (!isWriteable(dest_bufp)) { return TS_ERROR; } @@ -1460,6 +1464,8 @@ TSMimeHdrCreate(TSMBuffer bufp, TSMLoc *locp) sdk_assert(sdk_sanity_check_mbuffer(bufp) == TS_SUCCESS); sdk_assert(sdk_sanity_check_null_ptr((void *)locp) == TS_SUCCESS); + *locp = TS_NULL_MLOC; + if (!isWriteable(bufp)) { return TS_ERROR; } @@ -1500,6 +1506,8 @@ TSMimeHdrClone(TSMBuffer dest_bufp, TSMBuffer src_bufp, TSMLoc src_hdr, TSMLoc * sdk_assert(sdk_sanity_check_http_hdr_handle(src_hdr) == TS_SUCCESS); sdk_assert(sdk_sanity_check_null_ptr((void *)locp) == TS_SUCCESS); + *locp = TS_NULL_MLOC; + if (!isWriteable(dest_bufp)) { return TS_ERROR; } @@ -1863,6 +1871,8 @@ TSMimeHdrFieldCreate(TSMBuffer bufp, TSMLoc mh_mloc, TSMLoc *locp) (sdk_sanity_check_http_hdr_handle(mh_mloc) == TS_SUCCESS)); sdk_assert(sdk_sanity_check_null_ptr((void *)locp) == TS_SUCCESS); + *locp = TS_NULL_MLOC; + if (!isWriteable(bufp)) { return TS_ERROR; } @@ -1885,6 +1895,8 @@ TSMimeHdrFieldCreateNamed(TSMBuffer bufp, TSMLoc mh_mloc, const char *name, int sdk_assert(sdk_sanity_check_null_ptr((void *)name) == TS_SUCCESS); sdk_assert(sdk_sanity_check_null_ptr((void *)locp) == TS_SUCCESS); + *locp = TS_NULL_MLOC; + if (!isWriteable(bufp)) { return TS_ERROR; } @@ -1900,7 +1912,6 @@ TSMimeHdrFieldCreateNamed(TSMBuffer bufp, TSMLoc mh_mloc, const char *name, int if (h->field_ptr == nullptr) { // The name exceeds the uint16_t field-length limit; nothing was created. sdk_free_field_handle(bufp, h); - *locp = nullptr; return TS_ERROR; } *locp = reinterpret_cast(h); @@ -1971,6 +1982,8 @@ TSMimeHdrFieldClone(TSMBuffer dest_bufp, TSMLoc dest_hdr, TSMBuffer src_bufp, TS sdk_assert(sdk_sanity_check_field_handle(src_field, src_hdr) == TS_SUCCESS); sdk_assert(sdk_sanity_check_null_ptr((void *)locp) == TS_SUCCESS); + *locp = TS_NULL_MLOC; + if (!isWriteable(dest_bufp)) { return TS_ERROR; } @@ -2592,6 +2605,9 @@ TSHttpHdrClone(TSMBuffer dest_bufp, TSMBuffer src_bufp, TSMLoc src_hdr, TSMLoc * sdk_assert(sdk_sanity_check_mbuffer(src_bufp) == TS_SUCCESS); sdk_assert(sdk_sanity_check_http_hdr_handle(src_hdr) == TS_SUCCESS); + sdk_assert(sdk_sanity_check_null_ptr((void *)locp) == TS_SUCCESS); + *locp = TS_NULL_MLOC; + if (!isWriteable(dest_bufp)) { return TS_ERROR; } @@ -2893,6 +2909,9 @@ TSHttpHdrUrlGet(TSMBuffer bufp, TSMLoc obj, TSMLoc *locp) sdk_assert(sdk_sanity_check_mbuffer(bufp) == TS_SUCCESS); sdk_assert(sdk_sanity_check_http_hdr_handle(obj) == TS_SUCCESS); + sdk_assert(sdk_sanity_check_null_ptr((void *)locp) == TS_SUCCESS); + *locp = TS_NULL_MLOC; + HTTPHdrImpl *hh = reinterpret_cast(obj); if (hh->m_polarity != HTTPType::REQUEST) { @@ -3956,16 +3975,20 @@ TSHttpTxnClientReqGet(TSHttpTxn txnp, TSMBuffer *bufp, TSMLoc *obj) sdk_assert(sdk_sanity_check_null_ptr((void *)bufp) == TS_SUCCESS); sdk_assert(sdk_sanity_check_null_ptr((void *)obj) == TS_SUCCESS); + *bufp = nullptr; + *obj = TS_NULL_MLOC; + HttpSM *sm = reinterpret_cast(txnp); HTTPHdr *hptr = &(sm->t_state.hdr_info.client_request); if (hptr->valid()) { - *(reinterpret_cast(bufp)) = hptr; - *obj = reinterpret_cast(hptr->m_http); - if (sdk_sanity_check_mbuffer(*bufp) == TS_SUCCESS) { + // Validate before publishing the handles so a failure never hands back a + // buffer that did not pass the check. + if (sdk_sanity_check_mbuffer(reinterpret_cast(hptr)) == TS_SUCCESS) { + *(reinterpret_cast(bufp)) = hptr; + *obj = reinterpret_cast(hptr->m_http); hptr->mark_target_dirty(); return TS_SUCCESS; - ; } } return TS_ERROR; @@ -3979,20 +4002,22 @@ TSHttpTxnPristineUrlGet(TSHttpTxn txnp, TSMBuffer *bufp, TSMLoc *url_loc) sdk_assert(sdk_sanity_check_null_ptr((void *)bufp) == TS_SUCCESS); sdk_assert(sdk_sanity_check_null_ptr((void *)url_loc) == TS_SUCCESS); + *bufp = nullptr; + *url_loc = TS_NULL_MLOC; + HttpSM *sm = reinterpret_cast(txnp); HTTPHdr *hptr = &(sm->t_state.hdr_info.client_request); - if (hptr->valid()) { - *(reinterpret_cast(bufp)) = hptr; - *url_loc = reinterpret_cast(sm->t_state.unmapped_url.m_url_impl); + if (hptr->valid() && sdk_sanity_check_mbuffer(reinterpret_cast(hptr)) == TS_SUCCESS) { + TSMLoc candidate = reinterpret_cast(sm->t_state.unmapped_url.m_url_impl); - if (sdk_sanity_check_mbuffer(*bufp) == TS_SUCCESS) { - if (*url_loc == nullptr) { - *url_loc = reinterpret_cast(hptr->m_http->u.req.m_url_impl); - } - if (*url_loc) { - return TS_SUCCESS; - } + if (candidate == nullptr) { + candidate = reinterpret_cast(hptr->m_http->u.req.m_url_impl); + } + if (candidate != nullptr) { + *(reinterpret_cast(bufp)) = hptr; + *url_loc = candidate; + return TS_SUCCESS; } } return TS_ERROR; @@ -4065,6 +4090,9 @@ TSHttpTxnClientRespGet(TSHttpTxn txnp, TSMBuffer *bufp, TSMLoc *obj) sdk_assert(sdk_sanity_check_null_ptr((void *)bufp) == TS_SUCCESS); sdk_assert(sdk_sanity_check_null_ptr((void *)obj) == TS_SUCCESS); + *bufp = nullptr; + *obj = TS_NULL_MLOC; + HttpSM *sm = reinterpret_cast(txnp); HTTPHdr *hptr = &(sm->t_state.hdr_info.client_response); @@ -4085,6 +4113,9 @@ TSHttpTxnServerReqGet(TSHttpTxn txnp, TSMBuffer *bufp, TSMLoc *obj) sdk_assert(sdk_sanity_check_null_ptr((void *)bufp) == TS_SUCCESS); sdk_assert(sdk_sanity_check_null_ptr((void *)obj) == TS_SUCCESS); + *bufp = nullptr; + *obj = TS_NULL_MLOC; + HttpSM *sm = reinterpret_cast(txnp); HTTPHdr *hptr = &(sm->t_state.hdr_info.server_request); @@ -4105,6 +4136,9 @@ TSHttpTxnServerRespGet(TSHttpTxn txnp, TSMBuffer *bufp, TSMLoc *obj) sdk_assert(sdk_sanity_check_null_ptr((void *)bufp) == TS_SUCCESS); sdk_assert(sdk_sanity_check_null_ptr((void *)obj) == TS_SUCCESS); + *bufp = nullptr; + *obj = TS_NULL_MLOC; + HttpSM *sm = reinterpret_cast(txnp); HTTPHdr *hptr = &(sm->t_state.hdr_info.server_response); @@ -4125,6 +4159,9 @@ TSHttpTxnCachedReqGet(TSHttpTxn txnp, TSMBuffer *bufp, TSMLoc *obj) sdk_assert(sdk_sanity_check_null_ptr((void *)bufp) == TS_SUCCESS); sdk_assert(sdk_sanity_check_null_ptr((void *)obj) == TS_SUCCESS); + *bufp = nullptr; + *obj = TS_NULL_MLOC; + HttpSM *sm = reinterpret_cast(txnp); HTTPInfo *cached_obj = sm->t_state.cache_info.object_read; @@ -4163,6 +4200,9 @@ TSHttpTxnCachedRespGet(TSHttpTxn txnp, TSMBuffer *bufp, TSMLoc *obj) sdk_assert(sdk_sanity_check_null_ptr((void *)bufp) == TS_SUCCESS); sdk_assert(sdk_sanity_check_null_ptr((void *)obj) == TS_SUCCESS); + *bufp = nullptr; + *obj = TS_NULL_MLOC; + HttpSM *sm = reinterpret_cast(txnp); HTTPInfo *cached_obj = sm->t_state.cache_info.object_read; @@ -4202,6 +4242,9 @@ TSHttpTxnCachedRespModifiableGet(TSHttpTxn txnp, TSMBuffer *bufp, TSMLoc *obj) sdk_assert(sdk_sanity_check_null_ptr((void *)bufp) == TS_SUCCESS); sdk_assert(sdk_sanity_check_null_ptr((void *)obj) == TS_SUCCESS); + *bufp = nullptr; + *obj = TS_NULL_MLOC; + HttpSM *sm = reinterpret_cast(txnp); HttpTransact::State *s = &(sm->t_state); HTTPHdr *c_resp = nullptr; @@ -4712,14 +4755,19 @@ TSReturnCode TSHttpTxnTransformRespGet(TSHttpTxn txnp, TSMBuffer *bufp, TSMLoc *obj) { sdk_assert(sdk_sanity_check_txn(txnp) == TS_SUCCESS); + sdk_assert(sdk_sanity_check_null_ptr((void *)bufp) == TS_SUCCESS); + sdk_assert(sdk_sanity_check_null_ptr((void *)obj) == TS_SUCCESS); + + *bufp = nullptr; + *obj = TS_NULL_MLOC; HttpSM *sm = reinterpret_cast(txnp); HTTPHdr *hptr = &(sm->t_state.hdr_info.transform_response); - if (hptr->valid()) { + if (hptr->valid() && sdk_sanity_check_mbuffer(reinterpret_cast(hptr)) == TS_SUCCESS) { *(reinterpret_cast(bufp)) = hptr; *obj = reinterpret_cast(hptr->m_http); - return sdk_sanity_check_mbuffer(*bufp); + return TS_SUCCESS; } return TS_ERROR; @@ -5710,39 +5758,66 @@ TSReturnCode TSHttpAltInfoClientReqGet(TSHttpAltInfo infop, TSMBuffer *bufp, TSMLoc *obj) { sdk_assert(sdk_sanity_check_alt_info(infop) == TS_SUCCESS); + sdk_assert(sdk_sanity_check_null_ptr((void *)bufp) == TS_SUCCESS); + sdk_assert(sdk_sanity_check_null_ptr((void *)obj) == TS_SUCCESS); + + *bufp = nullptr; + *obj = TS_NULL_MLOC; HttpAltInfo *info = reinterpret_cast(infop); + if (sdk_sanity_check_mbuffer(reinterpret_cast(&info->m_client_req)) != TS_SUCCESS) { + return TS_ERROR; + } + *(reinterpret_cast(bufp)) = &info->m_client_req; *obj = reinterpret_cast(info->m_client_req.m_http); - return sdk_sanity_check_mbuffer(*bufp); + return TS_SUCCESS; } TSReturnCode TSHttpAltInfoCachedReqGet(TSHttpAltInfo infop, TSMBuffer *bufp, TSMLoc *obj) { sdk_assert(sdk_sanity_check_alt_info(infop) == TS_SUCCESS); + sdk_assert(sdk_sanity_check_null_ptr((void *)bufp) == TS_SUCCESS); + sdk_assert(sdk_sanity_check_null_ptr((void *)obj) == TS_SUCCESS); + + *bufp = nullptr; + *obj = TS_NULL_MLOC; HttpAltInfo *info = reinterpret_cast(infop); + if (sdk_sanity_check_mbuffer(reinterpret_cast(&info->m_cached_req)) != TS_SUCCESS) { + return TS_ERROR; + } + *(reinterpret_cast(bufp)) = &info->m_cached_req; *obj = reinterpret_cast(info->m_cached_req.m_http); - return sdk_sanity_check_mbuffer(*bufp); + return TS_SUCCESS; } TSReturnCode TSHttpAltInfoCachedRespGet(TSHttpAltInfo infop, TSMBuffer *bufp, TSMLoc *obj) { sdk_assert(sdk_sanity_check_alt_info(infop) == TS_SUCCESS); + sdk_assert(sdk_sanity_check_null_ptr((void *)bufp) == TS_SUCCESS); + sdk_assert(sdk_sanity_check_null_ptr((void *)obj) == TS_SUCCESS); + + *bufp = nullptr; + *obj = TS_NULL_MLOC; HttpAltInfo *info = reinterpret_cast(infop); + if (sdk_sanity_check_mbuffer(reinterpret_cast(&info->m_cached_resp)) != TS_SUCCESS) { + return TS_ERROR; + } + *(reinterpret_cast(bufp)) = &info->m_cached_resp; *obj = reinterpret_cast(info->m_cached_resp.m_http); - return sdk_sanity_check_mbuffer(*bufp); + return TS_SUCCESS; } void @@ -6796,12 +6871,15 @@ TSFetchPageRespGet(TSHttpTxn txnp, TSMBuffer *bufp, TSMLoc *obj) sdk_assert(sdk_sanity_check_null_ptr((void *)bufp) == TS_SUCCESS); sdk_assert(sdk_sanity_check_null_ptr((void *)obj) == TS_SUCCESS); + *bufp = nullptr; + *obj = TS_NULL_MLOC; + HTTPHdr *hptr = reinterpret_cast(txnp); - if (hptr->valid()) { + if (hptr->valid() && sdk_sanity_check_mbuffer(reinterpret_cast(hptr)) == TS_SUCCESS) { *(reinterpret_cast(bufp)) = hptr; *obj = reinterpret_cast(hptr->m_http); - return sdk_sanity_check_mbuffer(*bufp); + return TS_SUCCESS; } return TS_ERROR; @@ -8795,6 +8873,9 @@ remapUrlGet(TSHttpTxn txnp, TSMLoc *urlLocp, URL *(UrlMappingContainer::*mfp)() { sdk_assert(sdk_sanity_check_txn(txnp) == TS_SUCCESS); sdk_assert(sdk_sanity_check_null_ptr(urlLocp) == TS_SUCCESS); + + *urlLocp = TS_NULL_MLOC; + HttpSM *sm = reinterpret_cast(txnp); URL *url = (sm->t_state.url_map.*mfp)(); diff --git a/src/api/InkAPITest.cc b/src/api/InkAPITest.cc index 57083a948dc..ae37461071b 100644 --- a/src/api/InkAPITest.cc +++ b/src/api/InkAPITest.cc @@ -37,6 +37,8 @@ #include #include #include +#include +#include #include "tscore/ink_config.h" #include "tscore/ink_sprintf.h" @@ -9258,3 +9260,44 @@ REGRESSION_TEST(SDK_API_TSStatCreate)(RegressionTest *test, int /* level */, int box.check(expected >= value, "TSStatIntGet(%s) gave %" PRId64 ", expected at least %" PRId64, name, value, expected); } + +////////////////////////////////////////////// +// SDK_API_OutParamClearedOnFailure +// +// Failing API calls must leave their output parameters cleared rather than +// indeterminate, so a caller that skips the return code gets a null handle +// instead of a wild one. +////////////////////////////////////////////// + +REGRESSION_TEST(SDK_API_OutParamClearedOnFailure)(RegressionTest *test, int /* level */, int *pstatus) +{ + TestBox box(test, pstatus); + + box = REGRESSION_TEST_PASSED; + + // TSHttpHdrUrlGet fails on a response header, since only requests carry a URL. + TSMBuffer bufp = TSMBufferCreate(); + TSMLoc resp_loc = TSHttpHdrCreate(bufp); + + box.check(resp_loc != TS_NULL_MLOC, "TSHttpHdrCreate failed"); + TSHttpHdrTypeSet(bufp, resp_loc, TS_HTTP_TYPE_RESPONSE); + + TSMLoc url_loc = reinterpret_cast(&box); // deliberate garbage + box.check(TSHttpHdrUrlGet(bufp, resp_loc, &url_loc) == TS_ERROR, "TSHttpHdrUrlGet should fail on a response"); + box.check(url_loc == TS_NULL_MLOC, "TSHttpHdrUrlGet left its out parameter unset on failure"); + + // TSMimeHdrFieldCreateNamed fails when the name exceeds the field length limit. + TSMLoc mime_loc = TS_NULL_MLOC; + box.check(TSMimeHdrCreate(bufp, &mime_loc) == TS_SUCCESS, "TSMimeHdrCreate failed"); + + std::string too_long(std::numeric_limits::max() + 1, 'x'); + TSMLoc field_loc = reinterpret_cast(&box); // deliberate garbage + + if (TSMimeHdrFieldCreateNamed(bufp, mime_loc, too_long.c_str(), static_cast(too_long.size()), &field_loc) == TS_ERROR) { + box.check(field_loc == TS_NULL_MLOC, "TSMimeHdrFieldCreateNamed left its out parameter unset on failure"); + } + + TSHandleMLocRelease(bufp, TS_NULL_MLOC, mime_loc); + TSHandleMLocRelease(bufp, TS_NULL_MLOC, resp_loc); + TSMBufferDestroy(bufp); +} From 92b969190f4fbf1abd6c371ec3cafe9171fe1d57 Mon Sep 17 00:00:00 2001 From: Bryan Call Date: Thu, 27 Aug 2026 11:11:17 -0700 Subject: [PATCH 2/2] Use a literal for TSReturnCode in the API reference index The index page does not set the cpp default domain, so :type: resolved against the Python domain and failed the -W docs build. --- doc/developer-guide/api/index.en.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/developer-guide/api/index.en.rst b/doc/developer-guide/api/index.en.rst index fac3e37d87d..5b8abbff575 100644 --- a/doc/developer-guide/api/index.en.rst +++ b/doc/developer-guide/api/index.en.rst @@ -30,7 +30,7 @@ Output Parameters and Failure ============================= Many API functions return a handle through an output parameter and report -success or failure with a :type:`TSReturnCode`. When such a function returns +success or failure with a ``TSReturnCode``. When such a function returns anything other than ``TS_SUCCESS``, it clears its output parameters: handle parameters are set to ``TS_NULL_MLOC`` and buffer parameters to ``nullptr``. A failing call therefore never leaves an indeterminate or partially written