public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
* [PATCH] gdb/python: restrict the names accepted by gdb.register_window_type
@ 2022-09-14 14:29 Andrew Burgess
  2022-09-21 14:42 ` Tom Tromey
  0 siblings, 1 reply; 3+ messages in thread
From: Andrew Burgess @ 2022-09-14 14:29 UTC (permalink / raw)
  To: gdb-patches

I noticed that, from Python, I could register a new TUI window that
had whitespace in its name, like this:

  gdb.register_window_type('my window', MyWindowType)

however, it is not possible to then use this window in a new TUI
layout, e.g.:

  (gdb) tui new-layout foo my window 1 cmd 1
  Unknown window "my"
  (gdb) tui new-layout foo "my window" 1 cmd 1
  Unknown window ""my"
  (gdb) tui new-layout foo my\ window 1 cmd 1
  Unknown window "my\"

GDB clearly uses the whitespace to split the incoming command line.

I could fix this by trying to add a mechanism by which we can use
whitespace within a window name, but it seems like an easier solution
if we just forbid whitespace within a window name.  Not only is this
easier, but I think this is probably the better solution, identifier
names with spaces in would mean we'd need to audit all the places a
window name could be printed and ensure that the use of a space didn't
make the output ambiguous.

So, having decided to disallow whitespace, I then thought about other
special characters.  We currently accept anything as a window name,
and I wondered if this was a good idea.

My concerns were about how special characters used in a window name
might cause confusion, for example, we allow '$' in window names,
which is maybe fine now, but what if one day we wanted to allow
variable expansion when creating new layouts?  Or what about starting
a window name with '-'?  We already support a '-horizontal' option,
what if we want to add more in the future?  Or use of the special
character '{' which has special meaning within a new layout?

In the end I figured it might make sense to place some restrictive
rules in place, and then relax the rules later if/when users complain,
we can consider each relaxation as its requested.

So, I propose that window names should match this regular expression:

  [a-zA-Z][-_.a-zA-Z0-9]*

There is a chance that there is user code in the wild which will break
with the addition of this change, but hopefully adapting to the new
restrictions shouldn't be too difficult.
---
 gdb/NEWS                                      |  5 ++
 gdb/doc/python.texi                           |  4 +-
 gdb/testsuite/gdb.python/tui-window-names.exp | 84 +++++++++++++++++++
 gdb/tui/tui-layout.c                          | 13 +++
 4 files changed, 105 insertions(+), 1 deletion(-)
 create mode 100644 gdb/testsuite/gdb.python/tui-window-names.exp

diff --git a/gdb/NEWS b/gdb/NEWS
index 555ef2ddf77..9619842bc03 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -193,6 +193,11 @@ GNU/Linux/CSKY (gdbserver) csky*-*linux*
      gdb.BreakpointLocation objects specifying the locations where the
      breakpoint is inserted into the debuggee.
 
+  ** The gdb.register_window_type method now restricts the set of
+     acceptable window names.  The first character of a window's name
+     must start with a character in the set [a-zA-Z], every subsequent
+     character of a window's name must be in the set [-_.a-zA-Z0-9].
+
 * New features in the GDB remote stub, GDBserver
 
   ** GDBserver is now supported on LoongArch GNU/Linux.
diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
index 7aa9e853d85..2692211f388 100644
--- a/gdb/doc/python.texi
+++ b/gdb/doc/python.texi
@@ -6597,7 +6597,9 @@
 
 @var{name} is the name of the new window.  It's an error to try to
 replace one of the built-in windows, but other window types can be
-replaced.
+replaced.  The @var{name} should match the regular expression
+@code{[a-zA-Z][-_.a-zA-Z0-9]*}, it is an error to try and create a
+window with an invalid name.
 
 @var{function} is a factory function that is called to create the TUI
 window.  This is called with a single argument of type
diff --git a/gdb/testsuite/gdb.python/tui-window-names.exp b/gdb/testsuite/gdb.python/tui-window-names.exp
new file mode 100644
index 00000000000..8aaf3b42229
--- /dev/null
+++ b/gdb/testsuite/gdb.python/tui-window-names.exp
@@ -0,0 +1,84 @@
+# Copyright (C) 2022 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/>.
+
+# Test that GDB rejects invalid TUI window names, and that valid names
+# are allowed.
+
+load_lib gdb-python.exp
+
+tuiterm_env
+
+clean_restart
+
+if {[skip_tui_tests]} {
+    return
+}
+
+# Define a function we can use as a window constructor.  If this ever
+# gets called we'll throw an error, but that's OK, this test doesn't
+# actually try to create any windows.
+gdb_test_multiline "create a window constructor" \
+    "python" "" \
+    "def failwin(win):" "" \
+    "  raise RuntimeError('failwin')" "" \
+    "end" ""
+
+# Check for some of the characters that can't be used within a window
+# name.
+foreach c {$ * \{ \} ( ) @ #} {
+    set re [string_to_regexp "$c"]
+    gdb_test "python gdb.register_window_type('te${c}st', failwin)" \
+	[multi_line \
+	     "gdb.error: invalid character '${re}' in window name" \
+	     "Error while executing Python code\\." ]
+
+    gdb_test "python gdb.register_window_type('${c}test', failwin)" \
+	[multi_line \
+	     "gdb.error: invalid character '${re}' in window name" \
+	     "Error while executing Python code\\." ]
+}
+
+# Check that whitespace within a window name is rejected.
+foreach c [list " " "\\t" "\\n" "\\r"] {
+    gdb_test "python gdb.register_window_type('te${c}st', failwin)" \
+	[multi_line \
+	     "gdb.error: invalid whitespace character in window name" \
+	     "Error while executing Python code\\." ]
+}
+
+# Check some of the characters which are allowed within a window name,
+# but are not allowed to be used as the first character.
+foreach c {1 _ - .} {
+    set re [string_to_regexp "$c"]
+    gdb_test "python gdb.register_window_type('${c}test', failwin)" \
+	[multi_line \
+	     "gdb.error: window name must start with a letter, not '${re}'" \
+	     "Error while executing Python code\\." ]
+}
+
+# Check different capitalisations.
+gdb_test_no_output "python gdb.register_window_type('TEST', failwin)"
+gdb_test_no_output "python gdb.register_window_type('test', failwin)"
+gdb_test_no_output "python gdb.register_window_type('tEsT', failwin)"
+gdb_test_no_output "python gdb.register_window_type('TeSt', failwin)"
+
+# Check a set of characters that can appear within a name, just not
+# necessarily as the first character.  We check at both the end of the
+# name, and within the name.
+foreach c {1 _ - . A} {
+    set re [string_to_regexp "$c"]
+    gdb_test_no_output "python gdb.register_window_type('test${c}', failwin)"
+    gdb_test_no_output "python gdb.register_window_type('te${c}st', failwin)"
+}
diff --git a/gdb/tui/tui-layout.c b/gdb/tui/tui-layout.c
index 09887d3d594..2c4e60ab2cb 100644
--- a/gdb/tui/tui-layout.c
+++ b/gdb/tui/tui-layout.c
@@ -44,6 +44,7 @@
 #include "tui/tui-layout.h"
 #include "tui/tui-source.h"
 #include "gdb_curses.h"
+#include "safe-ctype.h"
 
 static void extract_display_start_addr (struct gdbarch **, CORE_ADDR *);
 
@@ -405,6 +406,18 @@ tui_register_window (const char *name, window_factory &&factory)
       || name_copy == DISASSEM_NAME || name_copy == STATUS_NAME)
     error (_("Window type \"%s\" is built-in"), name);
 
+  for (const char &c : name_copy)
+    {
+      if (ISSPACE (c))
+	error (_("invalid whitespace character in window name"));
+
+      if (!ISALNUM (c) && strchr ("-_.", c) == nullptr)
+	error (_("invalid character '%c' in window name"), c);
+    }
+
+  if (!ISALPHA (name_copy[0]))
+    error (_("window name must start with a letter, not '%c'"), name_copy[0]);
+
   known_window_types->emplace (std::move (name_copy),
 			       std::move (factory));
 }
-- 
2.25.4


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] gdb/python: restrict the names accepted by gdb.register_window_type
  2022-09-14 14:29 [PATCH] gdb/python: restrict the names accepted by gdb.register_window_type Andrew Burgess
@ 2022-09-21 14:42 ` Tom Tromey
  2022-09-22  9:42   ` Andrew Burgess
  0 siblings, 1 reply; 3+ messages in thread
From: Tom Tromey @ 2022-09-21 14:42 UTC (permalink / raw)
  To: Andrew Burgess via Gdb-patches

>>>>> "Andrew" == Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org> writes:

Andrew> In the end I figured it might make sense to place some restrictive
Andrew> rules in place, and then relax the rules later if/when users complain,
Andrew> we can consider each relaxation as its requested.

Andrew> So, I propose that window names should match this regular expression:

Andrew>   [a-zA-Z][-_.a-zA-Z0-9]*

Makes sense to me.

Andrew> There is a chance that there is user code in the wild which will break
Andrew> with the addition of this change, but hopefully adapting to the new
Andrew> restrictions shouldn't be too difficult.

It's possible but I wouldn't worry about it.

This patch looks good to me.

Tom

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] gdb/python: restrict the names accepted by gdb.register_window_type
  2022-09-21 14:42 ` Tom Tromey
@ 2022-09-22  9:42   ` Andrew Burgess
  0 siblings, 0 replies; 3+ messages in thread
From: Andrew Burgess @ 2022-09-22  9:42 UTC (permalink / raw)
  To: Tom Tromey, Andrew Burgess via Gdb-patches

Tom Tromey <tom@tromey.com> writes:

>>>>>> "Andrew" == Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org> writes:
>
> Andrew> In the end I figured it might make sense to place some restrictive
> Andrew> rules in place, and then relax the rules later if/when users complain,
> Andrew> we can consider each relaxation as its requested.
>
> Andrew> So, I propose that window names should match this regular expression:
>
> Andrew>   [a-zA-Z][-_.a-zA-Z0-9]*
>
> Makes sense to me.
>
> Andrew> There is a chance that there is user code in the wild which will break
> Andrew> with the addition of this change, but hopefully adapting to the new
> Andrew> restrictions shouldn't be too difficult.
>
> It's possible but I wouldn't worry about it.
>
> This patch looks good to me.

Thanks, pushed.

Andrew


^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2022-09-22  9:43 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-14 14:29 [PATCH] gdb/python: restrict the names accepted by gdb.register_window_type Andrew Burgess
2022-09-21 14:42 ` Tom Tromey
2022-09-22  9:42   ` Andrew Burgess

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).