Re: [PATCH] Fix error message and use-after-free on errors in nested source files

[Kevin Buettner <kevinb at redhat dot com>]

On Mon, 18 Feb 2019 18:46:40 -0500
Simon Marchi <simon.marchi@polymtl.ca> wrote:

> Errors that happen in nested sourced files (when a sourced file sources
> another file) lead to a wrong error message, or use-after-free.
> 
> For example, if I put this in "a.gdb":
> 
>     command_that_doesnt_exist
> 
> and this in "b.gdb":
> 
>    source a.gdb
> 
> and try to "source b.gdb" in GDB, the result may look like this:
> 
>     (gdb) source b.gdb
>     b.gdb:1: Error in sourced command file:
>     _that_doesnt_exist:1: Error in sourced command file:
>     Undefined command: "command_that_doesnt_exist".  Try "help".
> 
> Notice the wrong file name where "a.gdb" should be.  The exact result
> may differ, depending on the feelings of the memory allocator.
> 
> What happens is:
> 
> - The "source a.gdb" command is saved by command_line_append_input_line
>   in command_line_input's static buffer.
> - Since we are sourcing a file, the script_from_file function stores the
>   script name (a.gdb) in the source_file_name global.  However, it doesn't
>   do a copy, it just saves a pointer to command_line_input's static buffer.
> - The "command_that_doesnt_exist" command is saved by
>   command_line_append_input_line in command_line_input's static buffer.
>   Depending on what xrealloc does, source_file_name may now point to
>   freed memory, or at the minimum the data it was pointing to was
>   overwritten.
> - When the error is handled in script_from_file, we dererence
>   source_file_name to print the name of the file in which the error
>   occured.
> 
> To fix it, I made source_file_name an std::string, so that keeps a copy of
> the file name instead of pointing to a buffer with a too small
> lifetime.
> 
> With this patch, the expected filename is printed, and no use-after-free
> occurs:
> 
>     (gdb) source b.gdb
>     b.gdb:1: Error in sourced command file:
>     a.gdb:1: Error in sourced command file:
>     Undefined command: "command_that_doesnt_exist".  Try "help".
> 
> I passed explicit template parameters to make_scoped_restore
> (<std::string, const std::string &>), so that the second parameter is
> passed by reference and avoid a copy.
> 
> It was not as obvious as I first thought to change gdb.base/source.exp
> to test this, because source commands inside sourced files are
> interpreted relative to GDB's current working directory, not the
> directory of the currently sourced file.  As a workaround, I moved the
> snippet that tests errors after the snippet that adds the source
> directory to the search path.  This way, the "source source-error-1.exp"

I think you meant source-error-1.gdb (instead of .exp).

> line in source-error.exp manages to find the file.

Thanks for this detailed explanation!

Might it be possible to (additionally) place some semblance of that
last paragraph into the .exp file?  (I'm thinking that it might be useful
to explain the pitfalls of moving that test from where you have it
now to earlier in the file.)

The patch looks (mostly) good to me, though I do have a question...

> diff --git a/gdb/testsuite/gdb.base/source.exp b/gdb/testsuite/gdb.base/source.exp
> index c6ff65783da0..5dd4decf6a5e 100644
> --- a/gdb/testsuite/gdb.base/source.exp
> +++ b/gdb/testsuite/gdb.base/source.exp
> @@ -23,10 +23,6 @@ standard_testfile structs.c
>  
>  gdb_start
>  
> -gdb_test "source ${srcdir}/${subdir}/source-error.gdb" \
> -    "source-error.gdb:21: Error in sourced command file:\[\r\n\]*Cannot access memory at address 0x0.*" \
> -    "script contains error"
> -
>  gdb_test "source -v ${srcdir}/${subdir}/source-test.gdb" \
>      "echo test source options.*" \
>      "source -v"
> @@ -66,4 +62,9 @@ gdb_test "source for-sure-nonexistant-file" \
>  gdb_test "source source-nofile.gdb" \
>           "warning: for-sure-nonexistant-file: No such file or directory\.\[\r\n\]*source error not fatal"
>  
> -gdb_exit
> +
> +gdb_test "source ${srcdir}/${subdir}/source-error.gdb" \
> +    [multi_line ".*source-error.gdb:20: Error in sourced command file:" \
> +		"source-error-1.gdb:21: Error in sourced command file:" \
> +		"Cannot access memory at address 0x0" ] \
> +    "script contains error"

Did you mean to remove gdb_exit above?  If so, why?

Kevin