From 73bae1627bd671a8674c53091a7463fe2de2fe66 Mon Sep 17 00:00:00 2001 From: yoff Date: Tue, 13 May 2025 15:08:01 +0200 Subject: [PATCH 1/4] ruby: test for DeadStore and captured variables --- .../DeadStoreOfLocal.expected | 1 + .../DeadStoreOfLocal/DeadStoreOfLocal.rb | 30 +++++++++++++++++++ 2 files changed, 31 insertions(+) diff --git a/ruby/ql/test/query-tests/variables/DeadStoreOfLocal/DeadStoreOfLocal.expected b/ruby/ql/test/query-tests/variables/DeadStoreOfLocal/DeadStoreOfLocal.expected index 572308ee51e..2ec0fe7cc0d 100644 --- a/ruby/ql/test/query-tests/variables/DeadStoreOfLocal/DeadStoreOfLocal.expected +++ b/ruby/ql/test/query-tests/variables/DeadStoreOfLocal/DeadStoreOfLocal.expected @@ -1 +1,2 @@ | 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:61:17:61:17 | x | This assignment to $@ is useless, since its value is never read. | DeadStoreOfLocal.rb:56:17:56:17 | 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 7f3252a58fd..ccd6ba1bdd2 100644 --- a/ruby/ql/test/query-tests/variables/DeadStoreOfLocal/DeadStoreOfLocal.rb +++ b/ruby/ql/test/query-tests/variables/DeadStoreOfLocal/DeadStoreOfLocal.rb @@ -33,4 +33,34 @@ class Sub < Sup y = 3 # OK - the call to `super` sees the value of `y`` super end +end + +def do_twice + yield + yield +end + +def get_done_twice x + do_twice do + print x + x += 1 # OK - the block is executed twice + end +end + +def retry_once + yield +rescue + yield +end + +def get_retried x + retry_once do + print x + if x < 1 + begin + x += 1 #$ SPURIOUS: Alert + raise StandardError + end + end + end end \ No newline at end of file From 774b1820c24499896c03e58a1ef0c35f7e00314c Mon Sep 17 00:00:00 2001 From: yoff Date: Tue, 13 May 2025 15:11:00 +0200 Subject: [PATCH 2/4] ruby: also insert `capturedExitRead`-nodes by exceptional exits --- ruby/ql/lib/codeql/ruby/dataflow/internal/SsaImpl.qll | 1 - .../variables/DeadStoreOfLocal/DeadStoreOfLocal.expected | 1 - .../query-tests/variables/DeadStoreOfLocal/DeadStoreOfLocal.rb | 2 +- 3 files changed, 1 insertion(+), 3 deletions(-) diff --git a/ruby/ql/lib/codeql/ruby/dataflow/internal/SsaImpl.qll b/ruby/ql/lib/codeql/ruby/dataflow/internal/SsaImpl.qll index 3c1da6f3013..b4ba8e3ffad 100644 --- a/ruby/ql/lib/codeql/ruby/dataflow/internal/SsaImpl.qll +++ b/ruby/ql/lib/codeql/ruby/dataflow/internal/SsaImpl.qll @@ -106,7 +106,6 @@ private predicate writesCapturedVariable(Cfg::BasicBlock bb, LocalVariable v) { * at index `i` in exit block `bb`. */ private predicate capturedExitRead(Cfg::AnnotatedExitBasicBlock bb, int i, LocalVariable v) { - bb.isNormal() and writesCapturedVariable(bb.getAPredecessor*(), v) and i = bb.length() } diff --git a/ruby/ql/test/query-tests/variables/DeadStoreOfLocal/DeadStoreOfLocal.expected b/ruby/ql/test/query-tests/variables/DeadStoreOfLocal/DeadStoreOfLocal.expected index 2ec0fe7cc0d..572308ee51e 100644 --- a/ruby/ql/test/query-tests/variables/DeadStoreOfLocal/DeadStoreOfLocal.expected +++ b/ruby/ql/test/query-tests/variables/DeadStoreOfLocal/DeadStoreOfLocal.expected @@ -1,2 +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:61:17:61:17 | x | This assignment to $@ is useless, since its value is never read. | DeadStoreOfLocal.rb:56:17:56:17 | 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 ccd6ba1bdd2..b9ac29f1f25 100644 --- a/ruby/ql/test/query-tests/variables/DeadStoreOfLocal/DeadStoreOfLocal.rb +++ b/ruby/ql/test/query-tests/variables/DeadStoreOfLocal/DeadStoreOfLocal.rb @@ -58,7 +58,7 @@ def get_retried x print x if x < 1 begin - x += 1 #$ SPURIOUS: Alert + x += 1 #$ OK - the block may be executed again raise StandardError end end From c70fd6a58c6ce77391c37d6ded39416381d32c9d Mon Sep 17 00:00:00 2001 From: yoff Date: Tue, 13 May 2025 16:18:33 +0200 Subject: [PATCH 3/4] ruby: add change note --- .../2025-05-13-captured-variables-live-more-often.md | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 ruby/ql/lib/change-notes/2025-05-13-captured-variables-live-more-often.md diff --git a/ruby/ql/lib/change-notes/2025-05-13-captured-variables-live-more-often.md b/ruby/ql/lib/change-notes/2025-05-13-captured-variables-live-more-often.md new file mode 100644 index 00000000000..94ccaefa0f0 --- /dev/null +++ b/ruby/ql/lib/change-notes/2025-05-13-captured-variables-live-more-often.md @@ -0,0 +1,4 @@ +--- +category: minorAnalysis +--- +* Captured variables are currently considered live when the capturing function exists normally. Now they are also considered live when the capturing function exits via an exception. \ No newline at end of file From 3fcd46ec6c5346eed0de4594ace2b9efa1710de3 Mon Sep 17 00:00:00 2001 From: yoff Date: Tue, 13 May 2025 16:57:32 +0200 Subject: [PATCH 4/4] Apply suggestions from code review Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- .../2025-05-13-captured-variables-live-more-often.md | 2 +- .../query-tests/variables/DeadStoreOfLocal/DeadStoreOfLocal.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/ruby/ql/lib/change-notes/2025-05-13-captured-variables-live-more-often.md b/ruby/ql/lib/change-notes/2025-05-13-captured-variables-live-more-often.md index 94ccaefa0f0..3a0878e6553 100644 --- a/ruby/ql/lib/change-notes/2025-05-13-captured-variables-live-more-often.md +++ b/ruby/ql/lib/change-notes/2025-05-13-captured-variables-live-more-often.md @@ -1,4 +1,4 @@ --- category: minorAnalysis --- -* Captured variables are currently considered live when the capturing function exists normally. Now they are also considered live when the capturing function exits via an exception. \ No newline at end of file +* Captured variables are currently considered live when the capturing function exits normally. Now they are also considered live when the capturing function exits via an exception. \ 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 index b9ac29f1f25..ae40573c5c1 100644 --- a/ruby/ql/test/query-tests/variables/DeadStoreOfLocal/DeadStoreOfLocal.rb +++ b/ruby/ql/test/query-tests/variables/DeadStoreOfLocal/DeadStoreOfLocal.rb @@ -58,7 +58,7 @@ def get_retried x print x if x < 1 begin - x += 1 #$ OK - the block may be executed again + x += 1 # OK - the block may be executed again raise StandardError end end