fix: Avoid double free of stack data

[junjihashimoto]

Description

https://github.com/DataDog/dd-trace-php/issues/3208

A double free may be occurring in ext/coms. The cause of this seems to be that ownership of the stack is shared between the writer and the main thread, and it is suspected that the writer is freeing things that have been freed by other threads. In this PR, we will avoid double frees by putting NULL into data for things that have been freed once, and checking this. It's not a perfect fix, but it should avoid double frees.

Reviewer checklist

• [ ] Test coverage seems ok.
• [ ] Appropriate labels assigned.

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 6.81%. Comparing base (de4e99f) to head (d475a60).

:exclamation: There is a different number of reports uploaded between BASE (de4e99f) and HEAD (d475a60). Click for more details.

HEAD has 7 uploads less than BASE

Flag	BASE (de4e99f)	HEAD (d475a60)
tracer-php	12	6
appsec-extension	1	0

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #3209       +/-   ##
============================================
- Coverage     76.46%   6.81%   -69.65%     
  Complexity     2927    2927               
============================================
  Files           141     114       -27     
  Lines         16025   11582     -4443     
  Branches       1107       0     -1107     
============================================
- Hits          12253     789    -11464     
- Misses         3197   10793     +7596     
+ Partials        575       0      -575     

Flag	Coverage Δ
appsec-extension	?
tracer-php	6.81% <ø> (-72.59%)	:arrow_down:

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

see 112 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 de4e99f...d475a60. Read the comment docs.

:rocket: New features to boost your workflow:

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

[bwoebi]

That might mitigate a problem a little, but the actual problem is that there are two writers (two threads). There should be only ever one single coms writer.

[junjihashimoto]

@bwoebi Sorry, there is no two. It's just a duplicated command issue.('backtrace' and 'thread apply all bt')

[bwoebi]

Oh, I see, I'm blind then - thanks! Looking again.

[junjihashimoto]

Hi @bwoebi, Should I output a log with ddtrace_bgs_logf if there is a possibility of a double free?

[junjihashimoto]

@bwoebi What do you think of the reviews? It's difficult to put valgrind into production here from a performance standpoint. On the other hand, it's possible to put this PR in and see if any problems occur.

[junjihashimoto]

Hi @bwoebi , How can I install this patch library into production? Is it uploaded to the CI artifacts?

[bwoebi]

Hey @junjihashimoto if you search for "package extension" in the CI artifacts, you'll get it's artifacts. What you'll want is the datadog-setup.php from the artifact list.

[bwoebi]

But the problem with your patch is that it may just mitigate the issue - ultimately an use-after-free may just segfault.

If stack is freed, accessing stack->data is not allowed.

[junjihashimoto]

@bwoebi It is correct. It just prevents double-free from corrupting memory, and now it seems like the memory is being corrupted. I think that the complete fix requires locks for ownership and has performance tradeoffs.

BTW, I can not see the artifact list in the artifact tab of circle ci. Am I looking in the wrong place?

[realFlowControl]

Hey there @junjihashimoto,

build artifacts are removed from storage after 7 days. I restarted that job for you at https://app.circleci.com/pipelines/github/DataDog/dd-trace-php/20558/workflows/71d751ac-61ea-4353-8ffd-7aec794284be so you can check this CI later for the build artifacts.

[junjihashimoto]

@realFlowControl Thank you! I downloaded the packages as follows:

datadog-php-tracer_1.9.0+d475a605fd30217da25318f62e55f0de491b74a4_amd64.deb
datadog-setup.php
datadog-tracer.stubs.php
dd-library-php-1.9.0+d475a605fd30217da25318f62e55f0de491b74a4-aarch64-linux-gnu-20190902-debug.tar.gz
dd-library-php-1.9.0+d475a605fd30217da25318f62e55f0de491b74a4-aarch64-linux-gnu-20190902-zts.tar.gz
dd-library-php-1.9.0+d475a605fd30217da25318f62e55f0de491b74a4-aarch64-linux-gnu-20190902.tar.gz