Fix use of a dangling pointer for Python breakpoint objects

Message ID 72fe08f8-6a9f-1c50-ea5a-1ce75f624e6f@adacore.com
State New, archived
Headers

Commit Message

Pierre-Marie de Rodat June 27, 2016, 9:11 a.m. UTC
  Hi Pedro,

On 06/24/2016 06:41 PM, Pedro Alves wrote:
> I think this comment should be adjusted.

Done.

> But I think this would be even better:
>
> # ... and when it did, as a result, the following breakpoint creation
> # (not initiated by the Python API) would dereference the
> # already-freed Python breakpoint wrapper, resulting in undefined
> # behavior, sometimes observed as a gdb crash, and other times causing
> # the next stop to invoke the Python wrapper "stop" method for the
> # object that is not supposed to exist.

Yours is definitely better: done!

> Three things here:
>
> - Please make pass/fail messages here the same.
>
> - With gdb_test_multiple, you also need to match $gdb_prompt,
>   otherwise you confuse the next test.
>
> - No need for leading ".*" in regexes, it's implicit.

Done.

> While at it, how about renaming the new files to avoid
> the meaningless "2"?
>
> Maybe py-breakpoint-create-fail.[py|exp|c] ?

Done as well.

Thank you for your help! I’m not familiar with the testsuite so I had a 
hard time analyzing how other testcases do and investigating why my 
testcase did not work… Anyway, thank you for the rationale! Updated 
patch is attached.
  

Comments

Pedro Alves June 27, 2016, 10:03 a.m. UTC | #1
On 06/27/2016 10:11 AM, Pierre-Marie de Rodat wrote:

> +gdb_test "source py-breakpoint2.py"

This is still sourcing the old filename.

> +
> +# ... and when it did, as a result, the following breakpoint creation (not
> +# initiated by the Python API) would dereference the already-freed Python
> +# breakpoint wrapper, resulting in undefined behavior, sometimes observed as a
> +# gdb crash, and other times causing the next stop to invoke the Python wrapper
> +# "stop" method for the object that is not supposed to exist.
> +gdb_test "break foo"
> +
> +# ... eventually, triggering this breakpoint will invoke the Python wrapper
> +# "stop" method for an object that is not supposed to exist.

Remove this sentence, it no longer makes sense to have it.

> +set test "continuing to foo"
> +gdb_test_multiple "continue" "$test" {
> +    -re "MyBP\.stop was invoked\!.*$gdb_prompt $" {
> +        fail "$test"
> +    }
> +    -re "Continuing.*Breakpoint 2, foo.*$gdb_prompt $" {
> +        pass "$test"
> +    }
> +}

OK with the above fixed.

Thanks,
Pedro Alves
  
Pierre-Marie de Rodat June 27, 2016, 10:13 a.m. UTC | #2
On 06/27/2016 12:03 PM, Pedro Alves wrote:
> On 06/27/2016 10:11 AM, Pierre-Marie de Rodat wrote:
>
>> +gdb_test "source py-breakpoint2.py"
>
> This is still sourcing the old filename.

Blarf, sorry about this! I checked the testcase still fails as expected 
with an old GDB this time.

>> +# ... eventually, triggering this breakpoint will invoke the Python wrapper
>> +# "stop" method for an object that is not supposed to exist.
>
> Remove this sentence, it no longer makes sense to have it.

Done.

> OK with the above fixed.

Thank you! This is pushed, now.
  

Patch

From 8e8bf8cfd8c07b65cdde21066cf4a25b63b3fec5 Mon Sep 17 00:00:00 2001
From: Pierre-Marie de Rodat <derodat@adacore.com>
Date: Tue, 21 Jun 2016 12:32:56 +0200
Subject: [PATCH] Fix use of a dangling pointer for Python breakpoint objects

When a Python script tries to create a breakpoint but fails to do so,
gdb.Breakpoint.__init__ raises an exception and the breakpoint does not
exist anymore in the Python interpreter. However, GDB still keeps a
reference to the Python object to be used for a later hook, which is
wrong.

This commit adds the necessary cleanup code so that there is no stale
reference to this Python object. It also adds a new testcase to
reproduce the bug and check the fix.

2016-06-24  Pierre-Marie de Rodat  <derodat@adacore.com>

gdb/
	* python/py-breakpoint.c (bppy_init): Clear bppy_pending_object
	when there is an error during the breakpoint creation.

gdb/testsuite

	* gdb.python/py-breakpoint-create-fail.c,
	gdb.python/py-breakpoint-create-fail.exp,
	gdb.python/py-breakpoint-create-fail.py: New testcase.
---
 gdb/python/py-breakpoint.c                         |  1 +
 .../gdb.python/py-breakpoint-create-fail.c         | 28 +++++++++++
 .../gdb.python/py-breakpoint-create-fail.exp       | 58 ++++++++++++++++++++++
 .../gdb.python/py-breakpoint-create-fail.py        | 31 ++++++++++++
 4 files changed, 118 insertions(+)
 create mode 100644 gdb/testsuite/gdb.python/py-breakpoint-create-fail.c
 create mode 100644 gdb/testsuite/gdb.python/py-breakpoint-create-fail.exp
 create mode 100644 gdb/testsuite/gdb.python/py-breakpoint-create-fail.py

diff --git a/gdb/python/py-breakpoint.c b/gdb/python/py-breakpoint.c
index ed9cae6..5918bcc 100644
--- a/gdb/python/py-breakpoint.c
+++ b/gdb/python/py-breakpoint.c
@@ -705,6 +705,7 @@  bppy_init (PyObject *self, PyObject *args, PyObject *kwargs)
     }
   CATCH (except, RETURN_MASK_ALL)
     {
+      bppy_pending_object = NULL;
       PyErr_Format (except.reason == RETURN_QUIT
 		    ? PyExc_KeyboardInterrupt : PyExc_RuntimeError,
 		    "%s", except.message);
diff --git a/gdb/testsuite/gdb.python/py-breakpoint-create-fail.c b/gdb/testsuite/gdb.python/py-breakpoint-create-fail.c
new file mode 100644
index 0000000..c346bdd
--- /dev/null
+++ b/gdb/testsuite/gdb.python/py-breakpoint-create-fail.c
@@ -0,0 +1,28 @@ 
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2016 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see  <http://www.gnu.org/licenses/>.  */
+
+int
+foo (int a)
+{
+  return a * 2;
+}
+
+int
+main (void)
+{
+  return foo (2);
+}
diff --git a/gdb/testsuite/gdb.python/py-breakpoint-create-fail.exp b/gdb/testsuite/gdb.python/py-breakpoint-create-fail.exp
new file mode 100644
index 0000000..68cdccd
--- /dev/null
+++ b/gdb/testsuite/gdb.python/py-breakpoint-create-fail.exp
@@ -0,0 +1,58 @@ 
+# Copyright (C) 2016 Free Software Foundation, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# This file is part of the GDB testsuite.  It tests proper handling for
+# breakpoint creation failure.
+
+load_lib gdb-python.exp
+
+standard_testfile
+
+if { [prepare_for_testing ${testfile}.exp ${testfile} ${srcfile}] } {
+    return -1
+}
+
+# Skip all tests if Python scripting is not enabled.
+if { [skip_python_tests] } { continue }
+
+clean_restart "${testfile}"
+if ![runto_main] {
+    perror "could not run to main"
+    continue
+}
+
+# The following will create a breakpoint Python wrapper whose construction will
+# abort: the requested symbol is not defined.  GDB should not keep a reference
+# to the wrapper; however it used to...
+gdb_test "source py-breakpoint2.py"
+
+# ... and when it did, as a result, the following breakpoint creation (not
+# initiated by the Python API) would dereference the already-freed Python
+# breakpoint wrapper, resulting in undefined behavior, sometimes observed as a
+# gdb crash, and other times causing the next stop to invoke the Python wrapper
+# "stop" method for the object that is not supposed to exist.
+gdb_test "break foo"
+
+# ... eventually, triggering this breakpoint will invoke the Python wrapper
+# "stop" method for an object that is not supposed to exist.
+set test "continuing to foo"
+gdb_test_multiple "continue" "$test" {
+    -re "MyBP\.stop was invoked\!.*$gdb_prompt $" {
+        fail "$test"
+    }
+    -re "Continuing.*Breakpoint 2, foo.*$gdb_prompt $" {
+        pass "$test"
+    }
+}
diff --git a/gdb/testsuite/gdb.python/py-breakpoint-create-fail.py b/gdb/testsuite/gdb.python/py-breakpoint-create-fail.py
new file mode 100644
index 0000000..845eb0f
--- /dev/null
+++ b/gdb/testsuite/gdb.python/py-breakpoint-create-fail.py
@@ -0,0 +1,31 @@ 
+# Copyright (C) 2016 Free Software Foundation, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+import gdb
+
+
+class MyBP(gdb.Breakpoint):
+    def stop(self):
+        print('MyBP.stop was invoked!')
+        # Don't make this breakpoint stop
+        return False
+
+
+try:
+    bp = MyBP('does_not_exist', gdb.BP_WATCHPOINT)
+except RuntimeError:
+    pass
+else:
+    assert False
-- 
2.8.3