Re: [PATCH] gdb: fix possible use-after-free when executing commands

[Simon Marchi <simark@simark.ca>]

On 12/15/22 09:51, Jan Vrany wrote:
>>> +  "  def invoke (self, arg, from_tty):" "" \
>>> +  "    print (\"redefine_cmd output, msg = %s\" % self._msg)" "" \
>>> +  "    self._msg = arg" "" \
>>
>> Is it needed to assign arg to self._msg here?
>>
> 
> It is not, but found it usefull when testing the test.
> This way, one may only comment the next line and test would
> pass, without need to tweak following `gdb_test` lines.
> Removed in new version (below)
> 
>>> +  "    redefine_cmd (arg)" "" \
>>> +  "redefine_cmd (\"XXX\")" "" \
>>> +  "end" ""
>>> +
>>> +gdb_test "redefine_cmd AAA" \
>>> +  "redefine_cmd output, msg = XXX" \
>>> +  "call command redefining itself 1"
>>> +
>>> +gdb_test "redefine_cmd BBB" \
>>> +  "redefine_cmd output, msg = AAA" \
>>> +  "call command redefining itself 2"
>>> +
>>
>> Note that in TCL code, we use an indent of 4 columns (and just like with
>> C++ code, whole groups of 8 columns become a tab).
>>
>> In order to isolate the new test from the other tests in the file, can
>> you put the new test into its own `proc_with_prefix` function, and start
>> with a fresh GDB?  That would mean calling clean_restart at the
>> beginning of the proc.
> 
> Done, hopefully this is what you meant. Also I put the test to the end
> of the file, as it is now in its own function.
> 
> -- >8 --
> Subject: [PATCH v2] gdb/testsuite: add test for Python commands redefining
>  itself
> 
> This commit adds a test that creates a Python command that redefines
> itself during its execution. This is to test use-after-free in
> execute_command ().
> 
> This test needs run with ASan enabled in order to fail when it
> should.
> ---
>  gdb/testsuite/gdb.python/py-cmd.exp | 30 +++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
> 
> diff --git a/gdb/testsuite/gdb.python/py-cmd.exp b/gdb/testsuite/gdb.python/py-cmd.exp
> index aa95a459f46..48c3e18f1cc 100644
> --- a/gdb/testsuite/gdb.python/py-cmd.exp
> +++ b/gdb/testsuite/gdb.python/py-cmd.exp
> @@ -300,3 +300,33 @@ gdb_test_multiple "test_multiline" $test {
>  	pass $test
>      }
>  }
> +
> +# Test command redefining itself
> +
> +proc_with_prefix test_command_redefining_itself {} {
> +    # Start with a fresh gdb
> +    clean_restart
> +
> +
> +    gdb_test_multiline "input command redefining itself" \
> +	"python" "" \
> +	"class redefine_cmd (gdb.Command):" "" \
> +	"  def __init__ (self, msg):" "" \
> +	"    super (redefine_cmd, self).__init__ (\"redefine_cmd\", gdb.COMMAND_OBSCURE)" "" \
> +	"    self._msg = msg" "" \
> +	"  def invoke (self, arg, from_tty):" "" \
> +	"    print (\"redefine_cmd output, msg = %s\" % self._msg)" "" \
> +	"    redefine_cmd (arg)" "" \
> +	"redefine_cmd (\"XXX\")" "" \
> +	"end" ""
> +
> +    gdb_test "redefine_cmd AAA" \
> +	"redefine_cmd output, msg = XXX" \
> +	"call command redefining itself 1"
> +
> +    gdb_test "redefine_cmd BBB" \
> +	"redefine_cmd output, msg = AAA" \
> +	"call command redefining itself 2"
> +}
> +
> +test_command_redefining_itself

Thanks, this LGTM:

Approved-By: Simon Marchi <simon.marchi@efficios.com>

This way, you know that this part of the test doesn't rely on anything
that comes before.  The proc prefix part makes it easy to jump directly
at the right place, if you investigate a failure.  And if everything is
in its own little independent proc like that, it makes it easy to
comment out all the other tests if you are investigating a failure in a
specific one.

Simon

> -- 
> 2.35.1
>