diff mbox

Fix off-by-one bug calling value_cstring

Message ID 5433792E.206@dancol.org
State New
Headers show

Commit Message

Daniel Colascione Oct. 7, 2014, 5:25 a.m. UTC
Sometimes we create strings that aren't NUL-terminated.

Comments

Doug Evans Oct. 8, 2014, 1:13 a.m. UTC | #1
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.
Daniel Colascione Nov. 9, 2014, 10:03 p.m. UTC | #2
On 10/08/2014 02:13 AM, Doug Evans wrote:
> 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.

Did you get around to fixing the bug?
Doug Evans Nov. 12, 2014, 3:47 p.m. UTC | #3
On Sun, Nov 9, 2014 at 2:03 PM, Daniel Colascione <dancol@dancol.org> wrote:
> On 10/08/2014 02:13 AM, Doug Evans wrote:
>> 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.
>
> Did you get around to fixing the bug?

Hi.

I haven't had time to yet.
I looked at it a bit.
It seems most(all?) callers of value_cstring work like savestring and
pass a length that doesn't include the trailing NUL, so I'd say let's
stick with that.  We still need to fix value_cstring though and add
one to the given length (I presume that's the intended semantics).
diff mbox

Patch

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;