Amend security_response_id being release before displaying it #3493

estringana · 2025-11-19T10:29:30Z

Description

This PR fixes two errors related to security_response_id memory managment:

When emitting an error, shutdown handlers are called. Then security_response_id is free before the error message string is formed. When it is formed then security_response_id is pointing to garbage
When headers are already sent, security_response_id was not freed

Reviewer checklist

Test coverage seems ok.
Appropriate labels assigned.

codecov-commenter · 2025-11-19T10:40:52Z

Codecov Report

❌ Patch coverage is 74.20814% with 57 lines in your changes missing coverage. Please review.
✅ Project coverage is 61.71%. Comparing base (0c35caf) to head (1947c09).
⚠️ Report is 4 commits behind head on master.

Files with missing lines	Patch %	Lines
appsec/src/extension/request_abort.c	62.74%	30 Missing and 8 partials ⚠️
appsec/src/extension/request_abort.h	20.00%	6 Missing and 2 partials ⚠️
appsec/src/extension/request_lifecycle.c	86.88%	5 Missing and 3 partials ⚠️
appsec/src/extension/commands_helpers.c	90.00%	2 Missing and 1 partial ⚠️

❌ Your patch status has failed because the patch coverage (74.20%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3493      +/-   ##
==========================================
- Coverage   61.83%   61.71%   -0.13%     
==========================================
  Files         142      142              
  Lines       12923    12975      +52     
  Branches     1695     1700       +5     
==========================================
+ Hits         7991     8007      +16     
- Misses       4178     4210      +32     
- Partials      754      758       +4

Files with missing lines	Coverage Δ
appsec/src/extension/commands/request_exec.c	`100.00% <100.00%> (ø)`
appsec/src/extension/ddappsec.c	`75.00% <100.00%> (-2.13%)`	⬇️
appsec/src/extension/user_tracking.c	`74.91% <100.00%> (+0.68%)`	⬆️
appsec/src/extension/commands_helpers.c	`70.81% <90.00%> (+0.62%)`	⬆️
appsec/src/extension/request_abort.h	`38.46% <20.00%> (-61.54%)`	⬇️
appsec/src/extension/request_lifecycle.c	`65.14% <86.88%> (+1.53%)`	⬆️
appsec/src/extension/request_abort.c	`70.28% <62.74%> (-3.59%)`	⬇️

... and 2 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0c35caf...1947c09. Read the comment docs.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

pr-commenter · 2025-11-19T11:06:55Z

Benchmarks [ appsec ]

Benchmark execution time: 2025-11-28 17:32:42

Comparing candidate commit 1947c09 in PR branch estringana/fix-security-response-id-error-message with baseline commit 0c35caf in branch master.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 12 metrics, 0 unstable metrics.

pr-commenter · 2025-11-21T11:35:13Z

Benchmarks [ tracer ]

Benchmark execution time: 2025-11-28 18:02:10

Comparing candidate commit 1947c09 in PR branch estringana/fix-security-response-id-error-message with baseline commit 0c35caf in branch master.

Found 1 performance improvements and 6 performance regressions! Performance is the same for 185 metrics, 2 unstable metrics.

scenario:PHPRedisBench/benchRedisOverhead

🟥 execution_time [+33.178µs; +83.738µs] or [+3.955%; +9.982%]

scenario:SamplingRuleMatchingBench/benchRegexMatching1

🟥 execution_time [+78.850ns; +139.950ns] or [+6.692%; +11.878%]

scenario:SamplingRuleMatchingBench/benchRegexMatching2

🟥 execution_time [+54.616ns; +121.784ns] or [+4.547%; +10.139%]

scenario:SamplingRuleMatchingBench/benchRegexMatching3

🟥 execution_time [+39.299ns; +134.301ns] or [+3.254%; +11.119%]

scenario:SamplingRuleMatchingBench/benchRegexMatching4

🟥 execution_time [+61.578ns; +139.222ns] or [+5.238%; +11.842%]

scenario:SpanBench/benchOpenTelemetryAPI

🟥 mem_peak [+1.442MB; +1.456MB] or [+3.468%; +3.502%]

scenario:TraceSerializationBench/benchSerializeTrace

🟩 execution_time [-36.628µs; -22.672µs] or [-8.232%; -5.095%]

appsec/src/extension/request_abort.c

cataphract · 2025-11-28T11:57:25Z

appsec/src/extension/request_abort.c

-        va_copy(args2, args);
-        php_verror(NULL, "", E_COMPILE_WARNING, format, args2);
-        va_end(args2);
+        _php_verror(E_COMPILE_WARNING, "%s", msg);


I can't as on this case, the same msg is used for the next _php_verror

cataphract · 2025-11-28T12:11:24Z

appsec/src/extension/request_lifecycle.c

        dd_request_abort_redirect();
    }
+
+    dd_request_abort_rshutdown();


you're calling on the top as well. I don't think the call on top is correct, because the abort functions need block_parameters. This also makes _reset_globals() too early.

So maybe after this block:

if (_enabled_user_req) { if (_cur_req_span) { mlog_g(dd_log_info, "Finishing user request whose corresponding " "span is presumably still unclosed on rshutdown"); _do_request_finish_user_req(true, _superglob_equiv, 0, NULL, NULL); _reset_globals(); } } else { _do_request_finish_php(ignore_verdict); // _rest_globals already called } // ADDED: _block_parameters_free()

OTOH this will not be reached if we call _emit_error from _do_request_finish_php... so I guess sth like:

--- a/appsec/src/extension/request_abort.c +++ b/appsec/src/extension/request_abort.c @@ -375,11 +375,13 @@ void dd_request_abort_redirect(void) ? ZSTR_VAL(_block_parameters->security_response_id) : ""); } else { + __auto_type *block_parameters = _block_parameters; + _block_parameters = NULL; _emit_error("Datadog blocked the request and attempted a redirection " "to %s. No action required. Security Response ID: %s", - ZSTR_VAL(_block_parameters->redirection_location), - _block_parameters->security_response_id - ? ZSTR_VAL(_block_parameters->security_response_id) + ZSTR_VAL(block_parameters->redirection_location), + block_parameters->security_response_id + ? ZSTR_VAL(block_parameters->security_response_id) : ""); } } @@ -462,10 +464,12 @@ void _request_abort_static_page(int response_code, int type) ? ZSTR_VAL(_block_parameters->security_response_id) : ""); } else { + __auto_type *block_parameters = _block_parameters; + _block_parameters = NULL; _emit_error("Datadog blocked the request and presented a static error " "page. No action required. Security Response ID: %s", - _block_parameters->security_response_id - ? ZSTR_VAL(_block_parameters->security_response_id) + block_parameters->security_response_id + ? ZSTR_VAL(block_parameters->security_response_id) : ""); } } @@ -552,12 +556,14 @@ static bool _abort_prelude(void) ? ZSTR_VAL(_block_parameters->security_response_id) : ""); } else { + __auto_type *block_parameters = _block_parameters; + _block_parameters = NULL; _emit_error( "Datadog blocked the request, but the response has already " "been partially committed. No action required. Security " "Response ID: %s", - _block_parameters->security_response_id - ? ZSTR_VAL(_block_parameters->security_response_id) + block_parameters->security_response_id + ? ZSTR_VAL(block_parameters->security_response_id) : ""); } return false;

I don't get this last comment

github-actions bot added tracing area:asm labels Nov 19, 2025

estringana force-pushed the estringana/fix-security-response-id-error-message branch 3 times, most recently from c332291 to c837f84 Compare November 21, 2025 10:40

estringana force-pushed the estringana/fix-security-response-id-error-message branch 3 times, most recently from 1f57085 to 07be196 Compare November 21, 2025 15:31

estringana added 6 commits November 26, 2025 16:33

Amend security_response_id being release before displaying it

3114270

Make emit error generic to use string arguments before releasing them

326d98f

Fix issue when compile warning

ee63260

Fix issue

98d0a64

Format

4c9e791

Clean blocking parameter on shutdown

7de55c1

estringana force-pushed the estringana/fix-security-response-id-error-message branch from b64885f to 7de55c1 Compare November 26, 2025 15:33

estringana marked this pull request as ready for review November 26, 2025 15:34

estringana requested a review from a team as a code owner November 26, 2025 15:34

cataphract reviewed Nov 27, 2025

View reviewed changes

appsec/src/extension/request_abort.c Outdated Show resolved Hide resolved

appsec/src/extension/request_abort.c Outdated Show resolved Hide resolved

appsec/src/extension/request_abort.c Outdated Show resolved Hide resolved

appsec/src/extension/request_abort.c Outdated Show resolved Hide resolved

Addres PR comments

0183e69

cataphract reviewed Nov 28, 2025

View reviewed changes

estringana and others added 2 commits November 28, 2025 16:33

Fix code

193c190

remove global block_params; centralize blocking logic

2e9efdd

cataphract force-pushed the estringana/fix-security-response-id-error-message branch from be24fc8 to 2e9efdd Compare November 28, 2025 16:52

Add comment

1947c09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Amend security_response_id being release before displaying it #3493

Amend security_response_id being release before displaying it #3493

Uh oh!

estringana commented Nov 19, 2025 •

edited

Loading

Uh oh!

codecov-commenter commented Nov 19, 2025 •

edited

Loading

Uh oh!

pr-commenter bot commented Nov 19, 2025 •

edited

Loading

Uh oh!

pr-commenter bot commented Nov 21, 2025 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cataphract Nov 28, 2025

Uh oh!

estringana Nov 28, 2025

Uh oh!

cataphract Nov 28, 2025 •

edited

Loading

Uh oh!

estringana Nov 28, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Amend security_response_id being release before displaying it #3493

Are you sure you want to change the base?

Amend security_response_id being release before displaying it #3493

Uh oh!

Conversation

estringana commented Nov 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Reviewer checklist

Uh oh!

codecov-commenter commented Nov 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

pr-commenter bot commented Nov 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Benchmarks [ appsec ]

Uh oh!

pr-commenter bot commented Nov 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Benchmarks [ tracer ]

scenario:PHPRedisBench/benchRedisOverhead

scenario:SamplingRuleMatchingBench/benchRegexMatching1

scenario:SamplingRuleMatchingBench/benchRegexMatching2

scenario:SamplingRuleMatchingBench/benchRegexMatching3

scenario:SamplingRuleMatchingBench/benchRegexMatching4

scenario:SpanBench/benchOpenTelemetryAPI

scenario:TraceSerializationBench/benchSerializeTrace

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cataphract Nov 28, 2025

Choose a reason for hiding this comment

Uh oh!

estringana Nov 28, 2025

Choose a reason for hiding this comment

Uh oh!

cataphract Nov 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

estringana Nov 28, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

estringana commented Nov 19, 2025 •

edited

Loading

codecov-commenter commented Nov 19, 2025 •

edited

Loading

pr-commenter bot commented Nov 19, 2025 •

edited

Loading

pr-commenter bot commented Nov 21, 2025 •

edited

Loading

cataphract Nov 28, 2025 •

edited

Loading