Skip to content

chore(iast): optimize modulo aspect more (3/3) #10514

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 44 commits into from
Sep 10, 2024

Conversation

juanjux
Copy link
Collaborator

@juanjux juanjux commented Sep 4, 2024

Description

Further optimize the native modulo aspect. This times we have a 8x performance improvement over the second part of this PR or a 16x performance improvement over the original version.

Also:

  • The TRY_CATCH macro now accepts the return value expression as a parameter.
  • aspect_modulo now operates on PyObject* directly and uses CPython call mechanism (instead of PyBind11 one).
  • as_formatted_evidence now works with std::string to easy usage with aspects working with PyObject*.
  • All overloads of [api_]as_formatted_evidence now work with TaintRange references.
  • Added an overload to api_convert_escaped_text_to_tainted_text working directly with PyObject*.
  • CMakeLists.txt now fetches and links the ICU Unicode library.
  • Adds some utlity functions to convert between text types: AnyTextObjectToString, get_pytext_type, StringToPyObject, PyObjectToString and PyObjectToPyText.

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

Signed-off-by: Juanjo Alvarez <[email protected]>
Signed-off-by: Juanjo Alvarez <[email protected]>
Signed-off-by: Juanjo Alvarez <[email protected]>
Signed-off-by: Juanjo Alvarez <[email protected]>
Signed-off-by: Juanjo Alvarez <[email protected]>
…github.com:DataDog/dd-trace-py into juanjux/APPSEC-54648-optimize-operator-modulo-more
Signed-off-by: Juanjo Alvarez <[email protected]>
Signed-off-by: Juanjo Alvarez <[email protected]>
Signed-off-by: Juanjo Alvarez <[email protected]>
Signed-off-by: Juanjo Alvarez <[email protected]>
Signed-off-by: Juanjo Alvarez <[email protected]>
Signed-off-by: Juanjo Alvarez <[email protected]>
@juanjux juanjux requested a review from a team as a code owner September 4, 2024 20:33
@juanjux juanjux self-assigned this Sep 4, 2024
@juanjux juanjux added ASM Application Security Monitoring changelog/no-changelog A changelog entry is not required for this PR. labels Sep 4, 2024
@datadog-dd-trace-py-rkomorn
Copy link

datadog-dd-trace-py-rkomorn bot commented Sep 4, 2024

Datadog Report

Branch report: juanjux/optimize-as-formatted-evidence
Commit report: a015c47
Test service: dd-trace-py

❌ 245 Failed (0 Known Flaky), 1332 Passed, 756 Skipped, 7m 14.53s Total Time

❌ Failed Tests (245)

This report shows up to 5 failed tests.

  • test_django_command_injection - test_django_appsec_iast.py - Details

    Expand for error
     No module named 'ddtrace.appsec._iast._taint_tracking._native'
    
  • test_django_command_injection - test_django_appsec_iast.py - Details

    Expand for error
     No module named 'ddtrace.appsec._iast._taint_tracking._native'
    
  • test_django_command_injection - test_django_appsec_iast.py - Details

    Expand for error
     No module named 'ddtrace.appsec._iast._taint_tracking._native'
    
  • test_django_command_injection - test_django_appsec_iast.py - Details

    Expand for error
     No module named 'ddtrace.appsec._iast._taint_tracking._native'
    
  • test_django_header_injection - test_django_appsec_iast.py - Details

    Expand for error
     No module named 'ddtrace.appsec._iast._taint_tracking._native'
    

Copy link
Contributor

github-actions bot commented Sep 4, 2024

CODEOWNERS have been resolved as:

ddtrace/appsec/_iast/_taint_tracking/Aspects/AspectIndex.cpp            @DataDog/asm-python
ddtrace/appsec/_iast/_taint_tracking/Aspects/AspectModulo.cpp           @DataDog/asm-python
ddtrace/appsec/_iast/_taint_tracking/Aspects/AspectModulo.h             @DataDog/asm-python
ddtrace/appsec/_iast/_taint_tracking/Aspects/AspectOperatorAdd.cpp      @DataDog/asm-python
ddtrace/appsec/_iast/_taint_tracking/Aspects/AspectSlice.cpp            @DataDog/asm-python
ddtrace/appsec/_iast/_taint_tracking/Aspects/AspectSplit.cpp            @DataDog/asm-python
ddtrace/appsec/_iast/_taint_tracking/Aspects/AspectsOsPath.cpp          @DataDog/asm-python
ddtrace/appsec/_iast/_taint_tracking/Aspects/Helpers.cpp                @DataDog/asm-python
ddtrace/appsec/_iast/_taint_tracking/Aspects/Helpers.h                  @DataDog/asm-python
ddtrace/appsec/_iast/_taint_tracking/Aspects/_aspects_exports.h         @DataDog/asm-python
ddtrace/appsec/_iast/_taint_tracking/CMakeLists.txt                     @DataDog/asm-python
ddtrace/appsec/_iast/_taint_tracking/Utils/StringUtils.cpp              @DataDog/asm-python
ddtrace/appsec/_iast/_taint_tracking/Utils/StringUtils.h                @DataDog/asm-python
ddtrace/appsec/_iast/_taint_tracking/__init__.py                        @DataDog/asm-python
ddtrace/appsec/_iast/_taint_tracking/_native.cpp                        @DataDog/asm-python
ddtrace/appsec/_iast/_taint_tracking/aspects.py                         @DataDog/asm-python

Signed-off-by: Juanjo Alvarez <[email protected]>
…ataDog/dd-trace-py into juanjux/optimize-as-formatted-evidence
@juanjux juanjux requested review from a team as code owners September 5, 2024 11:10
@juanjux juanjux removed request for a team and emmettbutler September 5, 2024 14:23
@juanjux juanjux enabled auto-merge (squash) September 6, 2024 17:07
@avara1986 avara1986 disabled auto-merge September 9, 2024 07:17
Copy link
Member

@avara1986 avara1986 left a comment

Choose a reason for hiding this comment

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

LGTM but we're going to wait to 2.13 release candidate first

@juanjux juanjux enabled auto-merge (squash) September 10, 2024 07:49
@juanjux juanjux merged commit e87be1a into main Sep 10, 2024
419 checks passed
@juanjux juanjux deleted the juanjux/optimize-as-formatted-evidence branch September 10, 2024 07:56
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.

3 participants