[PATCH] gdb: consider null terminator for length arguments of value_cstring calls

[Pedro Alves]

Adding Philippe as $_gdb_setting original author.

On 2021-07-13 7:44 p.m., Simon Marchi wrote:
> On 2021-07-13 1:07 p.m., Pedro Alves wrote:
>> On 2021-07-13 4:31 p.m., Simon Marchi via Gdb-patches wrote:

> Ohh, thanks.  The changes I needed to do in the existing tests should
> have made me think about that.  I liked the inferior call approach,
> because it makes the inferior program crash, so it's an obvious bug.  By
> just printing the string elements, it may not be as clear cut that it's
> a bug.  As long as the values stay in GDB, they don't cause a crash (or
> at least, I didn't find a way to cause a crash).
> 
> But if there's not doubt that null-terminating these strings is what we
> want, your approach is fine and makes the test a bit simpler.  I added a
> little mention of the inferior function call way of reproducing the bug
> in the commit message.  See the updated patch below.
> 

For C, it makes sence to me to return a null terminated string.  Though I wonder
whether the string you get should depend on language?  I mean, see this:

 (gdb) set language c
 (gdb) ptype "foo"
 type = char [4]
 (gdb) set language ada
 (gdb) ptype "foo"
 type = array (1 .. 3) of character

Whatever we decide, I think we should update the manual to make it explicit.

> 
> creates a struct value of code TYPE_CODE_ARRAY of size 3, which doesn't
> have a null terminator.  That does not create a valid C string.  It is
> however printed correctly by GDB, because the printing code makes sure
> not to read past the value's length.
> 
> A way to expose the bug is to print each element of the created string,
> including the null terminator.  Before:
> 
>     (gdb) set args foo
>     (gdb) p $_gdb_setting("args")[3]
>     no such vector element
> 
> After:
> 
>     (gdb) set args foo
>     (gdb) p $_gdb_setting("args")[3]
>     $1 = 0 '\000'
> 
> Another perhaps more convincing way of showing the bug is if the string
> value is passed to an inferior function call;
> 
>     (gdb) print an_inferior_function($_gdb_setting("args))
> 
> The space allocate for the string in the inferior will not take into

"The space allocated" ?

Such an inferior call in Ada (and other non-terminated string languages) will now
be passing an unexpected \0 to the called function, of course.  I wonder
whether that's why Philippe didn't add the \0 in the first place, given his
Ada bias?  :-)

> account a null terminator (with the string "foo", 3 bytes will be
> allocated).  If the inferior tries to read the string until the null
> terminator, it will read past the allocated buffer.  Compiling the
> inferior with AddressSanitizer makes that bad access obvious.
> 
> I found a few calls to value_cstring that were wrong, I fixed them
> all, all exercised by the test.
> 
> The change in guile/scm-math.c maybe requires a bit of explanation.
> According to the doc of gdbscm_scm_to_string, if don't pass a length

"if we don't" ?

> pointer, we get back a null-terminated string.  If we pass a length
> pointer, we get a non-null-terminated string, but we get the length
> separately.  Trying to pass "len + 1" to value_cstring would lead to GDB
> reading past the allocated buffer, that is exactly of length `len`.  So
> instead, don't pass a length pointer and use strlen on the result.



> +load_lib "trace-support.exp"
> +load_lib "gdb-guile.exp"
> +
> +standard_testfile
> +
> +if {[build_executable "failed to prepare" $testfile $srcfile ]} {
> +    return
> +}
> +
> +# Check that the string contained in the convenienced variable $v is

convenienced -> convenience

> +# EXPECTED_STR.
> +#
> +# In particular, check that the null terminator is there and that we can't
> +# access a character past the end of the string.
> +
> +proc check_v_string { expected_str } {

> +# Test with string values made by $_gdb_setting & co.
> +
> +proc_with_prefix test_setting { } {
> +    clean_restart
> +
> +    # This is an internal GDB implementation detail, but the variable backing a
> +    # string setting starts as nullptr (unless explicitly initialized at startup).
> +    # When assigning an empty value, the variable then points to an empty string.
> +    # Test both cases, as it triggers different code paths (in addition to a
> +    # non-empty value).

The paranthesis remark in this last sentence gave me pause, as it sounded like
you were saying that the different code paths result in a non-empty value.

I'd suggest instead:

   # Test both cases, as it triggers different code paths, in addition to testing a
   # non-empty value.

> +    #
> +    # Use "set trace-user" and "maintenance set test-settings string" as they are
> +    # both not initialized at startup.
> +    with_test_prefix "user setting" {
> +	with_test_prefix "not set" {
> +	    foreach_with_prefix conv_func {$_gdb_setting $_gdb_setting_str} {
> +		gdb_test_no_output "set \$v = ${conv_func}(\"trace-user\")"
> +		check_v_string ""
> +	    }
> +	}


> +# Test with a string value created by make-value in Guile.
> +
> +proc_with_prefix test_guile_value { } {
> +    clean_restart
> +
> +    if {[skip_guile_tests]} {
> +	untested "skipping test_guile_value"
> +	return
> +    }
> +
> +    # We can't set a convenience var from Guile, but we can append to history.
> +    # Do that, then transfer to a convenience var with a CLI command.
> +    gdb_test_no_output "guile (use-modules (gdb))"
> +    gdb_test_multiple "guile (history-append! (make-value \"foo\"))" "make value" {
> +	-re -wrap "($::decimal)" {
> +	    set histnum $expect_out(1,string)

set histnum to something before gdb_test_multiple, to avoid ending up with an
unset tcl variable error if this gdb_test_multiple fails.

> +	}
> +    }
> +
> +    gdb_test_no_output "set \$v = \$$histnum"
> +    check_v_string "foo"
> +}
> +