This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: Fix off-by-one bug calling value_cstring
- From: Doug Evans <dje at google dot com>
- To: Daniel Colascione <dancol at dancol dot org>
- Cc: gdb-patches <gdb-patches at sourceware dot org>
- Date: Tue, 7 Oct 2014 21:13:43 -0400
- Subject: Re: Fix off-by-one bug calling value_cstring
- Authentication-results: sourceware.org; auth=none
- References: <5433792E dot 206 at dancol dot org>
On Tue, Oct 7, 2014 at 1:25 AM, Daniel Colascione <dancol@dancol.org> wrote:
> Sometimes we create strings that aren't NUL-terminated.
>
> diff --git a/gdb/c-lang.c b/gdb/c-lang.c
> index 185b38e..0953e0d 100644
> --- a/gdb/c-lang.c
> +++ b/gdb/c-lang.c
> @@ -660,7 +660,7 @@ evaluate_subexp_c (struct type *expect_type, struct
> expression *exp,
> else if ((dest_type & C_CHAR) != 0)
> result = allocate_value (type);
> else
> - result = value_cstring ("", 0, type);
> + result = value_cstring ("", 1, type);
> do_cleanups (cleanup);
> return result;
> }
> diff --git a/gdb/guile/scm-math.c b/gdb/guile/scm-math.c
> index e05f99e..d533bf6 100644
> --- a/gdb/guile/scm-math.c
> +++ b/gdb/guile/scm-math.c
> @@ -827,7 +827,7 @@ vlscm_convert_typed_value_from_scheme (const char
> *func_name,
> {
> cleanup = make_cleanup (xfree, s);
> value
> - = value_cstring (s, len,
> + = value_cstring (s, len + 1,
> language_string_char_type (language,
> gdbarch));
> do_cleanups (cleanup);
> diff --git a/gdb/python/py-value.c b/gdb/python/py-value.c
> index 0cefd4f..ca20921 100644
> --- a/gdb/python/py-value.c
> +++ b/gdb/python/py-value.c
> @@ -1601,7 +1601,7 @@ convert_value_from_python (PyObject *obj)
> struct cleanup *old;
>
> old = make_cleanup (xfree, s);
> - value = value_cstring (s, strlen (s), builtin_type_pychar);
> + value = value_cstring (s, strlen (s) + 1, builtin_type_pychar);
> do_cleanups (old);
> }
> }
> diff --git a/gdb/value.c b/gdb/value.c
> index fdc8858d..0198e03 100644
> --- a/gdb/value.c
> +++ b/gdb/value.c
> @@ -2147,7 +2147,7 @@ value_of_internalvar (struct gdbarch *gdbarch,
> struct internalvar *var)
> break;
>
> case INTERNALVAR_STRING:
> - val = value_cstring (var->u.string, strlen (var->u.string),
> + val = value_cstring (var->u.string, strlen (var->u.string) + 1,
> builtin_type (gdbarch)->builtin_char);
> break;
>
>
Interesting, thanks for finding this.
I think there is more going on here though.
In the scm-math.c case, the string passed to value_cstring is not
NUL-terminated, so the +1 isn't right.
I think the first thing we need to do is document the API of value_cstring.
Let's first agree on what its inputs and outputs are, and then update
all callers appropriately.
Often when we pass the length for a C string, the length does not
include the trailing NUL - it obviates requiring the caller to ensure
one is there.
E.g. savestring works this way.
IOW we *could* define value_cstring such that the LEN argument does
not include the trailing NUL.
I'm not saying we *want* to do that, at least not yet.
It's just one alternative.
I don't have an answer. If I get to this before someone else I'll
pick something, but it might be a few days.