bryancall opened a new pull request, #13594:
URL: https://github.com/apache/trafficserver/pull/13594
API functions that return a handle through an output parameter left that
parameter untouched when they failed. A caller that skipped the return code was
left holding whatever happened to be on the stack.
```c
TSMLoc url_loc; // uninitialized
TSHttpHdrUrlGet(hdr_url_buf, hdr_url_loc, &url_loc); // TS_ERROR: never
writes *locp
reqUrl.populate(hdr_url_buf, url_loc); // stack garbage used
as a TSMLoc
```
That is not hypothetical. In tree, 12 call sites ignore
`TSHttpTxnClientReqGet`'s return and 8 ignore `TSHttpHdrUrlGet`'s. Three of
these functions already carried comments claiming they set `*locp` to
`TS_NULL_MLOC` on failure while the code did nothing of the kind, which is how
the problem stayed invisible.
### What changed
- **Clear the output parameters on entry**, immediately after the
sanity-check asserts, in 24 functions: the `TSHttpTxn*Get` family, the three
`TSHttpAltInfo*Get` getters, `TSRemapFromUrlGet` and `TSRemapToUrlGet` (via
their shared helper), `TSHttpHdrUrlGet`, `TSHttpHdrClone`,
`TSMimeHdr{Create,Clone}`, `TSMimeHdrField{Create,CreateNamed,Clone}`,
`TSUrl{Create,Clone}`, and `TSFetchPageRespGet`. Every `TS_ERROR` path now
leaves `TS_NULL_MLOC` or `nullptr` behind.
- **Validate before publishing.** Six functions assigned both output
parameters and only then ran `sdk_sanity_check_mbuffer`, returning `TS_ERROR`
with the handles already handed out. That is worse than writing nothing,
because the caller gets handles that failed the check but look plausible. These
now compute into a local and publish only on success.
- **Add the missing null-pointer asserts** that the clearing depends on.
`TSHttpHdrClone`, `TSHttpHdrUrlGet`, and `TSHttpTxnTransformRespGet` never
asserted `sdk_sanity_check_null_ptr` on their output parameters, so clearing
without adding the assert would have converted a caught contract violation into
a segfault. The clearing is deliberately placed *after* the assert block for
this reason.
- **Document the guarantee once** in the API reference index rather than
repeating it on two dozen function pages, and state plainly that checking the
return code is still required because a cleared output parameter is not a
usable handle.
- **Add a regression test** (`SDK_API_OutParamClearedOnFailure`) that seeds
an output parameter with garbage, drives `TSHttpHdrUrlGet` and
`TSMimeHdrFieldCreateNamed` down their failure paths, and asserts the parameter
comes back `TS_NULL_MLOC`. The previous comments went stale precisely because
nothing tested them.
### Compatibility
This is a strictly widening guarantee: it constrains behavior that was
previously undefined, so no correct caller can depend on the old behavior.
There is no ABI impact, since only function bodies change. Every in-tree caller
was checked; none passes a pointer to a value it expects to be preserved across
a failed call.
### Testing
Builds clean with experimental plugins and examples enabled. All 166 unit
tests pass. The new regression test passes under `traffic_server -R`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
