From 2dcd7895ec963ce8379bb89b3923922a03b620b9 Mon Sep 17 00:00:00 2001 From: Taus Date: Thu, 27 Mar 2025 15:27:42 +0000 Subject: [PATCH 1/5] Python: Modernise `py/mixed-tuple-returns` Removes the dependence on points-to in favour of an approach based on (local) data-flow. I first tried a version that used type tracking, as this more accurately mimics the behaviour of the old query. However, I soon discovered that there were _many_ false positives in this setup. The main bad pattern I saw was a helper function somewhere deep inside the code that both receives and returns an argument that can be tuples with different sizes and origins. In this case, global flow produces something akin to a cartesian product of "n-tuples that flow into the function" and "m-tuples that flow into the function" where m < n. To combat this, I decided to instead focus on only flow _within_ a given function (and so local data-flow was sufficient). Additionally, another class of false positives I saw was cases where the return type actually witnessed that the function in question could return tuples of varying sizes. In this case it seems reasonable to not flag these instances, since they are already (presumably) being checked by a type checker. More generally, if you've annotated the return type of the function with anything (not just `Tuple[...]`), then there's probably little need to flag it. --- .../src/Functions/ReturnConsistentTupleSizes.ql | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/python/ql/src/Functions/ReturnConsistentTupleSizes.ql b/python/ql/src/Functions/ReturnConsistentTupleSizes.ql index 9046f52cecbd..f0cb83067e0f 100644 --- a/python/ql/src/Functions/ReturnConsistentTupleSizes.ql +++ b/python/ql/src/Functions/ReturnConsistentTupleSizes.ql @@ -4,6 +4,7 @@ * @kind problem * @tags reliability * maintainability + * quality * @problem.severity recommendation * @sub-severity high * @precision high @@ -11,13 +12,15 @@ */ import python +import semmle.python.ApiGraphs -predicate returns_tuple_of_size(Function func, int size, AstNode origin) { - exists(Return return, TupleValue val | +predicate returns_tuple_of_size(Function func, int size, Tuple tuple) { + exists(Return return, DataFlow::Node value | + value.asExpr() = return.getValue() and return.getScope() = func and - return.getValue().pointsTo(val, origin) + any(DataFlow::LocalSourceNode n | n.asExpr() = tuple).flowsTo(value) | - size = val.length() + size = count(int n | exists(tuple.getElt(n))) ) } @@ -25,6 +28,8 @@ from Function func, int s1, int s2, AstNode t1, AstNode t2 where returns_tuple_of_size(func, s1, t1) and returns_tuple_of_size(func, s2, t2) and - s1 < s2 + s1 < s2 and + // Don't report on functions that have a return type annotation + not exists(func.getDefinition().(FunctionExpr).getReturns()) select func, func.getQualifiedName() + " returns $@ and $@.", t1, "tuple of size " + s1, t2, "tuple of size " + s2 From f601f4ad9ba2a5d56365ef7692b7f7b6bb8e1ee9 Mon Sep 17 00:00:00 2001 From: Taus Date: Thu, 27 Mar 2025 15:31:28 +0000 Subject: [PATCH 2/5] Python: Update test expectations As we're no longer tracking tuples across function boundaries, we lose the result that related to this setup (which, as the preceding commit explains, lead to a lot of false positives). --- .../Functions/return_values/ReturnConsistentTupleSizes.expected | 1 - 1 file changed, 1 deletion(-) diff --git a/python/ql/test/query-tests/Functions/return_values/ReturnConsistentTupleSizes.expected b/python/ql/test/query-tests/Functions/return_values/ReturnConsistentTupleSizes.expected index fd4f1ee2dd70..2733ae8c26ac 100644 --- a/python/ql/test/query-tests/Functions/return_values/ReturnConsistentTupleSizes.expected +++ b/python/ql/test/query-tests/Functions/return_values/ReturnConsistentTupleSizes.expected @@ -1,2 +1 @@ | functions_test.py:306:1:306:39 | Function returning_different_tuple_sizes | returning_different_tuple_sizes returns $@ and $@. | functions_test.py:308:16:308:18 | Tuple | tuple of size 2 | functions_test.py:310:16:310:20 | Tuple | tuple of size 3 | -| functions_test.py:324:1:324:50 | Function indirectly_returning_different_tuple_sizes | indirectly_returning_different_tuple_sizes returns $@ and $@. | functions_test.py:319:12:319:14 | Tuple | tuple of size 2 | functions_test.py:322:12:322:16 | Tuple | tuple of size 3 | From 980c7d83dac91f6dae11d352a5603a45d23b52ca Mon Sep 17 00:00:00 2001 From: Taus Date: Thu, 27 Mar 2025 15:33:00 +0000 Subject: [PATCH 3/5] Python: Add change note --- .../2025-03-27-modernize-mixed-tuple-returns-query.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 python/ql/src/change-notes/2025-03-27-modernize-mixed-tuple-returns-query.md diff --git a/python/ql/src/change-notes/2025-03-27-modernize-mixed-tuple-returns-query.md b/python/ql/src/change-notes/2025-03-27-modernize-mixed-tuple-returns-query.md new file mode 100644 index 000000000000..9f527b6b5a3e --- /dev/null +++ b/python/ql/src/change-notes/2025-03-27-modernize-mixed-tuple-returns-query.md @@ -0,0 +1,5 @@ +--- +category: minorAnalysis +--- + +- The `py/mixed-tuple-returns` query no longer flags instances where the tuple is passed into the function as an argument, as this lead to too many false positives. From 68668b8e224a95591342610812b07e1f6489b113 Mon Sep 17 00:00:00 2001 From: Taus Date: Thu, 27 Mar 2025 23:23:29 +0100 Subject: [PATCH 4/5] Python: Fix grammar in change note --- .../2025-03-27-modernize-mixed-tuple-returns-query.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/python/ql/src/change-notes/2025-03-27-modernize-mixed-tuple-returns-query.md b/python/ql/src/change-notes/2025-03-27-modernize-mixed-tuple-returns-query.md index 9f527b6b5a3e..57cf5c69a139 100644 --- a/python/ql/src/change-notes/2025-03-27-modernize-mixed-tuple-returns-query.md +++ b/python/ql/src/change-notes/2025-03-27-modernize-mixed-tuple-returns-query.md @@ -2,4 +2,4 @@ category: minorAnalysis --- -- The `py/mixed-tuple-returns` query no longer flags instances where the tuple is passed into the function as an argument, as this lead to too many false positives. +- The `py/mixed-tuple-returns` query no longer flags instances where the tuple is passed into the function as an argument, as this led to too many false positives. From 6674288fd2bdb9425e3e64ae4eb83711b096b6c8 Mon Sep 17 00:00:00 2001 From: Taus Date: Fri, 28 Mar 2025 15:12:39 +0000 Subject: [PATCH 5/5] Python: Update test cases Adds a comment explaining why we no longer flag the indirect tuple example. Also adds a test case which _would_ be flagged if not for the type annotation. --- .../query-tests/Functions/return_values/functions_test.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/python/ql/test/query-tests/Functions/return_values/functions_test.py b/python/ql/test/query-tests/Functions/return_values/functions_test.py index 24b1943feeb5..9f72a7fec600 100644 --- a/python/ql/test/query-tests/Functions/return_values/functions_test.py +++ b/python/ql/test/query-tests/Functions/return_values/functions_test.py @@ -321,7 +321,7 @@ def function_returning_2_tuple(): def function_returning_3_tuple(): return 1,2,3 -def indirectly_returning_different_tuple_sizes(x): +def indirectly_returning_different_tuple_sizes(x): # OK, since we only look at local tuple returns if x: return function_returning_2_tuple() else: @@ -347,3 +347,9 @@ def ok_match2(x): # FP return 0 case _: return 1 + +def ok_tuple_returns_captured_in_type(x: bool) -> tuple[int, ...]: # OK because there is a type annotation present + if x: + return 1, 2 + else: + return 1, 2, 3