Blocking from a hook is not stopping code execution #2836
Conversation
estringana
changed the title
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2836 +/- ##
=============================================
- Coverage 72.75% 51.21% -21.54%
+ Complexity 2750 2745 -5
=============================================
Files 138 111 -27
Lines 15038 10895 -4143
Branches 1020 0 -1020
=============================================
- Hits 10941 5580 -5361
- Misses 3543 5315 +1772
+ Partials 554 0 -554
Flags with carried forward coverage won't be shown. Click here to find out more. see 60 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
|
I see, the tracer sandboxing is sandboxing the bailout away :-) |
estringana
force-pushed
the
estringana/blocking-within-tracer-hook
branch
from
c5d1d67 to
1d93a16
Compare
estringana
force-pushed
the
estringana/blocking-within-tracer-hook
branch
from
1d93a16 to
78a05a9
Compare
estringana
force-pushed
the
estringana/blocking-within-tracer-hook
branch
2 times, most recently
from
dedee17 to
3438fca
Compare
Benchmarks [ tracer ]Benchmark execution time: 2024-12-16 15:43:32 Comparing candidate commit 7fd4a50 in PR branch Found 3 performance improvements and 3 performance regressions! Performance is the same for 172 metrics, 0 unstable metrics. scenario:EmptyFileBench/benchEmptyFileBaseline
scenario:MessagePackSerializationBench/benchMessagePackSerialization
scenario:PDOBench/benchPDOBaseline
scenario:PDOBench/benchPDOBaseline-opcache
scenario:SamplingRuleMatchingBench/benchRegexMatching4-opcache
scenario:TraceFlushBench/benchFlushTrace
|
estringana
force-pushed
the
estringana/blocking-within-tracer-hook
branch
2 times, most recently
from
8322005 to
1602d2c
Compare
| inline void zai_sandbox_bailout(zai_sandbox *sandbox) { | ||
| if (!zai_sandbox_timed_out()) { | ||
| if (!zai_sandbox_timed_out() && !zai_is_request_blocked()) { |
There was a problem hiding this comment.
I don't know why sandbox.h is written this way, with just C inline functions, but I guess that in the context of its strangeness it's fine.
estringana
force-pushed
the
estringana/blocking-within-tracer-hook
branch
from
1602d2c to
ee5bff3
Compare
| @@ -356,8 +375,25 @@ inline bool zai_sandbox_timed_out(void) { | |||
| return false; | |||
| } | |||
|
|
|||
| inline bool zai_is_request_blocked(void) | |||
There was a problem hiding this comment.
Both implementations of this function are exactly the same, perhaps you can define it only once?
There was a problem hiding this comment.
They are the same yes. However, this file is full of this duplication. I did it that way so follow the same approach
There was a problem hiding this comment.
Unless there's a good technical reason, don't duplicate the function.
| return false; | ||
| } | ||
|
|
||
| if (strcmp("Datadog blocked the request and presented a static error page", ZSTR_VAL(PG(last_error_message))) == 0) { |
There was a problem hiding this comment.
My main concern with this approach is that if someone changes the error message in appsec, it would stop working. Can you make changes on the appsec side to:
- Make sure the error messages are validated at compile time?
- Add comments specifying that the error message must not be changed.
| return true; | ||
| } | ||
|
|
||
| if (strcmp("Datadog blocked the request, but the response has already been partially committed", ZSTR_VAL(PG(last_error_message))) == 0) { |
There was a problem hiding this comment.
Alternatively you could check the prefix, which is always Datadog blocked the request
| @@ -218,8 +220,25 @@ inline bool zai_sandbox_timed_out(void) { | |||
| return false; | |||
| } | |||
|
|
|||
| inline bool zai_is_request_blocked(void) | |||
There was a problem hiding this comment.
This is missing the redirection message as well.
estringana
force-pushed
the
estringana/blocking-within-tracer-hook
branch
from
ee5bff3 to
7fd4a50
Compare
Description
Blocking a request from Appsec should stop customer code execution. However, when this blocking happens within a tracer hook, it does not stop executing customer code execution.
Reviewer checklist