wasm: Fix potential segfault when reading filter_state#17880
wasm: Fix potential segfault when reading filter_state#17880ggreenway merged 1 commit intoenvoyproxy:mainfrom
Conversation
Other properties check for non-null `info` before attempting to read since it can be null. Signed-off-by: LaVigne, Scott <lavignes@amazon.com>
lavignes
force-pushed
the
fix-wasm-segfault
branch
from
5e7fdbd to
6e25ff0
Compare
kyessenov
left a comment
kyessenov
left a comment
There was a problem hiding this comment.
Thanks, it would be good to have a test for the crash. I assume it's calling out during configuration or creationg.
PiotrSikora
left a comment
PiotrSikora
left a comment
There was a problem hiding this comment.
Thanks, code LGTM, but like @kyessenov said, a test case would be appreciated.
Take a look at test/extensions/filters/network/wasm/test_data/close_stream_rust.rs and how it's used in tests - you can do pretty much the same with your reproducer.
|
Of course. Let me add a test. I wasn't sure where this was tested but after @kyessenov called this out, I figured it out. I'll update this :) |
|
kindly ping @lavignes, are you still around to add tests here? |
|
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
added
the
stale
PiotrSikora
left a comment
PiotrSikora
left a comment
There was a problem hiding this comment.
Let's get this in (since it's a real issue) and we can add tests in a follow-up PR.
|
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! |
|
@envoyproxy/senior-maintainers could someone reopen this and merge it? Thanks! |
github-actions
bot
removed
the
stale
ggreenway
left a comment
ggreenway
left a comment
There was a problem hiding this comment.
Approving per @PiotrSikora 's approval as extension owner.
6 participants
Signed-off-by: Scott LaVigne lavignes@amazon.com
Commit Message: Fix potential segfault when reading filter_state
Additional Description: Adding a needed null-check when reading the
filter_stateproperty from a WASM filter.Risk Level: low
Testing: I have a repro for the crash and tested the fix via: https://github.com/lavignes/proxy-wasm-crash-example
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: n/a
I was experimenting with a WASM network filter and ran into a case where my WASM plugin was crashing Envoy. Here's a minimal repro: https://github.com/lavignes/proxy-wasm-crash-example. I also got confirmation by @PiotrSikora that it would be safe to just open a PR publicly.