[COMMIT PATCH] More invalid pointer to pointer conversions.

[COMMIT PATCH] More invalid pointer to pointer conversions.

[Pedro Alves]

As a follow up to:

  http://sourceware.org/ml/gdb-patches/2013-03/msg00449.html

In a nutshell, casts between 'char **' <-> 'unsigned char **' and
'char **' <-> 'const char **' are invalid.

I grepped for "\*\*) &" and found these.  There's another one in
demangle.c, but I've split fixing that one to a separate patch.

I think the ada_decode_symbol change is perhaps the one that could be
surprising.  The function's description has this comment, which makes
things much clearer:

   The GSYMBOL parameter is "mutable" in the C++ sense: logically
   const, but nevertheless modified to a semantically equivalent form
   when a decoded name is cached in it.  */

 const char *
 ada_decode_symbol (const struct general_symbol_info *gsymbol)

With that out of the way, I think the patch ends up being pretty
obvious.

Tested on x86_64 Fedora 17.

gdb/
2013-03-13  Pedro Alves  <palves@redhat.com>

	* ada-lang.c (ada_decode_symbol): Cast away constness of GSYMBOL
	rather than casting 'const char * const *' to 'const char **'.
	* ada-lex.l (processInt): Make "trailer" local const.  Remove
	'const char **' cast.
	* arm-linux-tdep.c (arm_stap_parse_special_token): Add 'char *'
	locals, and use those as strtol output pointer, instead than doing
	invalid casts to from 'const char **' to 'char **'.
	(_initialize_demangle): Remove cast.
	* i386-tdep.c (i386_stap_parse_special_token): : Add 'char *'
	locals, and use those as strtol output pointer, instead than doing
	invalid casts to from 'const char **' to 'char **'.
	* solib-dsbt.c (dsbt_get_initial_loadmaps): Remove 'gdb_byte**'
	casts.
	* stap-probe.c (stap_parse_register_operand)
	(stap_parse_single_operand): Likewise.
---
 gdb/ada-lang.c       |    6 ++++--
 gdb/ada-lex.l        |    5 ++---
 gdb/arm-linux-tdep.c |    4 +++-
 gdb/i386-tdep.c      |   22 +++++++++++++++++-----
 gdb/solib-dsbt.c     |    4 ++--
 gdb/stap-probe.c     |   19 +++++++++++++++----
 6 files changed, 43 insertions(+), 17 deletions(-)

diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index 63a85ee..1e5c55e 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -1299,8 +1299,10 @@ static struct htab *decoded_names_store;
 const char *
 ada_decode_symbol (const struct general_symbol_info *gsymbol)
 {
-  const char **resultp =
-    (const char **) &gsymbol->language_specific.mangled_lang.demangled_name;
+  struct general_symbol_info *gsymbol_rw
+    = (struct general_symbol_info *) gsymbol;
+  const char **resultp
+    = &gsymbol_rw->language_specific.mangled_lang.demangled_name;
 
   if (*resultp == NULL)
     {
diff --git a/gdb/ada-lex.l b/gdb/ada-lex.l
index e4d72f2..93df2fb 100644
--- a/gdb/ada-lex.l
+++ b/gdb/ada-lex.l
@@ -329,8 +329,7 @@ processInt (const char *base0, const char *num0, const char *exp0)
   ULONGEST result;
   long exp;
   int base;
-
-  char *trailer;
+  const char *trailer;
 
   if (base0 == NULL)
     base = 10;
@@ -347,7 +346,7 @@ processInt (const char *base0, const char *num0, const char *exp0)
     exp = strtol(exp0, (char **) NULL, 10);
 
   errno = 0;
-  result = strtoulst (num0, (const char **) &trailer, base);
+  result = strtoulst (num0, &trailer, base);
   if (errno == ERANGE)
     error (_("Integer literal out of range"));
   if (isxdigit(*trailer))
diff --git a/gdb/arm-linux-tdep.c b/gdb/arm-linux-tdep.c
index 278a665..953e525 100644
--- a/gdb/arm-linux-tdep.c
+++ b/gdb/arm-linux-tdep.c
@@ -1088,6 +1088,7 @@ arm_stap_parse_special_token (struct gdbarch *gdbarch,
     {
       /* Temporary holder for lookahead.  */
       const char *tmp = p->arg;
+      char *endp;
       /* Used to save the register name.  */
       const char *start;
       char *regname;
@@ -1140,7 +1141,8 @@ arm_stap_parse_special_token (struct gdbarch *gdbarch,
 	  got_minus = 1;
 	}
 
-      displacement = strtol (tmp, (char **) &tmp, 10);
+      displacement = strtol (tmp, &endp, 10);
+      tmp = endp;
 
       /* Skipping last `]'.  */
       if (*tmp++ != ']')
diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c
index 61ccc3e..e76dcbe 100644
--- a/gdb/i386-tdep.c
+++ b/gdb/i386-tdep.c
@@ -3494,6 +3494,7 @@ i386_stap_parse_special_token (struct gdbarch *gdbarch,
 		  char *regname;
 		  int len;
 		  struct stoken str;
+		  char *endp;
 
 		  got_minus[0] = 0;
 		  if (*s == '+')
@@ -3504,7 +3505,8 @@ i386_stap_parse_special_token (struct gdbarch *gdbarch,
 		      got_minus[0] = 1;
 		    }
 
-		  displacements[0] = strtol (s, (char **) &s, 10);
+		  displacements[0] = strtol (s, &endp, 10);
+		  s = endp;
 
 		  if (*s != '+' && *s != '-')
 		    {
@@ -3521,7 +3523,8 @@ i386_stap_parse_special_token (struct gdbarch *gdbarch,
 		      got_minus[1] = 1;
 		    }
 
-		  displacements[1] = strtol (s, (char **) &s, 10);
+		  displacements[1] = strtol (s, &endp, 10);
+		  s = endp;
 
 		  if (*s != '+' && *s != '-')
 		    {
@@ -3538,7 +3541,8 @@ i386_stap_parse_special_token (struct gdbarch *gdbarch,
 		      got_minus[2] = 1;
 		    }
 
-		  displacements[2] = strtol (s, (char **) &s, 10);
+		  displacements[2] = strtol (s, &endp, 10);
+		  s = endp;
 
 		  if (*s != '(' || s[1] != '%')
 		    break;
@@ -3628,7 +3632,12 @@ i386_stap_parse_special_token (struct gdbarch *gdbarch,
 		    break;
 
 		  if (isdigit (*s))
-		    offset = strtol (s, (char **) &s, 10);
+		    {
+		      char *endp;
+
+		      offset = strtol (s, &endp, 10);
+		      s = endp;
+		    }
 
 		  if (*s != '(' || s[1] != '%')
 		    break;
@@ -3675,6 +3684,8 @@ i386_stap_parse_special_token (struct gdbarch *gdbarch,
 
 		  if (*s == ',')
 		    {
+		      char *endp;
+
 		      ++s;
 		      if (*s == '+')
 			++s;
@@ -3684,7 +3695,8 @@ i386_stap_parse_special_token (struct gdbarch *gdbarch,
 			  size_minus = 1;
 			}
 
-		      size = strtol (s, (char **) &s, 10);
+		      size = strtol (s, &endp, 10);
+		      s = endp;
 
 		      if (*s != ')')
 			break;
diff --git a/gdb/solib-dsbt.c b/gdb/solib-dsbt.c
index c41326b..ea2acd1 100644
--- a/gdb/solib-dsbt.c
+++ b/gdb/solib-dsbt.c
@@ -298,7 +298,7 @@ dsbt_get_initial_loadmaps (void)
   struct dsbt_info *info = get_dsbt_info ();
 
   if (0 >= target_read_alloc (&current_target, TARGET_OBJECT_FDPIC,
-			      "exec", (gdb_byte**) &buf))
+			      "exec", &buf))
     {
       info->exec_loadmap = NULL;
       error (_("Error reading DSBT exec loadmap"));
@@ -308,7 +308,7 @@ dsbt_get_initial_loadmaps (void)
     dsbt_print_loadmap (info->exec_loadmap);
 
   if (0 >= target_read_alloc (&current_target, TARGET_OBJECT_FDPIC,
-			      "interp", (gdb_byte**)&buf))
+			      "interp", &buf))
     {
       info->interp_loadmap = NULL;
       error (_("Error reading DSBT interp loadmap"));
diff --git a/gdb/stap-probe.c b/gdb/stap-probe.c
index 9b67304..1079e3b 100644
--- a/gdb/stap-probe.c
+++ b/gdb/stap-probe.c
@@ -422,9 +422,11 @@ stap_parse_register_operand (struct stap_parse_info *p)
     {
       /* The value of the displacement.  */
       long displacement;
+      char *endp;
 
       disp_p = 1;
-      displacement = strtol (p->arg, (char **) &p->arg, 10);
+      displacement = strtol (p->arg, &endp, 10);
+      p->arg = endp;
 
       /* Generating the expression for the displacement.  */
       write_exp_elt_opcode (OP_LONG);
@@ -598,7 +600,12 @@ stap_parse_single_operand (struct stap_parse_info *p)
 	tmp = skip_spaces_const (tmp);
 
       if (isdigit (*tmp))
-	number = strtol (tmp, (char **) &tmp, 10);
+	{
+	  char *endp;
+
+	  number = strtol (tmp, &endp, 10);
+	  tmp = endp;
+	}
 
       if (!reg_ind_prefix
 	  || strncmp (tmp, reg_ind_prefix, reg_ind_prefix_len) != 0)
@@ -627,11 +634,13 @@ stap_parse_single_operand (struct stap_parse_info *p)
     {
       /* A temporary variable, needed for lookahead.  */
       const char *tmp = p->arg;
+      char *endp;
       long number;
 
       /* We can be dealing with a numeric constant (if `const_prefix' is
 	 NULL), or with a register displacement.  */
-      number = strtol (tmp, (char **) &tmp, 10);
+      number = strtol (tmp, &endp, 10);
+      tmp = endp;
 
       if (p->inside_paren_p)
 	tmp = skip_spaces_const (tmp);
@@ -667,9 +676,11 @@ stap_parse_single_operand (struct stap_parse_info *p)
     {
       /* We are dealing with a numeric constant.  */
       long number;
+      char *endp;
 
       p->arg += const_prefix_len;
-      number = strtol (p->arg, (char **) &p->arg, 10);
+      number = strtol (p->arg, &endp, 10);
+      p->arg = endp;
 
       write_exp_elt_opcode (OP_LONG);
       write_exp_elt_type (builtin_type (gdbarch)->builtin_long);

Re: [COMMIT PATCH] More invalid pointer to pointer conversions.

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> I think the ada_decode_symbol change is perhaps the one that could be
Pedro> surprising.

The obj_section removal series tripped over this too :)

Pedro> With that out of the way, I think the patch ends up being pretty
Pedro> obvious.

I agree, looks good to me.

Pedro> -      displacement = strtol (tmp, (char **) &tmp, 10);
Pedro> +      displacement = strtol (tmp, &endp, 10);
Pedro> +      tmp = endp;

I saw this in Keith's patch, too, and I was wondering if we should have
a strtol_const convenience function.

Tom

Re: [COMMIT PATCH] More invalid pointer to pointer conversions.

[Pedro Alves]

On 03/13/2013 05:38 PM, Tom Tromey wrote:

> Pedro> -      displacement = strtol (tmp, (char **) &tmp, 10);
> Pedro> +      displacement = strtol (tmp, &endp, 10);
> Pedro> +      tmp = endp;
> 
> I saw this in Keith's patch, too, and I was wondering if we should have
> a strtol_const convenience function.

Yeah, I wondered the same.  I don't mind either way, actually.

( Read, I don't want it enough to add it myself :-) )

-- 
Pedro Alves

Re: [COMMIT PATCH] More invalid pointer to pointer conversions.

[Pedro Alves]

On 03/13/2013 05:55 PM, Pedro Alves wrote:
> On 03/13/2013 05:38 PM, Tom Tromey wrote:
> 
>> Pedro> -      displacement = strtol (tmp, (char **) &tmp, 10);
>> Pedro> +      displacement = strtol (tmp, &endp, 10);
>> Pedro> +      tmp = endp;
>>
>> I saw this in Keith's patch, too, and I was wondering if we should have
>> a strtol_const convenience function.
> 
> Yeah, I wondered the same.  I don't mind either way, actually.
> 
> ( Read, I don't want it enough to add it myself :-) )
> 

One thought occurred to me now though.

Every place that is doing:

  char *tmp;
  long l = strtol (tmp, &tmp, 0);

or:

  (const)? char *tmp;
  char *endp;
  displacement = strtol (tmp, &endp, 10);
  tmp = endp;

or the potential:

  const char *tmp;
  long l = strtol_const (tmp, &tmp, 0);

all suffer from the same problem -- they're not
really checking for strtol junk input / overflow.
That'd always go:

  l = strtol (tmp, &endp, 10);
  // --> here <--
  tmp = endp;

Given that for proper error handling you always need
a separate endp, strtol_const doesn't feel like it adds
much if anything in practice.

Perhaps instead we should either fix all the strtol
call sites for error handling, or even come up with
(a) throwing variant(s).  See e.g.,
xml_parse_unsigned_integer and gdb_xml_parse_ulongest
for possible interfaces.  (I note ERANGE handling is
missing there too).

-- 
Pedro Alves

Re: [COMMIT PATCH] More invalid pointer to pointer conversions.

[Tom Tromey]

Pedro> Perhaps instead we should either fix all the strtol
Pedro> call sites for error handling, or even come up with
Pedro> (a) throwing variant(s).  See e.g.,
Pedro> xml_parse_unsigned_integer and gdb_xml_parse_ulongest
Pedro> for possible interfaces.  (I note ERANGE handling is
Pedro> missing there too).

Yeah, I think a checking version would be an improvement.

Tom