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

disable Appsec when FrankenPHP SAPI is detected #2617

Merged
merged 1 commit into from
Apr 9, 2024

Conversation

realFlowControl
Copy link
Member

@realFlowControl realFlowControl commented Apr 8, 2024

Description

When installing via php datadog-setup.php appsec will also be installed but crash FrankenPHP after few HTTP requests with the following stack trace:

Thread 21 "thpool-7" received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0xffff68f9f020 (LWP 835)]
dd_entity_body_rinit () at /home/circleci/datadog/appsec/src/extension/entity_body.c:112
112	/home/circleci/datadog/appsec/src/extension/entity_body.c: No such file or directory.
(gdb) bt
#0  dd_entity_body_rinit () at /home/circleci/datadog/appsec/src/extension/entity_body.c:112
#1  0x0000ffff80ccbf68 in zm_activate_ddappsec (type=<optimized out>, module_number=<optimized out>) at /home/circleci/datadog/appsec/src/extension/ddappsec.c:266
#2  0x0000fffff72130e4 in zend_activate_modules () at /usr/local/src/php/Zend/zend_API.c:3218
#3  0x0000fffff70ef4dc in php_request_startup () at /usr/local/src/php/main/main.c:1811
#4  0x000000000169058c in frankenphp_request_startup () at frankenphp.c:818
#5  0x000000000169060c in frankenphp_execute_script (file_name=0xffff40078b20 "/go/src/app/testdata/session.php") at frankenphp.c:833
#6  0x000000000168dce4 in _cgo_b7d6fd74a0c8_Cfunc_frankenphp_execute_script (v=0x4000718da8) at /tmp/go-build/cgo-gcc-prolog:55
#7  0x000000000047e66c in runtime.asmcgocall () at /usr/local/go/src/runtime/asm_arm64.s:999
#8  0x00000040007308c0 in ?? ()
#9  0x0000ffff68f9e5a0 in ?? ()
#10 0xf94013f54b0003e0 in ?? ()

This PR will deactivate appsec when it detects the FrankenPHP SAPI.

Reviewer checklist

  • Test coverage seems ok.
  • Appropriate labels assigned.

@codecov-commenter
Copy link

codecov-commenter commented Apr 8, 2024

Codecov Report

Merging #2617 (f4c4c82) into master (c067f72) will decrease coverage by 1.11%.
The diff coverage is 0.00%.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2617      +/-   ##
============================================
- Coverage     76.53%   75.43%   -1.11%     
  Complexity     2609     2609              
============================================
  Files           217      243      +26     
  Lines         23319    27319    +4000     
  Branches          0      986     +986     
============================================
+ Hits          17847    20607    +2760     
- Misses         5472     6186     +714     
- Partials          0      526     +526     
Flag Coverage Δ
appsec-extension 69.00% <0.00%> (?)
tracer-extension 78.70% <ø> (ø)
tracer-php 73.86% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
appsec/src/extension/ddappsec.c 76.74% <0.00%> (ø)

... and 25 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 c067f72...f4c4c82. Read the comment docs.

Copy link
Collaborator

@bwoebi bwoebi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, until appsec properly supports it.

Copy link
Contributor

@Anilm3 Anilm3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, thanks for updating this.

@realFlowControl realFlowControl merged commit c85ea8f into master Apr 9, 2024
606 checks passed
@realFlowControl realFlowControl deleted the feature/disable-appsec-for-frankenphp branch April 9, 2024 09:32
@github-actions github-actions bot added this to the 0.100.0 milestone Apr 9, 2024
@nklmilojevic
Copy link

Thanks for this!

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

Successfully merging this pull request may close these issues.

5 participants