[dev/Wasm] Removing unsupported try catch

[M4tteoP]

Context

Several basic elements from the modsecurity.conf file (e.g. rules 200002, 200003, 200004) are leading to a RuntimeError anticipated by a stoi: no conversion error. As far as I know, the proxy Wasm SDK does not still support the handling of exceptions, therefore, the stoi conversion based on catching an exception when the conversion fails leads to this behaviour.

Solution

The PR proposes to handle the conversion based on the more recent std::from_chars that handles without exceptions the outcome.

Work in progress, request for tips and discussion

Other try catch patterns are still in place inside the code and they may lead to similar errors. Specifically:

1. Would it be possible to have some guidance or pointers on how to tweak .cc and .yy files? E.g. seclang-parser.cc#L2907-L2915
2. I think that a very sensitive point is the allocation of the request body: transaction.cc#L1019-L1025. Do you have any tips on how would it possible to avoid the usage of exceptions?

Thanks!

@martinhsv @leyao-daily

[M4tteoP]

Update: about stod conversions, as you may see looking at the commits, I tried both:

• std::from_chars: It should be faster, but requires a recent compiler to handle double variables.
• std::stringstream: It should be slower, better compatibility.

As far as I understood, these conversions happen just at loading time, the overhead of ss compared to from_chars should not be so relevant in favor of fewer compiling problems. I also saw that stringstream is currently used in the ModSecurity codebase.

Open to any discussion also about it.

[mathetake]

fyi, the exception support might work with the latest envoy, see the discussion https://github.com/proxy-wasm/proxy-wasm-cpp-sdk/issues/140

[leyao-daily]

fyi, the exception support might work with the latest envoy, see the discussion proxy-wasm/proxy-wasm-cpp-sdk#140

It helps a lot. Thanks.

[M4tteoP]

Thank you, Takeshi! I have followed the conversation for a bit, as of now I have not been able to make it work, but it sounds really promising. I tweaked all the code where I saw a feasible alternative, but (for example about memory allocation) I feel that exception support is very much needed to prevent unexpected behaviors.

[martinhsv]

Hello @M4tteoP ,

I'm open to revisiting some of these try/catch usages. Many of the existing usages aren't the highest-value usages of try/catch anyway.

On the other hand, if we're saying we would want to never implement additional try-catch blocks in ModSecurity in the future, that might be seen as a nontrivial limitation on development.

Another thing to consider is what the replacement code is. So far ModSecurity has not generally incorporated C++17 features. It's useful to maintain compatibility with older compiler versions (within reason), so we'd have to decide if this is the right time for that.

[M4tteoP]

Hello @martinhsv,

Another thing to consider is what the replacement code is. So far ModSecurity has not generally incorporated C++17 features. It's useful to maintain compatibility with older compiler versions (within reason), so we'd have to decide if this is the right time for that.

Thank you, if we agree on a proper replacement code that permits maintaining compatibility with older compile versions, I'm open to working on a PR on the main branch. Specifically, do you wish to avoid the usage of std::from_chars? Could std::stringstream be a valid replacement also for integer conversions?

if we're saying we would want to never implement additional try-catch blocks in ModSecurity in the future, that might be seen as a nontrivial limitation on development.

Yes, I totally understand that, it would be awesome just to properly revisit not needed usages and, strictly speaking about Wasm:

• Restrict only to the dev/Wasm branch more drastic PRs to permit Wasm to work even if some tradeoff in terms of functionalities may be introduced.
• Work on supporting exceptions handling (following what Takeshi pointed out).

[dspeg]

When testing ModSecurity Wasm plugin, we encountered unexpected "500" or "503" error codes with some simple rules added. I hope some of the issues can be fixed by this PR. Thanks @M4tteoP for working on it.