From: Yichun Zhang (agentzh) <yichun@openresty.com>
Date: Tue, 31 Oct 2023 20:22:48 +0000 (-0700)
Subject: PR31018: Map operations might get no lock protections due to "pushdown lock" bugs.
X-Git-Tag: release-5.0a~14
X-Git-Url: https://sourceware.org/git/?a=commitdiff_plain;h=1c547184ca3a6d1e54d00898157c6bb29f682dcc;p=systemtap.git

PR31018: Map operations might get no lock protections due to "pushdown lock" bugs.

The stap translator's "pushdown lock" optimization is buggy in that it may not
emit locking code for all the code branches in the first statement needing a
lock, making subsequent statements actually needing a lock go without a lock.

The correct way is to always emit locking code for statements that might
"push down" the locks, until seeing a simple statements what don't
(like an assignment statement using "<<<").

I forgot to add that it might not necessarily be triggered by a leading "if"
statement needing a lock. Other statements that "push down" locks may
also trigger it, like nested code blocks with "if" statements.
---

diff --git a/staptree.h b/staptree.h
index 5826fbaef..4596b3661 100644
--- a/staptree.h
+++ b/staptree.h
@@ -713,6 +713,7 @@ struct statement : public visitable
   statement ();
   statement (const token* tok);
   virtual ~statement ();
+  virtual bool might_pushdown_lock () = 0;
 };
 
 std::ostream& operator << (std::ostream& o, const statement& k);
@@ -729,6 +730,7 @@ struct embeddedcode: public statement
   bool tagged_p (const interned_string& tag) const;
   void print (std::ostream& o) const;
   void visit (visitor* u);
+  virtual bool might_pushdown_lock () { return false; };
 };
 
 
@@ -741,6 +743,7 @@ struct block: public statement
   block (statement* car, statement* cdr);
   block (statement* car, statement* cdr1, statement* cdr2);
   virtual ~block () {}
+  virtual bool might_pushdown_lock () { return true; };
 };
 
 
@@ -751,6 +754,9 @@ struct try_block: public statement
   symbol* catch_error_var; // may be 0
   void print (std::ostream& o) const;
   void visit (visitor* u);
+
+  // PR26296: for try/catch, don't try to push lock/unlock down
+  virtual bool might_pushdown_lock () { return false; };
 };
 
 
@@ -763,6 +769,10 @@ struct for_loop: public statement
   statement* block;
   void print (std::ostream& o) const;
   void visit (visitor* u);
+
+  // PR26269 lockpushdown:
+  // for loops, forget optimizing, just emit locks at top & bottom
+  virtual bool might_pushdown_lock () { return false; };
 };
 
 
@@ -781,6 +791,10 @@ struct foreach_loop: public statement
   statement* block;
   void print (std::ostream& o) const;
   void visit (visitor* u);
+
+  // PR26269 lockpushdown:
+  // for loops, forget optimizing, just emit locks at top & bottom
+  virtual bool might_pushdown_lock () { return false; };
 };
 
 
@@ -789,6 +803,7 @@ struct null_statement: public statement
   void print (std::ostream& o) const;
   void visit (visitor* u);
   null_statement (const token* tok);
+  virtual bool might_pushdown_lock () { return false; };
 };
 
 
@@ -797,6 +812,7 @@ struct expr_statement: public statement
   expression* value;  // executed for side-effects
   void print (std::ostream& o) const;
   void visit (visitor* u);
+  virtual bool might_pushdown_lock () { return false; };
 };
 
 
@@ -807,6 +823,7 @@ struct if_statement: public statement
   statement* elseblock; // may be 0
   void print (std::ostream& o) const;
   void visit (visitor* u);
+  virtual bool might_pushdown_lock () { return true; };
 };
 
 
@@ -814,6 +831,10 @@ struct return_statement: public expr_statement
 {
   void print (std::ostream& o) const;
   void visit (visitor* u);
+
+  // PR26296: We should not encounter a RETURN statement in a
+  // lock-relevant section of code (a probe handler body) at all.
+  virtual bool might_pushdown_lock () { return false; };
 };
 
 
@@ -821,6 +842,7 @@ struct delete_statement: public expr_statement
 {
   void print (std::ostream& o) const;
   void visit (visitor* u);
+  virtual bool might_pushdown_lock () { return false; };
 };
 
 
@@ -828,6 +850,7 @@ struct break_statement: public statement
 {
   void print (std::ostream& o) const;
   void visit (visitor* u);
+  virtual bool might_pushdown_lock () { return false; };
 };
 
 
@@ -835,6 +858,7 @@ struct continue_statement: public statement
 {
   void print (std::ostream& o) const;
   void visit (visitor* u);
+  virtual bool might_pushdown_lock () { return false; };
 };
 
 
@@ -842,6 +866,7 @@ struct next_statement: public statement
 {
   void print (std::ostream& o) const;
   void visit (visitor* u);
+  virtual bool might_pushdown_lock () { return false; };
 };
 
 
diff --git a/testsuite/systemtap.base/lock-pushdown-bugs.exp b/testsuite/systemtap.base/lock-pushdown-bugs.exp
new file mode 100644
index 000000000..38067c0fc
--- /dev/null
+++ b/testsuite/systemtap.base/lock-pushdown-bugs.exp
@@ -0,0 +1,130 @@
+set test "lock-pushdown-bugs"
+set testpath "$srcdir/$subdir"
+
+if {! [installtest_p]} { untested $test; return }
+if {! [uretprobes_p]} { untested $test; return }
+
+# --- TEST 1 ---
+
+set subtest1 "TEST 1: PR31018: if stmt needing locks before a map op stmt (check at runtime)"
+
+set res [target_compile ${testpath}/${test}_1.c ./a.out executable \
+    "additional_flags=-O additional_flags=-g additional_flags=-Wno-unused-parameter"]
+if {$res ne ""} {
+    verbose "target_compile failed: $res" 2
+    fail "$test: $subtest1: unable to compile ${test}_1.c"
+} else {
+    foreach runtime [get_runtime_list] {
+        if {$runtime eq ""} {
+            set runtime "kernel"
+        }
+        set test_name "$test: $subtest1 ($runtime)"
+        set cmd "stap -g --runtime=$runtime -c ./a.out '$srcdir/$subdir/${test}_1.stp'"
+        set exit_code [run_cmd_2way $cmd out stderr]
+        set exp_out "ok\n"
+        is "${test_name}: stdout" $out $exp_out
+        set exp_stderr ""
+        is "${test_name}: stderr" $stderr $exp_stderr
+        is "${test_name}: exit code" $exit_code 0
+    }
+}
+
+# --- TEST 2 ---
+
+set subtest2 "TEST 2: PR31018: if stmt needing locks before a map op stmt (check at runtime, if entered)"
+
+set res [target_compile ${testpath}/${test}_1.c ./a.out executable \
+    "additional_flags=-O additional_flags=-g additional_flags=-Wno-unused-parameter"]
+if {$res ne ""} {
+    verbose "target_compile failed: $res" 2
+    fail "$test: $subtest2: unable to compile ${test}_1.c"
+} else {
+    foreach runtime [get_runtime_list] {
+        if {$runtime eq ""} {
+            set runtime "kernel"
+        }
+        set test_name "$test: $subtest2 ($runtime)"
+        set cmd "stap -g --runtime=$runtime -c ./a.out '$srcdir/$subdir/${test}_2.stp'"
+        set exit_code [run_cmd_2way $cmd out stderr]
+        set exp_out "ok\n"
+        is "${test_name}: stdout" $out $exp_out
+        set exp_stderr ""
+        is "${test_name}: stderr" $stderr $exp_stderr
+        is "${test_name}: exit code" $exit_code 0
+    }
+}
+
+# --- TEST 3 ---
+
+set subtest3 "TEST 3: PR31018: nested block stmt needing locks before a map op stmt (check at runtime)"
+
+set res [target_compile ${testpath}/${test}_1.c ./a.out executable \
+    "additional_flags=-O additional_flags=-g additional_flags=-Wno-unused-parameter"]
+if {$res ne ""} {
+    verbose "target_compile failed: $res" 2
+    fail "$test: $subtest3: unable to compile ${test}_1.c"
+} else {
+    foreach runtime [get_runtime_list] {
+        if {$runtime eq ""} {
+            set runtime "kernel"
+        }
+        set test_name "$test: $subtest3 ($runtime)"
+        set cmd "stap -g --runtime=$runtime -c ./a.out '$srcdir/$subdir/${test}_3.stp'"
+        set exit_code [run_cmd_2way $cmd out stderr]
+        set exp_out "ok\n"
+        is "${test_name}: stdout" $out $exp_out
+        set exp_stderr ""
+        is "${test_name}: stderr" $stderr $exp_stderr
+        is "${test_name}: exit code" $exit_code 0
+    }
+}
+
+# --- TEST 4 ---
+
+set subtest4 "TEST 4: PR31018: nested block stmt needing locks before a map op stmt (check at runtime, if taken)"
+
+set res [target_compile ${testpath}/${test}_1.c ./a.out executable \
+    "additional_flags=-O additional_flags=-g additional_flags=-Wno-unused-parameter"]
+if {$res ne ""} {
+    verbose "target_compile failed: $res" 2
+    fail "$test: $subtest4: unable to compile ${test}_1.c"
+} else {
+    foreach runtime [get_runtime_list] {
+        if {$runtime eq ""} {
+            set runtime "kernel"
+        }
+        set test_name "$test: $subtest4 ($runtime)"
+        set cmd "stap -g --runtime=$runtime -c ./a.out '$srcdir/$subdir/${test}_4.stp'"
+        set exit_code [run_cmd_2way $cmd out stderr]
+        set exp_out "ok\n"
+        is "${test_name}: stdout" $out $exp_out
+        set exp_stderr ""
+        is "${test_name}: stderr" $stderr $exp_stderr
+        is "${test_name}: exit code" $exit_code 0
+    }
+}
+
+# --- TEST 5 ---
+
+set subtest5 "TEST 5: PR31018: if stmt needing locks before a map op stmt (check at runtime)"
+
+set res [target_compile ${testpath}/${test}_1.c ./a.out executable \
+    "additional_flags=-O additional_flags=-g additional_flags=-Wno-unused-parameter"]
+if {$res ne ""} {
+    verbose "target_compile failed: $res" 2
+    fail "$test: $subtest5: unable to compile ${test}_1.c"
+} else {
+    foreach runtime [get_runtime_list] {
+        if {$runtime eq ""} {
+            set runtime "kernel"
+        }
+        set test_name "$test: $subtest5 ($runtime)"
+        set cmd "stap -g --runtime=$runtime -c ./a.out '$srcdir/$subdir/${test}_5.stp'"
+        set exit_code [run_cmd_2way $cmd out stderr]
+        set exp_out "ok\n"
+        is "${test_name}: stdout" $out $exp_out
+        set exp_stderr ""
+        is "${test_name}: stderr" $stderr $exp_stderr
+        is "${test_name}: exit code" $exit_code 0
+    }
+}
diff --git a/testsuite/systemtap.base/lock-pushdown-bugs_1.c b/testsuite/systemtap.base/lock-pushdown-bugs_1.c
new file mode 100644
index 000000000..b834f07a0
--- /dev/null
+++ b/testsuite/systemtap.base/lock-pushdown-bugs_1.c
@@ -0,0 +1,3 @@
+int main(int argc, char *argv[]) {
+    return argc - 1;
+}
diff --git a/testsuite/systemtap.base/lock-pushdown-bugs_1.stp b/testsuite/systemtap.base/lock-pushdown-bugs_1.stp
new file mode 100644
index 000000000..af4bb8ba8
--- /dev/null
+++ b/testsuite/systemtap.base/lock-pushdown-bugs_1.stp
@@ -0,0 +1,18 @@
+@define assert_lock(v) %( if (%{ c->locked %} != @v) { error(sprintf("locked mismatch, expected %d, actual %d", @v, %{ c->locked %})) } %)
+
+global stats;
+
+probe process.function("main") {
+    my_pid = pid();
+    v = @var("argc");
+    @assert_lock(0);
+    if (v > 3) {
+        delete stats;
+    }
+    @assert_lock(0);
+    stats[my_pid] <<< v;
+    @assert_lock(1);
+    stats[my_pid] <<< v;
+    @assert_lock(2);
+    println("ok");
+}
diff --git a/testsuite/systemtap.base/lock-pushdown-bugs_2.stp b/testsuite/systemtap.base/lock-pushdown-bugs_2.stp
new file mode 100644
index 000000000..46ddff31d
--- /dev/null
+++ b/testsuite/systemtap.base/lock-pushdown-bugs_2.stp
@@ -0,0 +1,19 @@
+@define assert_lock(v) %( if (%{ c->locked %} != @v) { error(sprintf("locked mismatch, expected %d, actual %d", @v, %{ c->locked %})) } %)
+
+global stats;
+
+probe process.function("main") {
+    my_pid = pid();
+    v = @var("argc");
+    @assert_lock(0);
+    if (v == 1) {
+        @assert_lock(0);
+        delete stats;
+    }
+    @assert_lock(1);
+    stats[my_pid] <<< v;
+    @assert_lock(1);
+    stats[my_pid] <<< v;
+    @assert_lock(2);
+    println("ok");
+}
diff --git a/testsuite/systemtap.base/lock-pushdown-bugs_3.stp b/testsuite/systemtap.base/lock-pushdown-bugs_3.stp
new file mode 100644
index 000000000..fe186d002
--- /dev/null
+++ b/testsuite/systemtap.base/lock-pushdown-bugs_3.stp
@@ -0,0 +1,22 @@
+@define assert_lock(v) %( if (%{ c->locked %} != @v) { error(sprintf("locked mismatch, expected %d, actual %d", @v, %{ c->locked %})) } %)
+
+global stats;
+
+probe process.function("main") {
+    my_pid = pid();
+    v = @var("argc");
+    @assert_lock(0);
+    {
+        @assert_lock(0);
+        if (v > 3) {
+            delete stats;
+        }
+    }
+    @assert_lock(0);
+    stats[my_pid] <<< v;
+    @assert_lock(1);
+    stats[my_pid] <<< v;
+    @assert_lock(2);
+    println("ok");
+    @assert_lock(2);
+}
diff --git a/testsuite/systemtap.base/lock-pushdown-bugs_4.stp b/testsuite/systemtap.base/lock-pushdown-bugs_4.stp
new file mode 100644
index 000000000..ee3f4ac2e
--- /dev/null
+++ b/testsuite/systemtap.base/lock-pushdown-bugs_4.stp
@@ -0,0 +1,23 @@
+@define assert_lock(v) %( if (%{ c->locked %} != @v) { error(sprintf("locked mismatch, expected %d, actual %d", @v, %{ c->locked %})) } %)
+
+global stats;
+
+probe process.function("main") {
+    my_pid = pid();
+    v = @var("argc");
+    @assert_lock(0);
+    {
+        @assert_lock(0);
+        if (v == 1) {
+            @assert_lock(0);
+            delete stats;
+        }
+    }
+    @assert_lock(1);
+    stats[my_pid] <<< v;
+    @assert_lock(1);
+    stats[my_pid] <<< v;
+    @assert_lock(2);
+    println("ok");
+    @assert_lock(2);
+}
diff --git a/testsuite/systemtap.base/lock-pushdown-bugs_5.stp b/testsuite/systemtap.base/lock-pushdown-bugs_5.stp
new file mode 100644
index 000000000..38916d035
--- /dev/null
+++ b/testsuite/systemtap.base/lock-pushdown-bugs_5.stp
@@ -0,0 +1,22 @@
+@define assert_lock(v) %( if (%{ c->locked %} != @v) { error(sprintf("locked mismatch, expected %d, actual %d", @v, %{ c->locked %})) } %)
+
+global stats;
+
+probe process.function("main") {
+    my_pid = pid();
+    v = @var("argc");
+    @assert_lock(0);
+    if (v > 3) {
+        delete stats;
+    }
+    @assert_lock(0);
+    if (v > 2) {
+        delete stats;
+    }
+    @assert_lock(0);
+    stats[my_pid] <<< v;
+    @assert_lock(1);
+    stats[my_pid] <<< v;
+    @assert_lock(2);
+    println("ok");
+}
diff --git a/translate.cxx b/translate.cxx
index 525079605..0a3b58a4b 100644
--- a/translate.cxx
+++ b/translate.cxx
@@ -3981,10 +3981,25 @@ c_unparser::visit_block (block *s)
  
       // if needed, find the lock insertion site; instruct it to lock
       if (pushdown_lock_p(s))
-        for (unsigned i=0; i<s->statements.size(); i++)
-          if (locks_needed_p (s->statements[i]))
-            { pushdown_lock.insert(s->statements[i]); pushed_lock_down = true; break; }
-      
+        {
+          for (unsigned i=0; i<s->statements.size(); i++)
+            {
+              struct statement *stmt = s->statements[i];
+              if (locks_needed_p (stmt))
+                {
+                  pushed_lock_down = true;
+                  pushdown_lock.insert (stmt);
+
+                  if (! stmt->might_pushdown_lock ())
+                    {
+                      // now we know the subsquement stmts must have locks
+                      // held, so we don't bother going forward.
+                      break;
+                    }
+                }
+            }
+        }
+
       // if needed, find the unlock insertion site; instruct it to unlock
       if (pushdown_unlock_p(s))
         for (ssize_t i=s->statements.size()-1; i>=0; i--) // NB: traverse backward!