From dc1dc3a3b3a54a222c7bcd0ff076000247103fe4 Mon Sep 17 00:00:00 2001 From: Taegyun Kim Date: Mon, 7 Oct 2024 04:47:33 -0400 Subject: [PATCH 1/3] chore: set build parallel level for hatch ddtrace_unit_tests (#10952) ## Checklist - [x] 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](https://ddtrace.readthedocs.io/en/stable/releasenotes.html) - The change includes or references documentation updates if necessary - Backport labels are set (if [applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)) ## 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](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces) 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](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting) --- hatch.toml | 1 + 1 file changed, 1 insertion(+) diff --git a/hatch.toml b/hatch.toml index 0c619d273f5..c582f8f6a9d 100644 --- a/hatch.toml +++ b/hatch.toml @@ -369,6 +369,7 @@ dependencies = [ [envs.ddtrace_unit_tests.env-vars] DD_IAST_ENABLED = "false" DD_REMOTE_CONFIGURATION_ENABLED = "false" +CMAKE_BUILD_PARALLEL_LEVEL="6" [envs.ddtrace_unit_tests.scripts] test = [ From c4cfa38673986f41ddb82aa7d027375040ae5c3a Mon Sep 17 00:00:00 2001 From: Juanjo Alvarez Martinez Date: Mon, 7 Oct 2024 11:41:17 +0200 Subject: [PATCH 2/3] chore(iast): fix all known memleaks in the native module + safety fixes (#10947) Signed-off-by: Juanjo Alvarez ## Description - Move the `TaintedObjectPtr` from `TaintedObject*` to `share_ptr`. This fixes the infamous `flask-appsec-iast-p100` leak on the `add_aspect`. - Fix a leak when iterating a `Py_List` on `AspectJoin`. - Handle a missing case in `AspectOperatorAdd`. - Fix leak in `args_are_text_and_same_type`. - Add some nullptr-checks. Thanks @avara1986 for the joint work on this. ## Checklist - [X] 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](https://ddtrace.readthedocs.io/en/stable/releasenotes.html) - The change includes or references documentation updates if necessary - Backport labels are set (if [applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)) ## Reviewer Checklist - [x] 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](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces) 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](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting) --------- Signed-off-by: Juanjo Alvarez Co-authored-by: Alberto Vara --- .gitignore | 13 ++- .../_taint_tracking/Aspects/AspectJoin.cpp | 13 +-- .../Aspects/AspectOperatorAdd.cpp | 36 ++++++--- .../Initializer/Initializer.cpp | 27 +------ .../_taint_tracking/Initializer/Initializer.h | 2 - .../TaintTracking/TaintRange.cpp | 8 +- .../TaintTracking/TaintRange.h | 4 +- .../TaintTracking/TaintedObject.cpp | 23 ------ .../TaintTracking/TaintedObject.h | 9 +-- .../_taint_tracking/Utils/StringUtils.cpp | 6 ++ .../_iast/_taint_tracking/Utils/StringUtils.h | 14 +++- .../_taint_tracking/tests/test_add_aspect.cpp | 79 +++++++++++++++++++ .../_taint_tracking/tests/test_common.hpp | 18 +++++ .../_taint_tracking/tests/test_helpers.cpp | 1 - .../tests/test_index_aspect.cpp | 1 - .../_taint_tracking/tests/test_other.cpp | 1 - scripts/iast/mod_leak_functions.py | 4 +- 17 files changed, 168 insertions(+), 91 deletions(-) create mode 100644 ddtrace/appsec/_iast/_taint_tracking/tests/test_add_aspect.cpp diff --git a/.gitignore b/.gitignore index ce0f64a9433..f6b1906f30c 100644 --- a/.gitignore +++ b/.gitignore @@ -141,10 +141,21 @@ ddtrace/_version.py artifacts/ # IAST cpp +ddtrace/appsec/_iast/_taint_tracking/cmake_install.cmake +ddtrace/appsec/_iast/_taint_tracking/CMakeCache.txt +ddtrace/appsec/_iast/_taint_tracking/Makefile ddtrace/appsec/_iast/_taint_tracking/cmake-build-debug/* ddtrace/appsec/_iast/_taint_tracking/_deps/* ddtrace/appsec/_iast/_taint_tracking/CMakeFiles/* - +ddtrace/appsec/_iast/_taint_tracking/tests/CMakeFiles/* +ddtrace/appsec/_iast/_taint_tracking/tests/cmake_install.cmake +ddtrace/appsec/_iast/_taint_tracking/tests/CMakeCache.txt +ddtrace/appsec/_iast/_taint_tracking/tests/Makefile +ddtrace/appsec/_iast/_taint_tracking/tests/CTestTestfile.cmake +ddtrace/appsec/_iast/_taint_tracking/tests/native_tests* +ddtrace/appsec/_iast/_taint_tracking/_vendor/pybind11/Makefile +ddtrace/appsec/_iast/_taint_tracking/_vendor/pybind11/CMakeFiles/* +ddtrace/appsec/_iast/_taint_tracking/_vendor/pybind11/cmake_install.cmake # CircleCI generated config .circleci/config.gen.yml diff --git a/ddtrace/appsec/_iast/_taint_tracking/Aspects/AspectJoin.cpp b/ddtrace/appsec/_iast/_taint_tracking/Aspects/AspectJoin.cpp index 60d4aa3dea1..476246e2a84 100644 --- a/ddtrace/appsec/_iast/_taint_tracking/Aspects/AspectJoin.cpp +++ b/ddtrace/appsec/_iast/_taint_tracking/Aspects/AspectJoin.cpp @@ -55,7 +55,7 @@ aspect_join_str(PyObject* sep, PyObject* new_result{ new_pyobject_id(result) }; set_tainted_object(new_result, result_to, tx_taint_map); - Py_DecRef(result); + Py_DECREF(result); return new_result; } @@ -134,7 +134,7 @@ aspect_join(PyObject* sep, PyObject* result, PyObject* iterable_elements, const PyObject* new_result{ new_pyobject_id(result) }; set_tainted_object(new_result, result_to, tx_taint_map); - Py_DecRef(result); + Py_DECREF(result); return new_result; } @@ -158,9 +158,10 @@ api_join_aspect(PyObject* self, PyObject* const* args, const Py_ssize_t nargs) PyObject* list_aux = PyList_New(0); while ((item = PyIter_Next(iterator))) { PyList_Append(list_aux, item); + Py_DECREF(item); } arg0 = list_aux; - Py_DecRef(iterator); + Py_DECREF(iterator); decref_arg0 = true; } } @@ -181,7 +182,7 @@ api_join_aspect(PyObject* self, PyObject* const* args, const Py_ssize_t nargs) if (has_pyerr()) { if (decref_arg0) { - Py_DecRef(arg0); + Py_DECREF(arg0); } return nullptr; } @@ -190,13 +191,13 @@ api_join_aspect(PyObject* self, PyObject* const* args, const Py_ssize_t nargs) if (not ctx_map or ctx_map->empty() or get_pyobject_size(result) == 0) { // Empty result cannot have taint ranges if (decref_arg0) { - Py_DecRef(arg0); + Py_DECREF(arg0); } return result; } auto res = aspect_join(sep, result, arg0, ctx_map); if (decref_arg0) { - Py_DecRef(arg0); + Py_DECREF(arg0); } return res; }); diff --git a/ddtrace/appsec/_iast/_taint_tracking/Aspects/AspectOperatorAdd.cpp b/ddtrace/appsec/_iast/_taint_tracking/Aspects/AspectOperatorAdd.cpp index 0d613375aca..a92a1436f95 100644 --- a/ddtrace/appsec/_iast/_taint_tracking/Aspects/AspectOperatorAdd.cpp +++ b/ddtrace/appsec/_iast/_taint_tracking/Aspects/AspectOperatorAdd.cpp @@ -21,17 +21,16 @@ add_aspect(PyObject* result_o, const size_t len_candidate_text{ get_pyobject_size(candidate_text) }; const size_t len_text_to_add{ get_pyobject_size(text_to_add) }; - if (len_text_to_add == 0 and len_candidate_text > 0) { - return candidate_text; - } - if (len_text_to_add > 0 and len_candidate_text == 0 and text_to_add == result_o) { - return text_to_add; + // Appended from or with nothing, ranges didn't change + if ((len_text_to_add == 0 and len_candidate_text > 0) || + (len_text_to_add > 0 and len_candidate_text == 0 and text_to_add == result_o)) { + return result_o; } const auto& to_candidate_text = get_tainted_object(candidate_text, tx_taint_map); if (to_candidate_text and to_candidate_text->get_ranges().size() >= TaintedObject::TAINT_RANGE_LIMIT) { const auto& res_new_id = new_pyobject_id(result_o); - Py_DecRef(result_o); + Py_DECREF(result_o); // If left side is already at the maximum taint ranges, we just reuse its // ranges, we don't need to look at left side. set_tainted_object(res_new_id, to_candidate_text, tx_taint_map); @@ -44,12 +43,20 @@ add_aspect(PyObject* result_o, } if (!to_text_to_add) { const auto& res_new_id = new_pyobject_id(result_o); - Py_DecRef(result_o); + Py_DECREF(result_o); set_tainted_object(res_new_id, to_candidate_text, tx_taint_map); return res_new_id; } - auto tainted = initializer->allocate_tainted_object_copy(to_candidate_text); + if (!to_candidate_text) { + const auto tainted = initializer->allocate_tainted_object(); + tainted->add_ranges_shifted(to_text_to_add, static_cast(len_candidate_text)); + set_tainted_object(result_o, tainted, tx_taint_map); + } + + // At this point we have both to_candidate_text and to_text_to_add to we add the + // ranges from both to result_o + const auto tainted = initializer->allocate_tainted_object_copy(to_candidate_text); tainted->add_ranges_shifted(to_text_to_add, static_cast(len_candidate_text)); set_tainted_object(result_o, tainted, tx_taint_map); @@ -84,6 +91,9 @@ api_add_aspect(PyObject* self, PyObject* const* args, Py_ssize_t nargs) // PyNumber_Add actually works for any type! result_o = PyNumber_Add(candidate_text, text_to_add); + if (result_o == nullptr) { + return nullptr; + } TRY_CATCH_ASPECT("add_aspect", return result_o, , { const auto tx_map = Initializer::get_tainting_map(); @@ -116,8 +126,6 @@ api_add_aspect(PyObject* self, PyObject* const* args, Py_ssize_t nargs) PyObject* api_add_inplace_aspect(PyObject* self, PyObject* const* args, Py_ssize_t nargs) { - PyObject* result_o = nullptr; - if (nargs != 2) { py::set_error(PyExc_ValueError, MSG_ERROR_N_PARAMS); return nullptr; @@ -125,7 +133,10 @@ api_add_inplace_aspect(PyObject* self, PyObject* const* args, Py_ssize_t nargs) PyObject* candidate_text = args[0]; PyObject* text_to_add = args[1]; - result_o = PyNumber_InPlaceAdd(candidate_text, text_to_add); + PyObject* result_o = PyNumber_InPlaceAdd(candidate_text, text_to_add); + if (result_o == nullptr) { + return nullptr; + } TRY_CATCH_ASPECT("add_inplace_aspect", return result_o, , { const auto tx_map = Initializer::get_tainting_map(); @@ -152,7 +163,6 @@ api_add_inplace_aspect(PyObject* self, PyObject* const* args, Py_ssize_t nargs) is_notinterned_notfasttainted_unicode(text_to_add)) { return result_o; } - candidate_text = add_aspect(result_o, candidate_text, text_to_add, tx_map); - return candidate_text; + return add_aspect(result_o, candidate_text, text_to_add, tx_map); }); } \ No newline at end of file diff --git a/ddtrace/appsec/_iast/_taint_tracking/Initializer/Initializer.cpp b/ddtrace/appsec/_iast/_taint_tracking/Initializer/Initializer.cpp index 73b934366e2..657d8a92e10 100644 --- a/ddtrace/appsec/_iast/_taint_tracking/Initializer/Initializer.cpp +++ b/ddtrace/appsec/_iast/_taint_tracking/Initializer/Initializer.cpp @@ -14,7 +14,7 @@ Initializer::Initializer() { // Fill the taintedobjects stack for (int i = 0; i < TAINTEDOBJECTS_STACK_SIZE; i++) { - available_taintedobjects_stack.push(new TaintedObject()); + available_taintedobjects_stack.push(make_shared()); } // Fill the ranges stack @@ -41,9 +41,6 @@ Initializer::clear_tainting_map(const TaintRangeMapTypePtr& tx_map) return; } std::lock_guard lock(active_map_addreses_mutex); - for (const auto& [fst, snd] : *tx_map) { - snd.second->decref(); - } tx_map->clear(); active_map_addreses.erase(tx_map.get()); } @@ -114,12 +111,12 @@ TaintedObjectPtr Initializer::allocate_tainted_object() { if (!available_taintedobjects_stack.empty()) { - const auto& toptr = available_taintedobjects_stack.top(); + const auto toptr = available_taintedobjects_stack.top(); available_taintedobjects_stack.pop(); return toptr; } // Stack is empty, create new object - return new TaintedObject(); + return make_shared(); } TaintedObjectPtr @@ -147,24 +144,6 @@ Initializer::allocate_tainted_object_copy(const TaintedObjectPtr& from) return allocate_ranges_into_taint_object_copy(from->ranges_); } -void -Initializer::release_tainted_object(TaintedObjectPtr tobj) -{ - if (!tobj) { - return; - } - - tobj->reset(); - if (available_taintedobjects_stack.size() < TAINTEDOBJECTS_STACK_SIZE) { - available_taintedobjects_stack.push(tobj); - return; - } - - // Stack full, just delete the object (but to a reset before so ranges are - // reused or freed) - delete tobj; -} - TaintRangePtr Initializer::allocate_taint_range(const RANGE_START start, const RANGE_LENGTH length, const Source& origin) { diff --git a/ddtrace/appsec/_iast/_taint_tracking/Initializer/Initializer.h b/ddtrace/appsec/_iast/_taint_tracking/Initializer/Initializer.h index d453ce5527f..6d7fbf36499 100644 --- a/ddtrace/appsec/_iast/_taint_tracking/Initializer/Initializer.h +++ b/ddtrace/appsec/_iast/_taint_tracking/Initializer/Initializer.h @@ -140,8 +140,6 @@ class Initializer */ TaintedObjectPtr allocate_tainted_object_copy(const TaintedObjectPtr& from); - void release_tainted_object(TaintedObjectPtr tobj); - // FIXME: these should be static functions of TaintRange // IMPORTANT: if the returned object is not assigned to the map, you have // responsibility of calling release_taint_range on it or you'll have a leak. diff --git a/ddtrace/appsec/_iast/_taint_tracking/TaintTracking/TaintRange.cpp b/ddtrace/appsec/_iast/_taint_tracking/TaintTracking/TaintRange.cpp index a15274cd5e0..ed50046cecf 100644 --- a/ddtrace/appsec/_iast/_taint_tracking/TaintTracking/TaintRange.cpp +++ b/ddtrace/appsec/_iast/_taint_tracking/TaintTracking/TaintRange.cpp @@ -181,9 +181,8 @@ set_ranges(PyObject* str, const TaintRangeRefs& ranges, const TaintRangeMapTypeP auto new_tainted_object = initializer->allocate_ranges_into_taint_object(ranges); set_fast_tainted_if_notinterned_unicode(str); - new_tainted_object->incref(); if (it != tx_map->end()) { - it->second.second->decref(); + it->second.second.reset(); it->second = std::make_pair(get_internal_hash(str), new_tainted_object); return true; } @@ -312,7 +311,6 @@ get_tainted_object(PyObject* str, const TaintRangeMapTypePtr& tx_map) } if (get_internal_hash(str) != it->second.first) { - it->second.second->decref(); tx_map->erase(it); return nullptr; } @@ -368,15 +366,13 @@ set_tainted_object(PyObject* str, TaintedObjectPtr tainted_object, const TaintRa // If the tainted object is different, we need to decref the previous one // and incref the new one. But if it's the same object, we can avoid both // operations, since they would be redundant. - it->second.second->decref(); - tainted_object->incref(); + it->second.second.reset(); it->second = std::make_pair(get_internal_hash(str), tainted_object); } // Update the hash, because for bytearrays it could have changed after the extend operation it->second.first = get_internal_hash(str); return; } - tainted_object->incref(); tx_map->insert({ obj_id, std::make_pair(get_internal_hash(str), tainted_object) }); } diff --git a/ddtrace/appsec/_iast/_taint_tracking/TaintTracking/TaintRange.h b/ddtrace/appsec/_iast/_taint_tracking/TaintTracking/TaintRange.h index 6d6e1a9a279..efb817a3359 100644 --- a/ddtrace/appsec/_iast/_taint_tracking/TaintTracking/TaintRange.h +++ b/ddtrace/appsec/_iast/_taint_tracking/TaintTracking/TaintRange.h @@ -17,7 +17,7 @@ namespace py = pybind11; class TaintedObject; // Alias -using TaintedObjectPtr = TaintedObject*; +using TaintedObjectPtr = shared_ptr; #ifdef NDEBUG // Decide wether to use abseil @@ -134,7 +134,7 @@ api_is_unicode_and_not_fast_tainted(const py::handle& str) return is_notinterned_notfasttainted_unicode(str.ptr()); } -TaintedObject* +TaintedObjectPtr get_tainted_object(PyObject* str, const TaintRangeMapTypePtr& tx_taint_map); Py_hash_t diff --git a/ddtrace/appsec/_iast/_taint_tracking/TaintTracking/TaintedObject.cpp b/ddtrace/appsec/_iast/_taint_tracking/TaintTracking/TaintedObject.cpp index 3a5aa2aa277..3b00a5d28e8 100644 --- a/ddtrace/appsec/_iast/_taint_tracking/TaintTracking/TaintedObject.cpp +++ b/ddtrace/appsec/_iast/_taint_tracking/TaintTracking/TaintedObject.cpp @@ -140,34 +140,11 @@ void TaintedObject::reset() { move_ranges_to_stack(); - rc_ = 0; if (initializer) { ranges_.reserve(RANGES_INITIAL_RESERVE); } } -void -TaintedObject::incref() -{ - rc_++; -} - -void -TaintedObject::decref() -{ - if (--rc_ <= 0) { - release(); - } -} - -void -TaintedObject::release() -{ - // If rc_ is negative, there is a bug. - // assert(rc_ == 0); - initializer->release_tainted_object(this); -} - void pyexport_taintedobject(const py::module& m) { diff --git a/ddtrace/appsec/_iast/_taint_tracking/TaintTracking/TaintedObject.h b/ddtrace/appsec/_iast/_taint_tracking/TaintTracking/TaintedObject.h index a4198424a7c..88198fabcea 100644 --- a/ddtrace/appsec/_iast/_taint_tracking/TaintTracking/TaintedObject.h +++ b/ddtrace/appsec/_iast/_taint_tracking/TaintTracking/TaintedObject.h @@ -8,7 +8,6 @@ class TaintedObject private: TaintRangeRefs ranges_; - size_t rc_{}; public: constexpr static int TAINT_RANGE_LIMIT = 100; @@ -36,7 +35,7 @@ class TaintedObject [[nodiscard]] TaintRangeRefs get_ranges_copy() const { return ranges_; } - void add_ranges_shifted(TaintedObject* tainted_object, + void add_ranges_shifted(TaintedObjectPtr tainted_object, RANGE_START offset, RANGE_LENGTH max_length = -1, RANGE_START orig_offset = -1); @@ -53,12 +52,6 @@ class TaintedObject void move_ranges_to_stack(); void reset(); - - void incref(); - - void decref(); - - void release(); }; void diff --git a/ddtrace/appsec/_iast/_taint_tracking/Utils/StringUtils.cpp b/ddtrace/appsec/_iast/_taint_tracking/Utils/StringUtils.cpp index 0fd274925be..2df5ffa5e28 100644 --- a/ddtrace/appsec/_iast/_taint_tracking/Utils/StringUtils.cpp +++ b/ddtrace/appsec/_iast/_taint_tracking/Utils/StringUtils.cpp @@ -112,6 +112,9 @@ new_pyobject_id(PyObject* tainted_object) const auto res = PyObject_CallFunctionObjArgs(bytes_join_ptr.ptr(), val, NULL); Py_XDECREF(val); Py_XDECREF(empty_bytes); + if (res == nullptr) { + return tainted_object; + } return res; } @@ -138,6 +141,9 @@ new_pyobject_id(PyObject* tainted_object) Py_XDECREF(val); Py_XDECREF(empty_bytes); Py_XDECREF(empty_bytearray); + if (res == nullptr) { + return tainted_object; + } return res; } return tainted_object; diff --git a/ddtrace/appsec/_iast/_taint_tracking/Utils/StringUtils.h b/ddtrace/appsec/_iast/_taint_tracking/Utils/StringUtils.h index d0dbf1ddf80..d2da5dcd821 100644 --- a/ddtrace/appsec/_iast/_taint_tracking/Utils/StringUtils.h +++ b/ddtrace/appsec/_iast/_taint_tracking/Utils/StringUtils.h @@ -81,12 +81,22 @@ template bool args_are_text_and_same_type(PyObject* first, PyObject* second, Args... args) { + if (first == nullptr || second == nullptr) { + return false; + } + + const auto type_first = PyObject_Type(first); + const auto type_second = PyObject_Type(second); + // Check if both first and second are valid text types and of the same type - if (first == nullptr || second == nullptr || !is_text(first) || !is_text(second) || - PyObject_Type(first) != PyObject_Type(second)) { + if (!is_text(first) || !is_text(second) || type_first != type_second) { + Py_XDECREF(type_first); + Py_XDECREF(type_second); return false; } + Py_XDECREF(type_first); + Py_XDECREF(type_second); // Recursively check the rest of the arguments return args_are_text_and_same_type(second, args...); } diff --git a/ddtrace/appsec/_iast/_taint_tracking/tests/test_add_aspect.cpp b/ddtrace/appsec/_iast/_taint_tracking/tests/test_add_aspect.cpp new file mode 100644 index 00000000000..85ac7dc3d95 --- /dev/null +++ b/ddtrace/appsec/_iast/_taint_tracking/tests/test_add_aspect.cpp @@ -0,0 +1,79 @@ +#include "Aspects/Helpers.h" + +#include +#include +#include + +using AspectAddCheck = PyEnvWithContext; + +TEST_F(AspectAddCheck, check_api_add_aspect_strings_candidate_text_empty) +{ + PyObject* candidate_text = this->StringToPyObjectStr(""); + PyObject* text_to_add = this->StringToPyObjectStr("def"); + PyObject* args_array[2]; + args_array[0] = candidate_text; + args_array[1] = text_to_add; + auto result = api_add_aspect(nullptr, args_array, 2); + EXPECT_FALSE(has_pyerr()); + + std::string result_string = this->PyObjectStrToString(result); + + EXPECT_EQ(result_string, "def"); + Py_DecRef(candidate_text); + Py_DecRef(text_to_add); + Py_DecRef(result); +} + +TEST_F(AspectAddCheck, check_api_add_aspect_strings_text_to_add_empty) +{ + PyObject* candidate_text = this->StringToPyObjectStr("abc"); + PyObject* text_to_add = this->StringToPyObjectStr(""); + PyObject* args_array[2]; + args_array[0] = candidate_text; + args_array[1] = text_to_add; + auto result = api_add_aspect(nullptr, args_array, 2); + EXPECT_FALSE(has_pyerr()); + + std::string result_string = this->PyObjectStrToString(result); + + EXPECT_EQ(result_string, "abc"); + Py_DecRef(candidate_text); + Py_DecRef(text_to_add); + Py_DecRef(result); +} + +TEST_F(AspectAddCheck, check_api_add_aspect_strings) +{ + PyObject* candidate_text = this->StringToPyObjectStr("abc"); + PyObject* text_to_add = this->StringToPyObjectStr("def"); + PyObject* args_array[2]; + args_array[0] = candidate_text; + args_array[1] = text_to_add; + auto result = api_add_aspect(nullptr, args_array, 2); + EXPECT_FALSE(has_pyerr()); + + std::string result_string = this->PyObjectStrToString(result); + + EXPECT_EQ(result_string, "abcdef"); + Py_DecRef(candidate_text); + Py_DecRef(text_to_add); + Py_DecRef(result); +} + +TEST_F(AspectAddCheck, check_api_add_aspect_bytes) +{ + PyObject* candidate_text = this->StringToPyObjectBytes("abc"); + PyObject* text_to_add = this->StringToPyObjectBytes("def"); + PyObject* args_array[2]; + args_array[0] = candidate_text; + args_array[1] = text_to_add; + auto result = api_add_aspect(nullptr, args_array, 2); + EXPECT_FALSE(has_pyerr()); + + std::string result_string = this->PyObjectBytesToString(result); + + EXPECT_EQ(result_string, "abcdef"); + Py_DecRef(candidate_text); + Py_DecRef(text_to_add); + Py_DecRef(result); +} diff --git a/ddtrace/appsec/_iast/_taint_tracking/tests/test_common.hpp b/ddtrace/appsec/_iast/_taint_tracking/tests/test_common.hpp index 81af3566b5a..b7338e3d683 100644 --- a/ddtrace/appsec/_iast/_taint_tracking/tests/test_common.hpp +++ b/ddtrace/appsec/_iast/_taint_tracking/tests/test_common.hpp @@ -29,4 +29,22 @@ class PyEnvWithContext : public ::testing::Test initializer.reset(); py::finalize_interpreter(); } + + public: + PyObject* StringToPyObjectStr(const string& ob) { return PyUnicode_FromString(ob.c_str()); } + string PyObjectStrToString(PyObject* ob) + { + PyObject* utf8_str = PyUnicode_AsEncodedString(ob, "utf-8", "strict"); + const char* res_data = PyBytes_AsString(utf8_str); + std::string res_string(res_data); + Py_DecRef(utf8_str); + return res_string; + } + PyObject* StringToPyObjectBytes(const string& ob) { return PyBytes_FromString(ob.c_str()); } + string PyObjectBytesToString(PyObject* ob) + { + const char* res_data = PyBytes_AsString(ob); + std::string res_string(res_data); + return res_string; + } }; diff --git a/ddtrace/appsec/_iast/_taint_tracking/tests/test_helpers.cpp b/ddtrace/appsec/_iast/_taint_tracking/tests/test_helpers.cpp index 1078e23a1e0..825a03ba787 100644 --- a/ddtrace/appsec/_iast/_taint_tracking/tests/test_helpers.cpp +++ b/ddtrace/appsec/_iast/_taint_tracking/tests/test_helpers.cpp @@ -1,5 +1,4 @@ #include -#include #include #include diff --git a/ddtrace/appsec/_iast/_taint_tracking/tests/test_index_aspect.cpp b/ddtrace/appsec/_iast/_taint_tracking/tests/test_index_aspect.cpp index 444325d8fd4..2961cc80ea8 100644 --- a/ddtrace/appsec/_iast/_taint_tracking/tests/test_index_aspect.cpp +++ b/ddtrace/appsec/_iast/_taint_tracking/tests/test_index_aspect.cpp @@ -1,7 +1,6 @@ #include "Aspects/Helpers.h" #include -#include #include using AspectIndexCheck = PyEnvWithContext; diff --git a/ddtrace/appsec/_iast/_taint_tracking/tests/test_other.cpp b/ddtrace/appsec/_iast/_taint_tracking/tests/test_other.cpp index 940e52222b9..0a3ccf063bb 100644 --- a/ddtrace/appsec/_iast/_taint_tracking/tests/test_other.cpp +++ b/ddtrace/appsec/_iast/_taint_tracking/tests/test_other.cpp @@ -1,6 +1,5 @@ #include #include -#include // jjj #include using PyByteArray_Check = PyEnvCheck; diff --git a/scripts/iast/mod_leak_functions.py b/scripts/iast/mod_leak_functions.py index e43db154dbf..e55480a1d36 100644 --- a/scripts/iast/mod_leak_functions.py +++ b/scripts/iast/mod_leak_functions.py @@ -24,7 +24,9 @@ def test_doit(): string3 = string1 + string2 # 2 propagation ranges: hiroot1234 string4 = "-".join([string3, string3, string3]) # 6 propagation ranges: hiroot1234-hiroot1234-hiroot1234 - string5 = string4[0:20] # 1 propagation range: hiroot1234-hiroot123 + string4_2 = string1 + string4_2 += " " + " ".join(string_ for string_ in [string4, string4, string4]) + string5 = string4_2[0:20] # 1 propagation range: hiroot1234-hiroot123 string6 = string5.title() # 1 propagation range: Hiroot1234-Hiroot123 string7 = string6.upper() # 1 propagation range: HIROOT1234-HIROOT123 string8 = "%s_notainted" % string7 # 1 propagation range: HIROOT1234-HIROOT123_notainted From 6ae8477f0317c26aca50cbdca3e8288af9cebd08 Mon Sep 17 00:00:00 2001 From: Taegyun Kim Date: Mon, 7 Oct 2024 06:56:04 -0400 Subject: [PATCH 3/3] chore(ci): parallelize unittest github actions (#10954) 1. Use matrix in actions: No need to specify archs as GitHub actions runners only support specific archs for given OSes and we've only been running ubuntu-latest (x86_64), windows-latest (x86_64) and macos-latest (arm64). 2. Pass python version to hatch run. ## Checklist - [x] 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](https://ddtrace.readthedocs.io/en/stable/releasenotes.html) - The change includes or references documentation updates if necessary - Backport labels are set (if [applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)) ## 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](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces) 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](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting) --- .github/workflows/unit_tests.yml | 19 ++++--------------- 1 file changed, 4 insertions(+), 15 deletions(-) diff --git a/.github/workflows/unit_tests.yml b/.github/workflows/unit_tests.yml index b3d1981a5ba..4abce66d343 100644 --- a/.github/workflows/unit_tests.yml +++ b/.github/workflows/unit_tests.yml @@ -14,26 +14,15 @@ jobs: strategy: fail-fast: false matrix: - include: - - os: ubuntu-latest - archs: x86_64 i686 - #- os: arm-4core-linux - # archs: aarch64 - - os: windows-latest - archs: AMD64 x86 - - os: macos-latest - archs: arm64 + os: [ubuntu-latest, windows-latest, macos-latest] + # Keep this in sync with hatch.toml + python-version: ["3.7", "3.10", "3.12"] steps: - uses: actions/checkout@v4 # Include all history and tags with: fetch-depth: 0 - - uses: actions/setup-python@v5 - name: Install Python - with: - python-version: '3.12' - - uses: actions-rust-lang/setup-rust-toolchain@v1 - name: Install latest stable toolchain and rustfmt run: rustup update stable && rustup default stable && rustup component add rustfmt clippy @@ -44,4 +33,4 @@ jobs: version: "1.12.0" - name: Run tests - run: hatch run ddtrace_unit_tests:test + run: hatch run +py=${{ matrix.python-version }} ddtrace_unit_tests:test