[PR] Guard against a null hook list in HttpHookState::Scope::init [trafficserver]

["bryancall (via GitHub)" <gi...@apache.org>]

bryancall opened a new pull request, #13542:
URL: https://github.com/apache/trafficserver/pull/13542

   `FeatureAPIHooks::operator[]` returns `nullptr` for an out of range hook id:
   
   ```c++
   template <typename ID, int N>
   APIHooks const *
   FeatureAPIHooks<ID, N>::operator[](ID id) const
   {
     return likely(is_valid(id)) ? &(m_hooks[id]) : nullptr;
   }
   ```
   
   but `HttpHookState::Scope::init` dereferenced the result without testing it:
   
   ```c++
   _hooks = (*feature_hooks)[id];
   
   _p = nullptr;
   _c = _hooks->head();
   ```
   
   `HttpSM::state_api_callout` has a `default:` case that sets the hook id to -1 and then falls through to `HttpHookState::init`:
   
   ```c++
   default:
     cur_hook_id = static_cast<TSHttpHookID>(-1);
     ink_assert(!"not reached");
   }
   
   hook_state.init(cur_hook_id, http_global_hooks, ...);
   ```
   
   `ink_assert` compiles out in a release build, so a release binary that reaches that case does not stop. It calls `init` with an invalid id, `operator[]` hands back `nullptr`, and the dereference follows.
   
   `Scope::candidate` already tests `_hooks` for null before using it, so the rest of the class is written to tolerate an empty scope. This change makes `init` agree with `candidate`.
   
   ### Honest scope
   
   This is a latent defect found by inspection while looking at an unrelated crash. I am not claiming it explains any crash I have seen, and I do not have a reproduction: it needs the `default:` case to be reached, which is asserted to be unreachable. It is a cheap correctness fix that makes a release build fail safe where today it dereferences null, and it removes a null dereference that sits between an assertion that does not run and code that already expects null.
   
   Happy to close it if the preference is to make the unreachable case explicit some other way, for example by returning early rather than continuing with an invalid id.
   
   ### Testing
   
   Not yet built locally. Opened as a draft to let CI compile and run it first.


-- 
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: github-unsubscribe@trafficserver.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Guard against a null hook list in HttpHookState::Scope::init [trafficserver]

["bryancall (via GitHub)" <gi...@apache.org>]

bryancall merged PR #13542:
URL: https://github.com/apache/trafficserver/pull/13542


-- 
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: github-unsubscribe@trafficserver.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org