Fix crash when TUI window creation fails

Message ID 20200613202705.9639-1-tom@tromey.com
State New
Headers show
Series
  • Fix crash when TUI window creation fails
Related show

Commit Message

Tom Tromey June 13, 2020, 8:27 p.m.
If a TUI window is written in Python, and if the window construction
function fails, then gdb will crash.  This patch fixes the crash.

gdb/ChangeLog
2020-06-13  Tom Tromey  <tom@tromey.com>

	* python/py-tui.c (tui_py_window::~tui_py_window): Handle case
	where m_window==nullptr.

gdb/testsuite/ChangeLog
2020-06-13  Tom Tromey  <tom@tromey.com>

	* gdb.python/tui-window.py (failwin): New function.  Register it
	as a TUI window type.
	* gdb.python/tui-window.exp: Create new "fail" layout.  Test it.
---
 gdb/ChangeLog                           | 5 +++++
 gdb/python/py-tui.c                     | 4 +++-
 gdb/testsuite/ChangeLog                 | 6 ++++++
 gdb/testsuite/gdb.python/tui-window.exp | 3 +++
 gdb/testsuite/gdb.python/tui-window.py  | 6 ++++++
 5 files changed, 23 insertions(+), 1 deletion(-)

-- 
2.17.2

Comments

Simon Marchi via Gdb-patches June 16, 2020, 4:03 p.m. | #1
On 6/13/20 9:27 PM, Tom Tromey wrote:
> If a TUI window is written in Python, and if the window construction

> function fails, then gdb will crash.  This patch fixes the crash.

> 

> gdb/ChangeLog

> 2020-06-13  Tom Tromey  <tom@tromey.com>

> 

> 	* python/py-tui.c (tui_py_window::~tui_py_window): Handle case

> 	where m_window==nullptr.

> 

> gdb/testsuite/ChangeLog

> 2020-06-13  Tom Tromey  <tom@tromey.com>

> 

> 	* gdb.python/tui-window.py (failwin): New function.  Register it

> 	as a TUI window type.

> 	* gdb.python/tui-window.exp: Create new "fail" layout.  Test it.

> ---

>  gdb/ChangeLog                           | 5 +++++

>  gdb/python/py-tui.c                     | 4 +++-

>  gdb/testsuite/ChangeLog                 | 6 ++++++

>  gdb/testsuite/gdb.python/tui-window.exp | 3 +++

>  gdb/testsuite/gdb.python/tui-window.py  | 6 ++++++

>  5 files changed, 23 insertions(+), 1 deletion(-)

> 

> diff --git a/gdb/python/py-tui.c b/gdb/python/py-tui.c

> index ca88f85eb9f..375b7b47acd 100644

> --- a/gdb/python/py-tui.c

> +++ b/gdb/python/py-tui.c

> @@ -133,7 +133,9 @@ tui_py_window::~tui_py_window ()

>  {

>    gdbpy_enter enter_py (get_current_arch (), current_language);

>  

> -  if (PyObject_HasAttrString (m_window.get (), "close"))

> +  /* This can be null if the Python function failed.  */

> +  if (m_window != nullptr

> +      && PyObject_HasAttrString (m_window.get (), "close"))


Is this "the Python function" referring to this PyObject_CallFunctionObjArgs
call (*)?

  std::unique_ptr<tui_py_window> window
    (new tui_py_window (win_name, wrapper));

  gdbpy_ref<> user_window
    (PyObject_CallFunctionObjArgs (m_constr.get (),
				   (PyObject *) wrapper.get (),
				   nullptr));
  if (user_window == nullptr)
    {
      gdbpy_print_stack ();
      return nullptr;
    }

  window->set_user_window (std::move (user_window));

Wouldn't it make sense to to move the tui_py_window
allocation until after the Python call instead?

(*) - I think the comment would gain from mentioning
it more specifically, like 'the Python "contructor" function'
or something like that.

Thanks,
Pedro Alves
Tom Tromey June 16, 2020, 8:26 p.m. | #2
>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:


Pedro> Is this "the Python function" referring to this PyObject_CallFunctionObjArgs
Pedro> call (*)?

Yep.

Pedro>   std::unique_ptr<tui_py_window> window
Pedro>     (new tui_py_window (win_name, wrapper));

Pedro>   gdbpy_ref<> user_window
Pedro>     (PyObject_CallFunctionObjArgs (m_constr.get (),
Pedro> 				   (PyObject *) wrapper.get (),
Pedro> 				   nullptr));
Pedro>   if (user_window == nullptr)
Pedro>     {
Pedro>       gdbpy_print_stack ();
Pedro>       return nullptr;
Pedro>     }

window-> set_user_window (std::move (user_window));

Pedro> Wouldn't it make sense to to move the tui_py_window
Pedro> allocation until after the Python call instead?

No, because the TUI window object is passed to the user's constructor,
and that constructor can write text there, etc.

Pedro> (*) - I think the comment would gain from mentioning
Pedro> it more specifically, like 'the Python "contructor" function'
Pedro> or something like that.

I'll update it.

Tom
Simon Marchi via Gdb-patches June 16, 2020, 10:08 p.m. | #3
On 6/16/20 9:26 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

> 

> Pedro> Is this "the Python function" referring to this PyObject_CallFunctionObjArgs

> Pedro> call (*)?

> 

> Yep.

> 

> Pedro>   std::unique_ptr<tui_py_window> window

> Pedro>     (new tui_py_window (win_name, wrapper));

> 

> Pedro>   gdbpy_ref<> user_window

> Pedro>     (PyObject_CallFunctionObjArgs (m_constr.get (),

> Pedro> 				   (PyObject *) wrapper.get (),

> Pedro> 				   nullptr));

> Pedro>   if (user_window == nullptr)

> Pedro>     {

> Pedro>       gdbpy_print_stack ();

> Pedro>       return nullptr;

> Pedro>     }

> 

> window-> set_user_window (std::move (user_window));

> 

> Pedro> Wouldn't it make sense to to move the tui_py_window

> Pedro> allocation until after the Python call instead?

> 

> No, because the TUI window object is passed to the user's constructor,

> and that constructor can write text there, etc.

> 

> Pedro> (*) - I think the comment would gain from mentioning

> Pedro> it more specifically, like 'the Python "contructor" function'

> Pedro> or something like that.

> 

> I'll update it.


Thanks for the clarification.  Feel free to merge with that
update then.

Thanks,
Pedro Alves

Patch

diff --git a/gdb/python/py-tui.c b/gdb/python/py-tui.c
index ca88f85eb9f..375b7b47acd 100644
--- a/gdb/python/py-tui.c
+++ b/gdb/python/py-tui.c
@@ -133,7 +133,9 @@  tui_py_window::~tui_py_window ()
 {
   gdbpy_enter enter_py (get_current_arch (), current_language);
 
-  if (PyObject_HasAttrString (m_window.get (), "close"))
+  /* This can be null if the Python function failed.  */
+  if (m_window != nullptr
+      && PyObject_HasAttrString (m_window.get (), "close"))
     {
       gdbpy_ref<> result (PyObject_CallMethod (m_window.get (), "close",
 					       nullptr));
diff --git a/gdb/testsuite/gdb.python/tui-window.exp b/gdb/testsuite/gdb.python/tui-window.exp
index 1a4feebe22b..878d09c1428 100644
--- a/gdb/testsuite/gdb.python/tui-window.exp
+++ b/gdb/testsuite/gdb.python/tui-window.exp
@@ -36,6 +36,7 @@  gdb_test_no_output "source ${remote_python_file}" \
     "source ${testfile}.py"
 
 gdb_test_no_output "tui new-layout test test 1 status 0 cmd 1"
+gdb_test_no_output "tui new-layout fail fail 1 status 0 cmd 1"
 
 if {![Term::enter_tui]} {
     unsupported "TUI not supported"
@@ -49,3 +50,5 @@  Term::check_contents "Window display" "Test: 0"
 Term::resize 51 51
 # Remember that a resize request actually does two resizes...
 Term::check_contents "Window was updated" "Test: 2"
+
+Term::command "layout fail"
diff --git a/gdb/testsuite/gdb.python/tui-window.py b/gdb/testsuite/gdb.python/tui-window.py
index 4deb585f138..f362b8768ae 100644
--- a/gdb/testsuite/gdb.python/tui-window.py
+++ b/gdb/testsuite/gdb.python/tui-window.py
@@ -35,3 +35,9 @@  class TestWindow:
         self.count = self.count + 1
 
 gdb.register_window_type("test", TestWindow)
+
+# A TUI window "constructor" that always fails.
+def failwin(win):
+    raise RuntimeError("Whoops")
+
+gdb.register_window_type("fail", failwin)