[Bug] Wasm plugin crashed when calling get_http_request_headers()

[ytsssun]

We use proxy-wasm-rust-sdk for our stats plugin. Recently we received a bug report from our customer. By looking into the backtrace, we found that the crash happened within the hostcalls.rs in the SDK.

Error log:

[2024-01-03 23:46:27.247][92][critical][wasm] [source/extensions/common/wasm/context.cc:1157] wasm log aws.appmesh.ingress_http_stats: panicked at /codebuild/output/src3353/src/s3/00/wasm/cargo-project/vendor/proxy-wasm/src/hostcalls.rs:1192:42:
called `Result::unwrap()` on an `Err` value: FromUtf8Error { bytes: [49, 32, 192, 167, 192, 162, 37, 50, 53, 50, 55, 37, 50, 53, 50, 50, 44, 32, 49, 53, 52, 46, 51, 56, 46, 49, 55, 50, 46, 50, 52, 51, 44, 32, 49, 53, 46, 49, 53, 56, 46, 52, 55, 46, 49, 52, 49], error: Utf8Error { valid_up_to: 2, error_len: Some(1) } }
[2024-01-03 23:46:27.247][92][error][wasm] [source/extensions/common/wasm/wasm_vm.cc:38] Function: proxy_on_request_headers failed: Uncaught RuntimeError: unreachable
Proxy-Wasm plugin in-VM backtrace:
0: 0x385ad - rust_panic
1: 0x37842 - std::panicking::rust_panic_with_hook::h3aa054d35a0817d7
2: 0x38a56 - std::panicking::begin_panic_handler::_$u7b$$u7b$closure$u7d$$u7d$::h2f73e4cf6cd6319a
3: 0x3898f - std::sys_common::backtrace::__rust_end_short_backtrace::h98ac61a6abbff7e9
4: 0x24033 - rust_begin_unwind
5: 0x24d1 - core::panicking::panic_fmt::h3e1dd3d08288569e
6: 0x425f - core::result::unwrap_failed::h8b3db0f11171b57b
7: 0x103f3 - proxy_wasm::hostcalls::get_map::hfc84c18de12c4fd4
8: 0xff0e - _$LT$amzn_appmesh_wasm..reader..host..Host$u20$as$u20$amzn_appmesh_wasm..reader..HttpMapReader$GT$::get_request_headers::h60ed0437583308a0
9: 0xd219 - _$LT$amzn_appmesh_wasm..plugin..context..http..HttpFilterContext$u20$as$u20$proxy_wasm..traits..HttpContext$GT$::on_http_request_headers::h706d8f144b9ef225

[93][critical][wasm] [source/extensions/common/wasm/context.cc:1157] wasm log aws.appmesh.egress_http_stats: panicked at /codebuild/output/src3353/src/s3/00/wasm/cargo-project/vendor/proxy-wasm/src/hostcalls.rs:1192:42:

This easily reproducible if I ran following code, see the playground:

#![allow(unused)]
fn main() {
  use std::str;

  // some bytes, in a vector
  let problematic_bytes = vec! [49, 32, 192, 167, 192, 162, 37, 50, 53, 50, 55, 37, 50, 53, 50, 50, 44, 32, 49, 53, 52, 46, 51, 56, 46, 49, 55, 50, 46, 50, 52, 51, 44, 32, 49, 53, 46, 49, 53, 56, 46, 52, 55, 46, 49, 52, 49];

  let result = str::from_utf8(&problematic_bytes).unwrap();
}

Looks like the byte is not following UTF-8 encoding rules, are there any breaking change introduced from Envoy side that would make the bytes invalid UTF-8 encoding?

Would need some help troubleshooting this thanks!

[PiotrSikora]

The issue is that this SDK originally defined get_http_call_response_headers() as returning Vec<(String, String)>, and String is always UTF-8 in Rust, but HTTP header values are not.

You should be using get_http_call_response_headers_bytes() that returns Vec<(String, Bytes)> instead and process Bytes inside the plugin.

[ytsssun]

Thanks for your response. This makes a lot sense! I will use get_http_call_response_headers_bytes() as per your suggestion. Meanwhile, can we deprecate get_http_call_response_headers() given that this may be a known bug?

We also are using these APIs:

get_http_request_trailers()
get_http_response_headers()
get_http_response_trailers()

I assume we should be using the "byte" version of them as well?

[pi-pi-miao]

建议使用 match 返回 result来替代.unwrap();可以有效防止崩溃

[pi-pi-miao]

you can use https://github.com/pi-pi-miao/proxy-wasm-rust-sdk solve the problem

[darshan95]

Hi, I'd like to take a shot at fixing this.

The panic is at src/hostcalls.rs#L1322-L1323, where utils::deserialize_map calls String::from_utf8(...).unwrap() on both the header key and value. Envoy is free to pass through header values as opaque octets (RFC 7230 obs-text allows %x80-FF), so any such byte sequence — like the 0xC0 0xA7 0xC0 0xA2 shown in the panic log above — aborts the Wasm VM.

Proposed fix: switch the value decode to String::from_utf8_lossy(&value).into_owned() so invalid bytes become U+FFFD instead of panicking. Keys would keep the strict decode (they're overwhelmingly ASCII in practice), unless you'd prefer lossy there too.

One thing worth calling out: test_deserialize_map_all_chars currently asserts the panic for bytes 0x80..0xFF, so it would need to be updated to assert lossy decoding, and I'd add a regression test using the exact byte sequence from the panic log above.

Alternative: change get_map's return type to propagate the UTF-8 error via Result instead of decoding lossily. That's more invasive but preserves strictness for callers who want it.

Happy to go either direction — which do you prefer? I'll sign the CLA and open a PR once you confirm the approach.

[PiotrSikora]

@darshan95 neither of the proposed solutions are strictly correct, since HTTP header values are not UTF-8 in the first place. Please check #287 for a solution that's going to be merged in the next few days. Let me know if that fixes the issue for you!

Note that you can use String::from_utf8_lossy() with _bytes() variants, as mentioned earlier in this issue.

[darshan95]

Thanks for the pointer — that makes sense. HeaderValue is a cleaner fit than either of my proposals since it preserves the raw header bytes instead of lying about them being UTF-8 (lossy would silently corrupt, and Result<String, _> would still assume headers should be strings).

I'll switch our plugin to the _bytes() variants with String::from_utf8_lossy() in the meantime, and move to the header-value feature once #287 lands.