diff --git a/doc/developer-guide/api/index.en.rst b/doc/developer-guide/api/index.en.rst index df0ffb32d19..5b8abbff575 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 ``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); +}