From e5fc1b0b001865d7f1ccd0e28e61c79531f97ff8 Mon Sep 17 00:00:00 2001 From: yoff Date: Fri, 4 Apr 2025 15:11:11 +0200 Subject: [PATCH 1/5] ruby: add qhelp to `rb/useless-assignment-to-local` --- .../queries/variables/DeadStoreOfLocal.qhelp | 49 +++++++++++++++++++ .../variables/examples/DeadStoreOfLocal.rb | 5 ++ .../examples/DeadStoreOfLocalGood.rb | 9 ++++ 3 files changed, 63 insertions(+) create mode 100644 ruby/ql/src/queries/variables/DeadStoreOfLocal.qhelp create mode 100644 ruby/ql/src/queries/variables/examples/DeadStoreOfLocal.rb create mode 100644 ruby/ql/src/queries/variables/examples/DeadStoreOfLocalGood.rb diff --git a/ruby/ql/src/queries/variables/DeadStoreOfLocal.qhelp b/ruby/ql/src/queries/variables/DeadStoreOfLocal.qhelp new file mode 100644 index 000000000000..b4e32eb1708f --- /dev/null +++ b/ruby/ql/src/queries/variables/DeadStoreOfLocal.qhelp @@ -0,0 +1,49 @@ + + + +

+A value is assigned to a local variable, but either that variable is never read +later on, or its value is always overwritten before being read. This means +that the original assignment has no effect, and could indicate a logic error or +incomplete code. +

+ + + + +

+Ensure that you check the control and data flow in the method carefully. +If a value is really not needed, consider omitting the assignment. Be careful, +though: if the right-hand side has a side-effect (like performing a method call), +it is important to keep this to preserve the overall behavior. +

+ + + + +

+In the following example, the return value of the call to send on line 2 +is assigned to the local variable result, but then never used. +

+ + + +

+Assuming that send returns a status code indicating whether the operation +succeeded or not, the value of result should be checked, perhaps like this: +

+ + + + + + + +
  • Wikipedia: Dead store.
    • + + + +     +     diff --git a/ruby/ql/src/queries/variables/examples/DeadStoreOfLocal.rb b/ruby/ql/src/queries/variables/examples/DeadStoreOfLocal.rb new file mode 100644 index 000000000000..0b5dc3d6c2ee --- /dev/null +++ b/ruby/ql/src/queries/variables/examples/DeadStoreOfLocal.rb @@ -0,0 +1,5 @@ +def f(x) + result = send(x) + waitForResponse + return getResponse +end \ No newline at end of file diff --git a/ruby/ql/src/queries/variables/examples/DeadStoreOfLocalGood.rb b/ruby/ql/src/queries/variables/examples/DeadStoreOfLocalGood.rb new file mode 100644 index 000000000000..fc40a7fc4ebd --- /dev/null +++ b/ruby/ql/src/queries/variables/examples/DeadStoreOfLocalGood.rb @@ -0,0 +1,9 @@ +def f(x) + result = send(x) + # check for error + if (result == -1) + raise "Unable to send, check network." + end + waitForResponse + return getResponse +end \ No newline at end of file From b205fedef47c9402bb9df7cd932609e1195a6f60 Mon Sep 17 00:00:00 2001 From: yoff Date: Fri, 4 Apr 2025 16:16:26 +0200 Subject: [PATCH 2/5] ruby: add tests --- .../DeadStoreOfLocal.expected | 4 +++ .../DeadStoreOfLocal/DeadStoreOfLocal.qlref | 2 ++ .../DeadStoreOfLocal/DeadStoreOfLocal.rb | 36 +++++++++++++++++++ .../DeadStoreOfLocal/TestTemplate.rb | 10 ++++++ 4 files changed, 52 insertions(+) create mode 100644 ruby/ql/test/query-tests/variables/DeadStoreOfLocal/DeadStoreOfLocal.expected create mode 100644 ruby/ql/test/query-tests/variables/DeadStoreOfLocal/DeadStoreOfLocal.qlref create mode 100644 ruby/ql/test/query-tests/variables/DeadStoreOfLocal/DeadStoreOfLocal.rb create mode 100644 ruby/ql/test/query-tests/variables/DeadStoreOfLocal/TestTemplate.rb diff --git a/ruby/ql/test/query-tests/variables/DeadStoreOfLocal/DeadStoreOfLocal.expected b/ruby/ql/test/query-tests/variables/DeadStoreOfLocal/DeadStoreOfLocal.expected new file mode 100644 index 000000000000..9b903f762f4f --- /dev/null +++ b/ruby/ql/test/query-tests/variables/DeadStoreOfLocal/DeadStoreOfLocal.expected @@ -0,0 +1,4 @@ +| DeadStoreOfLocal.rb:2:5:2:5 | y | This assignment to $@ is useless, since its value is never read. | DeadStoreOfLocal.rb:2:5:2:5 | y | y | +| DeadStoreOfLocal.rb:14:9:14:9 | x | This assignment to $@ is useless, since its value is never read. | DeadStoreOfLocal.rb:8:5:8:5 | x | x | +| DeadStoreOfLocal.rb:21:5:21:5 | x | This assignment to $@ is useless, since its value is never read. | DeadStoreOfLocal.rb:21:5:21:5 | x | x | +| TestTemplate.rb:9:1:9:1 | x | This assignment to $@ is useless, since its value is never read. | TestTemplate.rb:9:1:9:1 | x | x | diff --git a/ruby/ql/test/query-tests/variables/DeadStoreOfLocal/DeadStoreOfLocal.qlref b/ruby/ql/test/query-tests/variables/DeadStoreOfLocal/DeadStoreOfLocal.qlref new file mode 100644 index 000000000000..222bba3622b5 --- /dev/null +++ b/ruby/ql/test/query-tests/variables/DeadStoreOfLocal/DeadStoreOfLocal.qlref @@ -0,0 +1,2 @@ +query: queries/variables/DeadStoreOfLocal.ql +postprocess: utils/test/InlineExpectationsTestQuery.ql \ No newline at end of file diff --git a/ruby/ql/test/query-tests/variables/DeadStoreOfLocal/DeadStoreOfLocal.rb b/ruby/ql/test/query-tests/variables/DeadStoreOfLocal/DeadStoreOfLocal.rb new file mode 100644 index 000000000000..9d7b8a95fb47 --- /dev/null +++ b/ruby/ql/test/query-tests/variables/DeadStoreOfLocal/DeadStoreOfLocal.rb @@ -0,0 +1,36 @@ +def test_basic(x) + y = x #$ Alert + y = x + 2 + return y +end + +def test_retry + x = 0 + begin + if x == 0 + raise "error" + end + rescue + x = 2 #$ SPURIOUS: Alert + retry + end + return 42 +end + +def test_binding + x = 4 #$ SPURIOUS: Alert + return binding +end + +class Sup + def m(x) + print(x + 1) + end +end + +class Sub < Sup + def m(y) + y = 3 # OK - the call to `super` sees the value of y + super + end +end \ No newline at end of file diff --git a/ruby/ql/test/query-tests/variables/DeadStoreOfLocal/TestTemplate.rb b/ruby/ql/test/query-tests/variables/DeadStoreOfLocal/TestTemplate.rb new file mode 100644 index 000000000000..09aa20d3a8b7 --- /dev/null +++ b/ruby/ql/test/query-tests/variables/DeadStoreOfLocal/TestTemplate.rb @@ -0,0 +1,10 @@ +# -*- coding: utf-8 -*- +require 'erb' + +template = ERB.new < + \_\_ENCODING\_\_ is <%= \_\_ENCODING\_\_ %>. + x is <%= x %>. +EOF +x = 5 #$ SPURIOUS: Alert +puts template.result \ No newline at end of file From 385598d46dd7324ac059d61f0f7e318fea3d77ab Mon Sep 17 00:00:00 2001 From: yoff Date: Mon, 31 Mar 2025 14:39:02 +0200 Subject: [PATCH 3/5] ruby: remove some FPs from `rb/useless-assignment-to-local` --- ruby/ql/src/queries/variables/DeadStoreOfLocal.ql | 11 ++++++++++- .../DeadStoreOfLocal/DeadStoreOfLocal.expected | 3 --- .../variables/DeadStoreOfLocal/DeadStoreOfLocal.rb | 6 +++--- .../variables/DeadStoreOfLocal/TestTemplate.rb | 2 +- 4 files changed, 14 insertions(+), 8 deletions(-) diff --git a/ruby/ql/src/queries/variables/DeadStoreOfLocal.ql b/ruby/ql/src/queries/variables/DeadStoreOfLocal.ql index 2cf36bdc3e50..eb2fb4a7b5a0 100644 --- a/ruby/ql/src/queries/variables/DeadStoreOfLocal.ql +++ b/ruby/ql/src/queries/variables/DeadStoreOfLocal.ql @@ -12,11 +12,20 @@ import codeql.ruby.AST import codeql.ruby.dataflow.SSA +import codeql.ruby.ApiGraphs class RelevantLocalVariableWriteAccess extends LocalVariableWriteAccess { RelevantLocalVariableWriteAccess() { not this.getVariable().getName().charAt(0) = "_" and - not this = any(Parameter p).getAVariable().getDefiningAccess() + not this = any(Parameter p).getAVariable().getDefiningAccess() and + not API::getTopLevelMember("ERB").getInstance().getAMethodCall("result").asExpr().getScope() = + this.getCfgScope() and + not exists(RetryStmt r | r.getCfgScope() = this.getCfgScope()) and + not exists(MethodCall c | + c.getReceiver() instanceof SelfVariableAccess and + c.getMethodName() = "binding" and + c.getCfgScope() = this.getCfgScope() + ) } } diff --git a/ruby/ql/test/query-tests/variables/DeadStoreOfLocal/DeadStoreOfLocal.expected b/ruby/ql/test/query-tests/variables/DeadStoreOfLocal/DeadStoreOfLocal.expected index 9b903f762f4f..572308ee51e4 100644 --- a/ruby/ql/test/query-tests/variables/DeadStoreOfLocal/DeadStoreOfLocal.expected +++ b/ruby/ql/test/query-tests/variables/DeadStoreOfLocal/DeadStoreOfLocal.expected @@ -1,4 +1 @@ | DeadStoreOfLocal.rb:2:5:2:5 | y | This assignment to $@ is useless, since its value is never read. | DeadStoreOfLocal.rb:2:5:2:5 | y | y | -| DeadStoreOfLocal.rb:14:9:14:9 | x | This assignment to $@ is useless, since its value is never read. | DeadStoreOfLocal.rb:8:5:8:5 | x | x | -| DeadStoreOfLocal.rb:21:5:21:5 | x | This assignment to $@ is useless, since its value is never read. | DeadStoreOfLocal.rb:21:5:21:5 | x | x | -| TestTemplate.rb:9:1:9:1 | x | This assignment to $@ is useless, since its value is never read. | TestTemplate.rb:9:1:9:1 | x | x | diff --git a/ruby/ql/test/query-tests/variables/DeadStoreOfLocal/DeadStoreOfLocal.rb b/ruby/ql/test/query-tests/variables/DeadStoreOfLocal/DeadStoreOfLocal.rb index 9d7b8a95fb47..7f3252a58fdc 100644 --- a/ruby/ql/test/query-tests/variables/DeadStoreOfLocal/DeadStoreOfLocal.rb +++ b/ruby/ql/test/query-tests/variables/DeadStoreOfLocal/DeadStoreOfLocal.rb @@ -11,14 +11,14 @@ def test_retry raise "error" end rescue - x = 2 #$ SPURIOUS: Alert + x = 2 # OK - the retry will allow a later read retry end return 42 end def test_binding - x = 4 #$ SPURIOUS: Alert + x = 4 # OK - the binding collects the value of x return binding end @@ -30,7 +30,7 @@ def m(x) class Sub < Sup def m(y) - y = 3 # OK - the call to `super` sees the value of y + y = 3 # OK - the call to `super` sees the value of `y`` super end end \ No newline at end of file diff --git a/ruby/ql/test/query-tests/variables/DeadStoreOfLocal/TestTemplate.rb b/ruby/ql/test/query-tests/variables/DeadStoreOfLocal/TestTemplate.rb index 09aa20d3a8b7..e13b54d0e038 100644 --- a/ruby/ql/test/query-tests/variables/DeadStoreOfLocal/TestTemplate.rb +++ b/ruby/ql/test/query-tests/variables/DeadStoreOfLocal/TestTemplate.rb @@ -6,5 +6,5 @@ \_\_ENCODING\_\_ is <%= \_\_ENCODING\_\_ %>. x is <%= x %>. EOF -x = 5 #$ SPURIOUS: Alert +x = 5 # OK - the template can see the value of x puts template.result \ No newline at end of file From eb8cbfa287e6648e9fb510b08ae6d18db15a10ed Mon Sep 17 00:00:00 2001 From: yoff Date: Fri, 4 Apr 2025 15:18:00 +0200 Subject: [PATCH 4/5] ruby: add change note --- ruby/ql/src/change-notes/2025-04-04-refine-deadstore.md | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 ruby/ql/src/change-notes/2025-04-04-refine-deadstore.md diff --git a/ruby/ql/src/change-notes/2025-04-04-refine-deadstore.md b/ruby/ql/src/change-notes/2025-04-04-refine-deadstore.md new file mode 100644 index 000000000000..c0bff9adf211 --- /dev/null +++ b/ruby/ql/src/change-notes/2025-04-04-refine-deadstore.md @@ -0,0 +1,4 @@ +--- +category: majorAnalysis +--- +* The query `rb/useless-assignment-to-local` now comes with query help and has been tweaked to produce fewer false positives. From 6a8484f84375c8ba0d7c68bc9f14bcae40c1d919 Mon Sep 17 00:00:00 2001 From: yoff Date: Fri, 4 Apr 2025 15:18:42 +0200 Subject: [PATCH 5/5] ruby: adjust precision of `rb/useless-assignment-to-local` to medium --- ruby/ql/src/queries/variables/DeadStoreOfLocal.ql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ruby/ql/src/queries/variables/DeadStoreOfLocal.ql b/ruby/ql/src/queries/variables/DeadStoreOfLocal.ql index eb2fb4a7b5a0..8717047e9954 100644 --- a/ruby/ql/src/queries/variables/DeadStoreOfLocal.ql +++ b/ruby/ql/src/queries/variables/DeadStoreOfLocal.ql @@ -7,7 +7,7 @@ * @id rb/useless-assignment-to-local * @tags maintainability * external/cwe/cwe-563 - * @precision low + * @precision medium */ import codeql.ruby.AST