Skip to content

chore(aap): ensure ctypes dynamic loading is unpatched #13386

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

Merged
merged 16 commits into from
May 30, 2025

Conversation

christophe-papazian
Copy link
Contributor

@christophe-papazian christophe-papazian commented May 12, 2025

Ensure that ctypes dynamic loading for libddwaf is using the unpatched version of subprocess.Popen.

  • avoid adverse interaction with other features
  • avoid adverse interaction with gevent

Changes:

  • keep a version of Popen in unpatched, by ensuring that all loaded modules to access it are unloaded.
  • add a context in appsec/_utils to patch unpatch popen and avoid any side effects of patches inside the context
  • use the context to load all required files for the waf

APPSEC-57808

Checklist

  • PR author has checked that all the criteria below are met
  • The PR description includes an overview of the change
  • The PR description articulates the motivation for the change
  • The change includes tests OR the PR description describes a testing strategy
  • The PR description notes risks associated with the change, if any
  • Newly-added code is easy to change
  • The change follows the library release note guidelines
  • The change includes or references documentation updates if necessary
  • Backport labels are set (if applicable)

Reviewer Checklist

  • Reviewer has checked that all the criteria below are met
  • Title is accurate
  • All changes are related to the pull request's stated goal
  • Avoids breaking API changes
  • Testing strategy adequately addresses listed risks
  • Newly-added code is easy to change
  • Release note makes sense to a user of the library
  • If necessary, author has acknowledged and discussed the performance implications of this PR as reported in the benchmarks PR comment
  • Backport labels are set in a manner that is consistent with the release branch maintenance policy

Copy link
Contributor

github-actions bot commented May 12, 2025

CODEOWNERS have been resolved as:

ddtrace/appsec/_ddwaf/ddwaf_types.py                                    @DataDog/asm-python
ddtrace/appsec/_processor.py                                            @DataDog/asm-python
ddtrace/appsec/_utils.py                                                @DataDog/asm-python
ddtrace/internal/_unpatched.py                                          @DataDog/python-guild

Copy link
Contributor

github-actions bot commented May 12, 2025

Bootstrap import analysis

Comparison of import times between this PR and base.

Summary

The average import time from this PR is: 219 ± 4 ms.

The average import time from base is: 222 ± 5 ms.

The import time difference between this PR and base is: -3.6 ± 0.2 ms.

Import time breakdown

The following import paths have appeared:

ddtrace.auto 4.959 ms (2.27%)
ddtrace 4.959 ms (2.27%)
ddtrace.internal._unpatched 4.959 ms (2.27%)
subprocess 4.959 ms (2.27%)
selectors 1.485 ms (0.68%)
math 0.188 ms (0.09%)
collections.abc 0.175 ms (0.08%)
locale 0.788 ms (0.36%)
_locale 0.072 ms (0.03%)
signal 0.646 ms (0.30%)
contextlib 0.552 ms (0.25%)
warnings 0.278 ms (0.13%)
fcntl 0.171 ms (0.08%)
select 0.163 ms (0.07%)
_posixsubprocess 0.145 ms (0.07%)
msvcrt 0.071 ms (0.03%)
errno 0.062 ms (0.03%)

The following import paths have disappeared:

ddtrace.auto 6.023 ms (2.75%)
ddtrace 6.023 ms (2.75%)
ddtrace._logger 4.676 ms (2.14%)
ddtrace.internal.telemetry 3.046 ms (1.39%)
ddtrace.internal.telemetry.writer 1.494 ms (0.68%)
http.client 0.874 ms (0.40%)
email.parser 0.874 ms (0.40%)
email.feedparser 0.874 ms (0.40%)
email._policybase 0.874 ms (0.40%)
email.utils 0.874 ms (0.40%)
email._parseaddr 0.874 ms (0.40%)
calendar 0.874 ms (0.40%)
locale 0.874 ms (0.40%)
_locale 0.078 ms (0.04%)
ddtrace.internal.atexit 0.620 ms (0.28%)
signal 0.620 ms (0.28%)
ddtrace.settings._agent 1.307 ms (0.60%)
socket 1.307 ms (0.60%)
selectors 1.307 ms (0.60%)
select 0.179 ms (0.08%)
ddtrace.internal.utils.formats 0.245 ms (0.11%)
ddtrace.internal.compat 0.245 ms (0.11%)
pathlib 0.245 ms (0.11%)
urllib.parse 0.193 ms (0.09%)
math 0.193 ms (0.09%)
errno 0.052 ms (0.02%)
logging 1.630 ms (0.75%)
traceback 1.340 ms (0.61%)
contextlib 1.181 ms (0.54%)
collections.abc 0.158 ms (0.07%)
warnings 0.291 ms (0.13%)
ddtrace.settings._config 1.346 ms (0.62%)
ddtrace.internal.gitmetadata 1.346 ms (0.62%)
ddtrace.ext.ci 1.346 ms (0.62%)
ddtrace.ext.git 1.346 ms (0.62%)
subprocess 1.346 ms (0.62%)
fcntl 0.287 ms (0.13%)
_posixsubprocess 0.264 ms (0.12%)
msvcrt 0.106 ms (0.05%)

The following import paths have grown:

ddtrace.auto 4.299 ms (1.97%)
ddtrace 3.203 ms (1.46%)
ddtrace._logger 2.123 ms (0.97%)
ddtrace.internal.telemetry 2.069 ms (0.95%)
ddtrace.internal.telemetry.writer 1.315 ms (0.60%)
ddtrace.internal.utils.version 0.846 ms (0.39%)
ddtrace.vendor.packaging.version 0.846 ms (0.39%)
ddtrace.vendor.packaging 0.846 ms (0.39%)
ddtrace.vendor 0.846 ms (0.39%)
ddtrace.internal.module 0.846 ms (0.39%)
ddtrace.internal.wrapping.context 0.846 ms (0.39%)
ddtrace.internal.wrapping 0.846 ms (0.39%)
bytecode 0.583 ms (0.27%)
bytecode.bytecode 0.583 ms (0.27%)
ddtrace.internal.assembly 0.263 ms (0.12%)
http.client 0.182 ms (0.08%)
email.parser 0.101 ms (0.05%)
email.feedparser 0.101 ms (0.05%)
email._policybase 0.101 ms (0.05%)
email.utils 0.055 ms (0.03%)
email._parseaddr 0.055 ms (0.03%)
calendar 0.055 ms (0.03%)
email.header 0.046 ms (0.02%)
ddtrace.internal.forksafe 0.090 ms (0.04%)
wrapt 0.090 ms (0.04%)
wrapt.__wrapt__ 0.090 ms (0.04%)
wrapt.wrappers 0.090 ms (0.04%)
ddtrace.internal.telemetry.data 0.067 ms (0.03%)
ddtrace.internal.runtime.container 0.067 ms (0.03%)
ddtrace.internal.runtime 0.066 ms (0.03%)
uuid 0.066 ms (0.03%)
platform 0.066 ms (0.03%)
ddtrace.internal.encoding 0.064 ms (0.03%)
ddtrace.internal._encoding 0.064 ms (0.03%)
ddtrace.settings._agent 0.597 ms (0.27%)
ddtrace.settings 0.519 ms (0.24%)
ddtrace.settings.http 0.519 ms (0.24%)
ddtrace.internal.utils.http 0.519 ms (0.24%)
email.encoders 0.519 ms (0.24%)
base64 0.519 ms (0.24%)
ddtrace.internal.utils.formats 0.157 ms (0.07%)
ddtrace.internal.compat 0.157 ms (0.07%)
pathlib 0.043 ms (0.02%)
logging 0.053 ms (0.02%)
weakref 0.053 ms (0.02%)
ddtrace._monkey 0.605 ms (0.28%)
ddtrace.appsec._listeners 0.605 ms (0.28%)
ddtrace.internal.core 0.605 ms (0.28%)
ddtrace.internal.core.event_hub 0.605 ms (0.28%)
ddtrace.trace 0.264 ms (0.12%)
ddtrace._trace.filters 0.264 ms (0.12%)
ddtrace._trace.processor 0.264 ms (0.12%)
ddtrace.internal.dogstatsd 0.096 ms (0.04%)
ddtrace.vendor.dogstatsd 0.096 ms (0.04%)
ddtrace.vendor.dogstatsd.base 0.096 ms (0.04%)
queue 0.096 ms (0.04%)
heapq 0.096 ms (0.04%)
ddtrace._trace.sampler 0.077 ms (0.04%)
ddtrace._trace.span 0.077 ms (0.04%)
ddtrace.internal.sampling 0.077 ms (0.04%)
ddtrace._trace.sampling_rule 0.077 ms (0.04%)
ddtrace.settings._config 0.212 ms (0.10%)
ddtrace.internal.gitmetadata 0.140 ms (0.06%)
ddtrace.ext.ci 0.140 ms (0.06%)
ddtrace.ext.git 0.140 ms (0.06%)
shutil 0.053 ms (0.02%)
ddtrace.settings.endpoint_config 0.071 ms (0.03%)
ddtrace.internal.utils.retry 0.071 ms (0.03%)
ddtrace.bootstrap.sitecustomize 1.097 ms (0.50%)
ddtrace.bootstrap.preload 1.097 ms (0.50%)
ddtrace.settings.profiling 0.606 ms (0.28%)
ddtrace.internal.datadog.profiling.ddup 0.606 ms (0.28%)
ddtrace.internal.datadog.profiling.ddup._ddup 0.606 ms (0.28%)
ddtrace.internal.remoteconfig.worker 0.490 ms (0.22%)

The following import paths have shrunk:

ddtrace.auto 5.780 ms (2.64%)
ddtrace 3.360 ms (1.54%)
ddtrace._logger 1.495 ms (0.68%)
ddtrace.internal.telemetry 1.419 ms (0.65%)
ddtrace.internal.telemetry.writer 1.116 ms (0.51%)
ddtrace.internal.utils.version 0.636 ms (0.29%)
ddtrace.vendor.packaging.version 0.636 ms (0.29%)
ddtrace.vendor.packaging 0.636 ms (0.29%)
ddtrace.vendor 0.636 ms (0.29%)
ddtrace.internal.module 0.636 ms (0.29%)
http.client 0.238 ms (0.11%)
email.parser 0.154 ms (0.07%)
email.feedparser 0.154 ms (0.07%)
email._policybase 0.084 ms (0.04%)
email.utils 0.084 ms (0.04%)
random 0.084 ms (0.04%)
bisect 0.012 ms (0.01%)
email.errors 0.071 ms (0.03%)
http 0.084 ms (0.04%)
ddtrace.internal.forksafe 0.092 ms (0.04%)
wrapt 0.076 ms (0.03%)
wrapt.decorators 0.076 ms (0.03%)
ddtrace.internal.runtime 0.081 ms (0.04%)
uuid 0.081 ms (0.04%)
ddtrace.internal.telemetry.data 0.068 ms (0.03%)
ddtrace.internal.packages 0.068 ms (0.03%)
_sysconfigdata__linux_x86_64-linux-gnu 0.068 ms (0.03%)
ddtrace.settings._agent 0.216 ms (0.10%)
ddtrace.settings 0.132 ms (0.06%)
ddtrace.settings.http 0.132 ms (0.06%)
ddtrace.internal.utils.cache 0.132 ms (0.06%)
inspect 0.132 ms (0.06%)
ast 0.132 ms (0.06%)
_ast 0.132 ms (0.06%)
ddtrace.settings._core 0.084 ms (0.04%)
envier 0.084 ms (0.04%)
envier.env 0.084 ms (0.04%)
ddtrace.internal.utils.formats 0.087 ms (0.04%)
ddtrace.internal.compat 0.087 ms (0.04%)
pathlib 0.087 ms (0.04%)
urllib.parse 0.087 ms (0.04%)
logging 0.076 ms (0.03%)
string 0.076 ms (0.03%)
ddtrace.settings._config 0.876 ms (0.40%)
ddtrace.internal.gitmetadata 0.222 ms (0.10%)
ddtrace.ext.ci 0.222 ms (0.10%)
ddtrace.ext.git 0.114 ms (0.05%)
shutil 0.068 ms (0.03%)
lzma 0.068 ms (0.03%)
tempfile 0.046 ms (0.02%)
ddtrace.internal._file_queue 0.086 ms (0.04%)
secrets 0.086 ms (0.04%)
hmac 0.086 ms (0.04%)
ddtrace.trace 0.276 ms (0.13%)
ddtrace._trace.filters 0.276 ms (0.13%)
ddtrace._trace.processor 0.276 ms (0.13%)
ddtrace._trace.sampler 0.173 ms (0.08%)
ddtrace._trace.span 0.173 ms (0.08%)
ddtrace.internal.sampling 0.074 ms (0.03%)
ddtrace._trace.sampling_rule 0.074 ms (0.03%)
ddtrace.internal.glob_matching 0.074 ms (0.03%)
ddtrace.internal.dogstatsd 0.103 ms (0.05%)
ddtrace.vendor.dogstatsd 0.103 ms (0.05%)
ddtrace.vendor.dogstatsd.base 0.103 ms (0.05%)
queue 0.103 ms (0.05%)
heapq 0.103 ms (0.05%)
_heapq 0.103 ms (0.05%)
ddtrace._monkey 0.076 ms (0.03%)
ddtrace.appsec._listeners 0.076 ms (0.03%)
ddtrace.internal.core 0.076 ms (0.03%)
ddtrace.bootstrap.sitecustomize 2.420 ms (1.11%)
ddtrace.bootstrap.preload 2.420 ms (1.11%)
ddtrace.internal.remoteconfig.client 1.106 ms (0.51%)
ddtrace.internal.runtime.runtime_metrics 0.608 ms (0.28%)
ddtrace.internal.runtime.metric_collectors 0.608 ms (0.28%)
ddtrace.internal.runtime.collector 0.608 ms (0.28%)

@pr-commenter
Copy link

pr-commenter bot commented May 12, 2025

Benchmarks

Benchmark execution time: 2025-05-28 15:30:25

Comparing candidate commit 661c258 in PR branch christophe-papazian/fix_waf_init_with_gevent with baseline commit 3eaf6a8 in branch main.

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

@juanjux
Copy link
Collaborator

juanjux commented May 22, 2025

Nice. Also make easier to find the possible breaking PR (the last one merged).

@christophe-papazian christophe-papazian changed the base branch from main to chore/di-in-product-enablement May 26, 2025 14:40
@christophe-papazian christophe-papazian force-pushed the christophe-papazian/fix_waf_init_with_gevent branch from 8a8fbe9 to 88ad87b Compare May 26, 2025 14:50
@christophe-papazian christophe-papazian changed the base branch from chore/di-in-product-enablement to main May 26, 2025 14:51
@christophe-papazian christophe-papazian changed the title chore(debugging): in-product enablement chore(asm): ensure ctype dynamic loading is unpatched. May 26, 2025
@christophe-papazian christophe-papazian changed the title chore(asm): ensure ctype dynamic loading is unpatched. chore(aap): ensure ctype dynamic loading is unpatched May 26, 2025
@christophe-papazian christophe-papazian added changelog/no-changelog A changelog entry is not required for this PR. ASM Application Security Monitoring labels May 27, 2025
@christophe-papazian christophe-papazian changed the title chore(aap): ensure ctype dynamic loading is unpatched chore(aap): ensure ctypes dynamic loading is unpatched May 27, 2025
@christophe-papazian christophe-papazian marked this pull request as ready for review May 27, 2025 14:40
@christophe-papazian christophe-papazian requested review from a team as code owners May 27, 2025 14:40
@christophe-papazian christophe-papazian requested review from emmettbutler and P403n1x87 and removed request for tabgok May 28, 2025 16:06
@avara1986 avara1986 merged commit 8ac0c91 into main May 30, 2025
728 of 729 checks passed
@avara1986 avara1986 deleted the christophe-papazian/fix_waf_init_with_gevent branch May 30, 2025 09:42
@P403n1x87
Copy link
Contributor

P403n1x87 commented Jun 2, 2025

When I was looking into this a few weeks ago, my realisation was that, by removing the need for https://github.com/Datadog/dd-trace-py/blob/16054515e292a66c5eaf79b9ea62df6f348cd67e/ddtrace/appsec/_processor.py#L118, the problem underlying this PR would not occur. A possible way of making delayed_init obsolete could be achieved by removing the appsec logic from the tracer https://github.com/Datadog/dd-trace-py/blob/16054515e292a66c5eaf79b9ea62df6f348cd67e/ddtrace/_trace/tracer.py#L108, and using the processor registration API instead https://github.com/Datadog/dd-trace-py/blob/16054515e292a66c5eaf79b9ea62df6f348cd67e/ddtrace/_trace/processor/__init__.py#L108

Attempt: 91b7508

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ASM Application Security Monitoring changelog/no-changelog A changelog entry is not required for this PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants