Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Invalid HTTP method crash expr_eval #231

Open
geNAZt opened this issue May 2, 2024 · 0 comments
Open

Invalid HTTP method crash expr_eval #231

geNAZt opened this issue May 2, 2024 · 0 comments

Comments

@geNAZt
Copy link

geNAZt commented May 2, 2024

Hi,

we are using this SDK for attribute generation via dynamic expressions. We use expr_create to get a expression identifier via call_foreign_function("expr_create", Option::from(self.condition.as_bytes()));

We cache this id and reuse it for expr_eval when HttpContext on_log gets called.

The bug we now see is that expr_eval called via call_foreign_function with a non standard HTTP method crashes the expr_eval with InternalFailure (10) and thus panic:

    | 2024-05-02 11:27:22.378 | {"clientDuration":0,"protocol":"HTTP/1.1","upstreamName":null,"targetDuration":null,"path":null,"clientIPs":null,"upstreamHost":null,"responseCode":400,"method":null,"traceId":null} |  
    | 2024-05-02 11:27:21.425 | 8:  0x21ab0 - proxy_on_log	thread=30 |  
    | 2024-05-02 11:27:21.425 | 7:  0x15cd0 - _$LT$attributegen..attribute_generator..AttributeGenerator$u20$as$u20$proxy_wasm..traits..HttpContext$GT$::on_log::h8e4584a43625dd44 |  
    | 2024-05-02 11:27:21.425 | 6:  0x4f7f - attributegen::AttributeGeneratorRoot::generate::had45e525e7ba4c35 |  
    | 2024-05-02 11:27:21.425 | 5:  0xc79 - core::panicking::panic_fmt::h3aff855fe938c13f |  
    | 2024-05-02 11:27:21.425 | 4:  0x19795 - rust_begin_unwind |  
    | 2024-05-02 11:27:21.425 | 3:  0x2f49d - std::sys_common::backtrace::__rust_end_short_backtrace::ha76513a70bb070b0 |  
    | 2024-05-02 11:27:21.425 | 2:  0x2f572 - std::panicking::begin_panic_handler::_$u7b$$u7b$closure$u7d$$u7d$::h96d2bc381fa6ee1e |  
    | 2024-05-02 11:27:21.425 | 1:  0x2ce60 - std::panicking::rust_panic_with_hook::h9aabd906218897c3 |  
    | 2024-05-02 11:27:21.425 | 0:  0x2f360 - rust_panic |  
    | 2024-05-02 11:27:21.425 | Proxy-Wasm plugin in-VM backtrace: |  
    | 2024-05-02 11:27:21.425 | 2024-05-02T09:27:21.425393Z	error	envoy wasm external/envoy/source/extensions/common/wasm/wasm_vm.cc:38	Function: proxy_on_log failed: Uncaught RuntimeError: unreachable |  
    | 2024-05-02 11:27:21.425 | unexpected status: 10	thread=30 |  
    | 2024-05-02 11:27:21.425 | 2024-05-02T09:27:21.425075Z	critical	envoy wasm external/envoy/source/extensions/common/wasm/context.cc:1157	wasm log fort-knox.metric-attributes-api-gateway: panicked at /home/runner/.cargo/registry/src/index.crates.io-6f17d22bba15001f/proxy-wasm-0.2.1/src/hostcalls.rs:1031:23:

The curl call to trigger this:

curl --location --request MBGR 'https://<URL redacted>/'

The expression in question looks something like this:

request.url_path.startsWith('/some/sub/url/')

We use istio 1.19.7 in this setup and it seems that when the plugin panics it doesn't take down the envoy proxy. It just kills the listener so we end up with a practically half dead proxy which doesn't recover until we manually restart it.

I would like to see that call_foreign_function allows the developer to decide what states are ok and which are not. Currently it just panics when it thinks the outcome is not ok.

Edit:

More debugging found that in those cases url_path is not part of the request object (even though envoy docs say it should be present). So modifying the expression to has(request.url_path) && request.url_path.startsWith('/some/sub/url/') is a working workaround for this panic/crash

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant