]> git.ipfire.org Git - thirdparty/binutils-gdb.git/commit
gdb: consider null terminator for length arguments of value_cstring calls
authorSimon Marchi <simon.marchi@polymtl.ca>
Tue, 13 Jul 2021 15:31:42 +0000 (11:31 -0400)
committerPedro Alves <pedro@palves.net>
Thu, 15 Jul 2021 14:10:52 +0000 (15:10 +0100)
commit67ea24cb99efcd50d8acb6ce3e3ffbd8c97f0829
tree31dcd343217cede2087c7fb1b794ef7da81d9bd2
parentf9e5d80cf75c4c549c392b6bb1bd33e103824657
gdb: consider null terminator for length arguments of value_cstring calls

This patch started when I looked at this bit in cli/cli-cmds.c:

    if (*(char **) cmd->var)
      return value_cstring (*(char **) cmd->var, strlen (*(char **) cmd->var),
    builtin_type (gdbarch)->builtin_char);
    else
      return value_cstring ("", 1,
    builtin_type (gdbarch)->builtin_char);

I found it odd that the first value_cstring call passes a length that
does not consider the null terminator of the C string, but second does.
I want to clarify the documentation of value_cstring and fix the one
that is wrong.

Debugging a little bit, I found that the first call is the wrong one.
Doing:

  (gdb) set args foo
  (gdb) print $_gdb_setting("args")
  $1 = "foo"

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
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
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.

gdb.base/settings.exp and gdb.python/py-mi.exp required changes in some
expected outputs, because the array type created by $_gdb_setting_str
is now one element larger, to account for the null terminator.  I don't
think this change in user-visible behavior is a problem.

Change-Id: If8dd13cce55c70521e34e7c360139064b4e87496
gdb/cli/cli-cmds.c
gdb/guile/scm-math.c
gdb/python/py-value.c
gdb/testsuite/gdb.base/internal-string-values.c [new file with mode: 0644]
gdb/testsuite/gdb.base/internal-string-values.exp [new file with mode: 0644]
gdb/testsuite/gdb.base/settings.exp
gdb/testsuite/gdb.python/py-mi.exp
gdb/valops.c
gdb/value.c
gdb/value.h