Re: [PATCH 2/4] Return true in TuiWindow.is_valid only if TUI is enabled

[Andrew Burgess <andrew.burgess@embecosm.com>]

* Tom Tromey <tom@tromey.com> [2021-02-06 12:38:56 -0700]:

> >>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:
> 
> Andrew>     gdb/ChangeLog:
>     
> Andrew>             * python/py-tui.c (gdbpy_tui_window) <is_valid>: New member
> Andrew>             function.
> Andrew>             (REQUIRE_WINDOW): Call is_valid member function.
> Andrew>             (REQUIRE_WINDOW_FOR_SETTER): New define.
> Andrew>             (gdbpy_tui_is_valid): Call is_valid member function.
> Andrew>             (gdbpy_tui_set_title): Remove duplicate error checking, call
> Andrew>             REQUIRE_WINDOW_FOR_SETTER instead.
> Andrew>             * tui/tui-data.h (struct tui_win_info) <is_visible>: Check
> Andrew>             tui_active too.
> Andrew>             * tui/tui-layout.c (tui_apply_current_layout): Add an assert.
> Andrew>             * tui/tui.c (tui_enable): Move setting of tui_active earlier in
> Andrew>             the function.
>     
> Hi.  Thanks for the patch.
> 
> I really thought I had covered this scenario somehow, but I guess not!
> Oops, sorry about that.  Maybe I was thinking that tui disable would
> destory the windows.  Or maybe I planned to do that and then never did.
> I don't recall any more.
> 
> Andrew> -  if (win->window == nullptr)
> Andrew> -    {
> Andrew> -      PyErr_Format (PyExc_RuntimeError, _("TUI window is invalid."));
> Andrew> -      return -1;
> Andrew> -    }
> Andrew> -
> Andrew> -  if (win->window == nullptr)
> Andrew> -    {
> Andrew> -      PyErr_Format (PyExc_TypeError, _("Cannot delete \"title\" attribute."));
> Andrew> -      return -1;
> Andrew> -    }
> 
> This second 'if' has the same condition.  It's actually a latent bug, as
> the code should read:
> 
>     if (newvalue == nullptr)
> 
> ... and then the "if" should remain, rather than being deleted by this
> patch.
> 
> I think the patch is ok with this tweak.

Thanks for pointing this out.

I span this fix out into its own commit and added a test for it.
Below is what I pushed.

Thanks,
Andrew

---

commit e0c23e11da18b615c382888da8e978f16428e81b
Author: Andrew Burgess <andrew.burgess@embecosm.com>
Date:   Mon Feb 8 11:44:51 2021 +0000

    gdb/python: don't allow the user to delete window title attributes
    
    There's a bug in the python tui API.  If the user tries to delete the
    window title attribute then this will trigger undefined behaviour in
    GDB due to a missing nullptr check.
    
    gdb/ChangeLog:
    
            * python/py-tui.c (gdbpy_tui_set_title): Check that the new value
            for the title is not nullptr.
    
    gdb/testsuite/ChangeLog:
    
            * gdb.python/tui-window.exp: Add new tests.
            * gdb.python/tui-window.py (TestWindow) <__init__>: Store
            TestWindow object into global the_window.
            <remote_title>: New method.
            (delete_window_title): New function.

diff --git a/gdb/python/py-tui.c b/gdb/python/py-tui.c
index 6e9a1462ec5..73b73f33d4a 100644
--- a/gdb/python/py-tui.c
+++ b/gdb/python/py-tui.c
@@ -434,7 +434,7 @@ gdbpy_tui_set_title (PyObject *self, PyObject *newvalue, void *closure)
       return -1;
     }
 
-  if (win->window == nullptr)
+  if (newvalue == nullptr)
     {
       PyErr_Format (PyExc_TypeError, _("Cannot delete \"title\" attribute."));
       return -1;
diff --git a/gdb/testsuite/gdb.python/tui-window.exp b/gdb/testsuite/gdb.python/tui-window.exp
index 13e14be0654..8d86afb1449 100644
--- a/gdb/testsuite/gdb.python/tui-window.exp
+++ b/gdb/testsuite/gdb.python/tui-window.exp
@@ -47,6 +47,12 @@ Term::check_contents "test title" \
     "This Is The Title"
 Term::check_contents "Window display" "Test: 0"
 
+Term::command "python delete_window_title ()"
+Term::check_contents "error message after trying to delete title" \
+    "TypeError: Cannot delete \"title\" attribute\\."
+Term::check_contents "title is unchanged" \
+    "This Is The Title"
+
 Term::resize 51 51
 # Remember that a resize request actually does two resizes...
 Term::check_contents "Window was updated" "Test: 2"
diff --git a/gdb/testsuite/gdb.python/tui-window.py b/gdb/testsuite/gdb.python/tui-window.py
index 88a1b06f12c..3bea78846ae 100644
--- a/gdb/testsuite/gdb.python/tui-window.py
+++ b/gdb/testsuite/gdb.python/tui-window.py
@@ -22,7 +22,7 @@ the_window = None
 class TestWindow:
     def __init__(self, win):
         global the_window
-        the_window = win
+        the_window = self
         self.count = 0
         self.win = win
         win.title = "This Is The Title"
@@ -34,8 +34,16 @@ class TestWindow:
         self.win.write("Test: " + str(self.count) + " " + str(w) + "x" + str(h))
         self.count = self.count + 1
 
+    # Tries to delete the title attribute.  GDB will throw an error.
+    def remove_title(self):
+        del self.win.title
+
 gdb.register_window_type("test", TestWindow)
 
+# Call REMOVE_TITLE on the global window object.
+def delete_window_title ():
+    the_window.remove_title ()
+
 # A TUI window "constructor" that always fails.
 def failwin(win):
     raise RuntimeError("Whoops")