envoy icon indicating copy to clipboard operation
envoy copied to clipboard

Harden stream lifetime

Open danzh2010 opened this issue 1 year ago • 4 comments

Commit Message: made 2 interface changes to harden the life time interaction between codec server streams and HCM or between codec client streams and UpstreamRequest.

  1. Added StreamCallbacksRegistry interface to cooperate with Http::StreamCallbacks to cross reference each other so that when StreamCallbacks objects go out of life time, it will unregister itself from the registry implementation StreamCallbacksHelper and vice versa.

  2. Added a new ObjectHandle interface to wrap request/response encoder and decoder objects and a new function createHandle() to Request/Response Encoder and Decoder interfaces to return a handle to themselves. When these encoder and decoders get destroyed, they will clear the reference in the handle object so that the owners of those handles can no longer access encoder and decoder objects. This is pretty much similar to weak_ptr, but the handle interface also provide the ability to cache the destruction state of the encoder or decoder for debugging purpose. The existing StreamCallbacks interface together with conditional runResetCallbacks() should have already guaranteed that the decoder and encoder objects shouldn't be accessed after the end of the stream life time. But such contract and the condition can easily get wrong in both side and missing callbacks indeed happens in production. The destruction state in the handle could be used to debug how the callbacks are missed. And this PR also changed the code to switch to hold an ObjectHandle instead of the raw pointer at a few places, i.e. HCM and HTTP/3 and HTTP/2 codec.

Additional Description: this PR doesn't make change to HTTP/1 codec. And it only added destruction state to HTTP/3 codec.

Risk Level: Testing: Docs Changes: Release Notes: Platform Specific Features: [Optional Runtime guard:] [Optional Fixes #Issue] [Optional Fixes commit #PR or SHA] [Optional Deprecated:] [Optional API Considerations:]

danzh2010 avatar Mar 25 '24 19:03 danzh2010

As a reminder, PRs marked as draft will not be automatically assigned reviewers, or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

:cat:

Caused by: https://github.com/envoyproxy/envoy/pull/33102 was opened by danzh2010.

see: more, trace.

@alyssawilk @RyanTheOptimist I'm planning to propose these interface changes to harden the stream lifetime interaction between codec and HCM. It added some implementation code into the encoder/decoder interfaces unfortunately, but that's where I found most appropriate. Let me know what you think.

danzh2010 avatar Mar 25 '24 20:03 danzh2010

I'm also debating if I should runtime guard this change, given the whole thing is to protect UAF.

danzh2010 avatar Mar 25 '24 20:03 danzh2010

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

github-actions[bot] avatar May 11 '24 16:05 github-actions[bot]

This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

github-actions[bot] avatar May 18 '24 20:05 github-actions[bot]

Still working on it.

danzh2010 avatar May 20 '24 14:05 danzh2010