Handle correctly passing a bad interpreter name to new-ui
Commit Message
When a bad interpreter name is passed to new-ui, such as:
(gdb) new-ui bloop /dev/pts/10
A partially created UI is left in the UI list, with interp set to NULL.
Trying to do anything that will print on this UI (such as "start") will
cause a segmentation fault.
gdb/ChangeLog:
* top.h (make_delete_ui_cleanup): New declaration.
* top.c (delete_ui_cleanup): New function.
(make_delete_ui_cleanup): New function.
(new_ui_command): Create restore_ui cleanup earlier, create a
delete_ui cleanup and discard it on success.
gdb/testsuite/ChangeLog:
* gdb.base/new-ui.exp (do_test_invalid_interpreter): New
procedure.
---
gdb/testsuite/gdb.base/new-ui.exp | 23 +++++++++++++++++++++++
gdb/top.c | 34 +++++++++++++++++++++++++---------
gdb/top.h | 3 +++
3 files changed, 51 insertions(+), 9 deletions(-)
Comments
On 07/13/2016 03:13 PM, Simon Marchi wrote:
> When a bad interpreter name is passed to new-ui, such as:
>
> (gdb) new-ui bloop /dev/pts/10
>
> A partially created UI is left in the UI list, with interp set to NULL.
> Trying to do anything that will print on this UI (such as "start") will
> cause a segmentation fault.
Whoops. Thanks for fixing this.
>
> gdb/ChangeLog:
>
> * top.h (make_delete_ui_cleanup): New declaration.
> * top.c (delete_ui_cleanup): New function.
> (make_delete_ui_cleanup): New function.
> (new_ui_command): Create restore_ui cleanup earlier, create a
> delete_ui cleanup and discard it on success.
>
> gdb/testsuite/ChangeLog:
>
> * gdb.base/new-ui.exp (do_test_invalid_interpreter): New
> procedure.
> ---
> gdb/testsuite/gdb.base/new-ui.exp | 23 +++++++++++++++++++++++
> gdb/top.c | 34 +++++++++++++++++++++++++---------
> gdb/top.h | 3 +++
> 3 files changed, 51 insertions(+), 9 deletions(-)
>
> diff --git a/gdb/testsuite/gdb.base/new-ui.exp b/gdb/testsuite/gdb.base/new-ui.exp
> index f3f66db..ddff8c4 100644
> --- a/gdb/testsuite/gdb.base/new-ui.exp
> +++ b/gdb/testsuite/gdb.base/new-ui.exp
> @@ -143,4 +143,27 @@ proc do_test {} {
> }
> }
>
> +proc do_test_invalid_interpreter {} {
An intro comment would be nice. Something like the
"wrong interpreter" comment below.
> + global testfile
> +
> + clean_restart $testfile
> +
> + spawn -pty
> + set extra_tty_name $spawn_out(slave,name)
> +
> + if ![runto_main] {
> + untested "could not run to main"
> + return -1
> + }
Is this runto_main needed?
> +
> + # Test that asking for a wrong interpreter is properly reported.
> + gdb_test "new-ui bloop $extra_tty_name" "Interpreter `bloop' unrecognized"
lowercase "Interpreter".
Did you consider instead adding this to do_test, just before
the "new-ui console" test? We do a couple other basic "new-ui"
tests there too, like the no args, and no-repeat checks. Seems this
fits right in. Since do_test also runs execution commands that print to
both consoles, that would give the same coverage. I ask because do_test is
the driver procedure for the real tests, and with the new procedure we
now have multiple clean_restart calls with no with_test_prefix, which is
something that pedantically doesn't look 100% right to me because it can
cause duplicate test messages (on internal errors, etc.).
> +
> + # Test that we can continue normally (this used to crash).
> + if ![runto_main] {
> + fail "could not run to main"
> + }
> +}
> +
> do_test
> +do_test_invalid_interpreter
> diff --git a/gdb/top.c b/gdb/top.c
> index 3174f3c..cf0b544 100644
> --- a/gdb/top.c
> +++ b/gdb/top.c
> @@ -324,6 +324,20 @@ delete_ui (struct ui *todel)
> free_ui (ui);
> }
>
> +static void
> +delete_ui_cleanup (void *void_ui)
> +{
> + struct ui *ui = (struct ui *) void_ui;
> +
> + delete_ui (ui);
> +}
> +
> +struct cleanup *
> +make_delete_ui_cleanup (struct ui *ui)
> +{
> + return make_cleanup (delete_ui_cleanup, ui);
> +}
Intro comments.
Thanks,
Pedro Alves
On 16-07-19 01:34 PM, Pedro Alves wrote:
> On 07/13/2016 03:13 PM, Simon Marchi wrote:
>> When a bad interpreter name is passed to new-ui, such as:
>>
>> (gdb) new-ui bloop /dev/pts/10
>>
>> A partially created UI is left in the UI list, with interp set to NULL.
>> Trying to do anything that will print on this UI (such as "start") will
>> cause a segmentation fault.
>
> Whoops. Thanks for fixing this.
>
>>
>> gdb/ChangeLog:
>>
>> * top.h (make_delete_ui_cleanup): New declaration.
>> * top.c (delete_ui_cleanup): New function.
>> (make_delete_ui_cleanup): New function.
>> (new_ui_command): Create restore_ui cleanup earlier, create a
>> delete_ui cleanup and discard it on success.
>>
>> gdb/testsuite/ChangeLog:
>>
>> * gdb.base/new-ui.exp (do_test_invalid_interpreter): New
>> procedure.
>> ---
>> gdb/testsuite/gdb.base/new-ui.exp | 23 +++++++++++++++++++++++
>> gdb/top.c | 34 +++++++++++++++++++++++++---------
>> gdb/top.h | 3 +++
>> 3 files changed, 51 insertions(+), 9 deletions(-)
>>
>> diff --git a/gdb/testsuite/gdb.base/new-ui.exp b/gdb/testsuite/gdb.base/new-ui.exp
>> index f3f66db..ddff8c4 100644
>> --- a/gdb/testsuite/gdb.base/new-ui.exp
>> +++ b/gdb/testsuite/gdb.base/new-ui.exp
>> @@ -143,4 +143,27 @@ proc do_test {} {
>> }
>> }
>>
>> +proc do_test_invalid_interpreter {} {
>
> An intro comment would be nice. Something like the
> "wrong interpreter" comment below.
Right, that comment makes more sense here.
>> + global testfile
>> +
>> + clean_restart $testfile
>> +
>> + spawn -pty
>> + set extra_tty_name $spawn_out(slave,name)
>> +
>> + if ![runto_main] {
>> + untested "could not run to main"
>> + return -1
>> + }
>
> Is this runto_main needed?
Woops no, copy-pasta.
>> +
>> + # Test that asking for a wrong interpreter is properly reported.
>> + gdb_test "new-ui bloop $extra_tty_name" "Interpreter `bloop' unrecognized"
>
> lowercase "Interpreter".
Well, that's the actual output from GDB, if I change it it won't match anymore :).
> Did you consider instead adding this to do_test, just before
> the "new-ui console" test? We do a couple other basic "new-ui"
> tests there too, like the no args, and no-repeat checks. Seems this
> fits right in. Since do_test also runs execution commands that print to
> both consoles, that would give the same coverage. I ask because do_test is
> the driver procedure for the real tests, and with the new procedure we
> now have multiple clean_restart calls with no with_test_prefix, which is
> something that pedantically doesn't look 100% right to me because it can
> cause duplicate test messages (on internal errors, etc.).
I don't mind, it's just that I like to have small, independent test sequences in
each exp. It's easier to analyze when you try to fix a test than when you have a
single big sequence, where you need to weed out what's relevant to what is failing
and what's not. But you are right, we should use with_test_prefix at the root level.
Perhaps it would make sense to put all the "invalid usages" tests in the separate
procedure then, and leave the main procedure to test the real stuff. We could also
factor out the extra pty creation, or just create a single one that we pass to all
the procedures.
I'll send a v2 based on your upcoming comments.
Thanks!
Simon
On 07/20/2016 06:46 PM, Simon Marchi wrote:
> On 16-07-19 01:34 PM, Pedro Alves wrote:
>> On 07/13/2016 03:13 PM, Simon Marchi wrote:
>>> When a bad interpreter name is passed to new-ui, such as:
>>>
>>> (gdb) new-ui bloop /dev/pts/10
>>>
>>> A partially created UI is left in the UI list, with interp set to NULL.
>>> Trying to do anything that will print on this UI (such as "start") will
>>> cause a segmentation fault.
>>
>> Whoops. Thanks for fixing this.
>>
>>>
>>> gdb/ChangeLog:
>>>
>>> * top.h (make_delete_ui_cleanup): New declaration.
>>> * top.c (delete_ui_cleanup): New function.
>>> (make_delete_ui_cleanup): New function.
>>> (new_ui_command): Create restore_ui cleanup earlier, create a
>>> delete_ui cleanup and discard it on success.
>>>
>>> gdb/testsuite/ChangeLog:
>>>
>>> * gdb.base/new-ui.exp (do_test_invalid_interpreter): New
>>> procedure.
>>> ---
>>> gdb/testsuite/gdb.base/new-ui.exp | 23 +++++++++++++++++++++++
>>> gdb/top.c | 34 +++++++++++++++++++++++++---------
>>> gdb/top.h | 3 +++
>>> 3 files changed, 51 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/gdb/testsuite/gdb.base/new-ui.exp b/gdb/testsuite/gdb.base/new-ui.exp
>>> index f3f66db..ddff8c4 100644
>>> --- a/gdb/testsuite/gdb.base/new-ui.exp
>>> +++ b/gdb/testsuite/gdb.base/new-ui.exp
>>> @@ -143,4 +143,27 @@ proc do_test {} {
>>> }
>>> }
>>>
>>> +proc do_test_invalid_interpreter {} {
>>
>> An intro comment would be nice. Something like the
>> "wrong interpreter" comment below.
>
> Right, that comment makes more sense here.
>
>>> + global testfile
>>> +
>>> + clean_restart $testfile
>>> +
>>> + spawn -pty
>>> + set extra_tty_name $spawn_out(slave,name)
>>> +
>>> + if ![runto_main] {
>>> + untested "could not run to main"
>>> + return -1
>>> + }
>>
>> Is this runto_main needed?
>
> Woops no, copy-pasta.
>
>>> +
>>> + # Test that asking for a wrong interpreter is properly reported.
>>> + gdb_test "new-ui bloop $extra_tty_name" "Interpreter `bloop' unrecognized"
>>
>> lowercase "Interpreter".
>
> Well, that's the actual output from GDB, if I change it it won't match anymore :).
Whoops, misread it as test message. :-P But hey, I still caught a problem
then. :-) "$extra_tty_name" is not stable, thus you're ending up with
an unstable test message that depends on number of pts's already taken
on your system, phase of the moon, etc.
>
>> Did you consider instead adding this to do_test, just before
>> the "new-ui console" test? We do a couple other basic "new-ui"
>> tests there too, like the no args, and no-repeat checks. Seems this
>> fits right in. Since do_test also runs execution commands that print to
>> both consoles, that would give the same coverage. I ask because do_test is
>> the driver procedure for the real tests, and with the new procedure we
>> now have multiple clean_restart calls with no with_test_prefix, which is
>> something that pedantically doesn't look 100% right to me because it can
>> cause duplicate test messages (on internal errors, etc.).
>
> I don't mind, it's just that I like to have small, independent test sequences in
> each exp. It's easier to analyze when you try to fix a test than when you have a
> single big sequence, where you need to weed out what's relevant to what is failing
> and what's not.
Thought that might be the reason. That's a fine principle.
> But you are right, we should use with_test_prefix at the root level.
>
> Perhaps it would make sense to put all the "invalid usages" tests in the separate
> procedure then, and leave the main procedure to test the real stuff. We could also
> factor out the extra pty creation, or just create a single one that we pass to all
> the procedures.
All reasonable choices.
>
> I'll send a v2 based on your upcoming comments.
As long as my "duplicate test messages" itch is addressed, I'll be
fine with whatever you prefer.
Thanks,
Pedro Alves
@@ -143,4 +143,27 @@ proc do_test {} {
}
}
+proc do_test_invalid_interpreter {} {
+ global testfile
+
+ clean_restart $testfile
+
+ spawn -pty
+ set extra_tty_name $spawn_out(slave,name)
+
+ if ![runto_main] {
+ untested "could not run to main"
+ return -1
+ }
+
+ # Test that asking for a wrong interpreter is properly reported.
+ gdb_test "new-ui bloop $extra_tty_name" "Interpreter `bloop' unrecognized"
+
+ # Test that we can continue normally (this used to crash).
+ if ![runto_main] {
+ fail "could not run to main"
+ }
+}
+
do_test
+do_test_invalid_interpreter
@@ -324,6 +324,20 @@ delete_ui (struct ui *todel)
free_ui (ui);
}
+static void
+delete_ui_cleanup (void *void_ui)
+{
+ struct ui *ui = (struct ui *) void_ui;
+
+ delete_ui (ui);
+}
+
+struct cleanup *
+make_delete_ui_cleanup (struct ui *ui)
+{
+ return make_cleanup (delete_ui_cleanup, ui);
+}
+
/* Open file named NAME for read/write, making sure not to make it the
controlling terminal. */
@@ -353,13 +367,13 @@ new_ui_command (char *args, int from_tty)
char **argv;
const char *interpreter_name;
const char *tty_name;
- struct cleanup *back_to;
- struct cleanup *streams_chain;
+ struct cleanup *success_chain;
+ struct cleanup *failure_chain;
dont_repeat ();
argv = gdb_buildargv (args);
- back_to = make_cleanup_freeargv (argv);
+ success_chain = make_cleanup_freeargv (argv);
argc = countargv (argv);
if (argc < 2)
@@ -368,7 +382,9 @@ new_ui_command (char *args, int from_tty)
interpreter_name = argv[0];
tty_name = argv[1];
- streams_chain = make_cleanup (null_cleanup, NULL);
+ make_cleanup (restore_ui_cleanup, current_ui);
+
+ failure_chain = make_cleanup (null_cleanup, NULL);
/* Open specified terminal, once for each of
stdin/stdout/stderr. */
@@ -379,20 +395,20 @@ new_ui_command (char *args, int from_tty)
}
ui = new_ui (stream[0], stream[1], stream[2]);
-
- discard_cleanups (streams_chain);
+ make_cleanup (delete_ui_cleanup, ui);
ui->async = 1;
- make_cleanup (restore_ui_cleanup, current_ui);
current_ui = ui;
set_top_level_interpreter (interpreter_name);
interp_pre_command_loop (top_level_interpreter ());
- /* This restores the previous UI. */
- do_cleanups (back_to);
+ discard_cleanups (failure_chain);
+
+ /* This restores the previous UI and frees argv. */
+ do_cleanups (success_chain);
printf_unfiltered ("New UI allocated\n");
}
@@ -185,6 +185,9 @@ extern void switch_thru_all_uis_next (struct switch_thru_all_uis *state);
extern struct ui *new_ui (FILE *instream, FILE *outstream, FILE *errstream);
extern void delete_ui (struct ui *todel);
+/* Cleanup that deletes a UI. */
+extern struct cleanup *make_delete_ui_cleanup (struct ui *ui);
+
/* Cleanup that restores the current UI. */
extern void restore_ui_cleanup (void *data);