Re: [RFA] ARI fixes: Remove some sprintf calls

Re: [RFA] ARI fixes: Remove some sprintf calls

[Andreas Schwab]

"Pierre Muller" <pierre.muller@ics-cnrs.unistra.fr> writes:

> Index: charset.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/charset.c,v
> retrieving revision 1.50
> diff -u -p -r1.50 charset.c
> --- charset.c	17 Apr 2012 15:47:09 -0000	1.50
> +++ charset.c	8 Nov 2012 15:03:14 -0000
> @@ -531,7 +531,7 @@ convert_between_encodings (const char *f
>  		  {
>  		    char octal[5];
>  
> -		    sprintf (octal, "\\%.3o", *inp & 0xff);
> +		    xsnprintf (octal, 5, "\\%.3o", *inp & 0xff);

sizeof(octal)?

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [RFA] ARI fixes: Remove some sprintf calls

[Tom Tromey]

>>>>> "Pierre" == Pierre Muller <pierre.muller@ics-cnrs.unistra.fr> writes:

Pierre>   I do not commit this as obvious because,
Pierre> I had to add some local variables to simplify 
Pierre> things for several cases.

Pierre>   Is this OK?

I like Andreas' suggestion.

Also:

Pierre> +++ windows-nat.c	8 Nov 2012 15:03:15 -0000
Pierre> @@ -613,6 +613,7 @@ struct safe_symbol_file_add_args
Pierre>  struct lm_info
Pierre>  {
Pierre>    LPVOID load_addr;
Pierre> +  unsigned long text_offset;
Pierre>  };
 
Pierre>  static struct so_list solib_start, *solib_end;
Pierre> @@ -778,6 +779,7 @@ windows_make_so (const char *name, LPVOI
Pierre>      }
Pierre>  #endif
 
Pierre> +
Pierre>    return so;
Pierre>  }

These hunks don't seem related to the patch.

Pierre> @@ -1227,11 +1229,18 @@ handle_exception (struct target_waitstat
Pierre>      default:
Pierre>        /* Treat unhandled first chance exceptions specially.  */
Pierre>        if (current_event.u.Exception.dwFirstChance)
Pierre> -	return -1;
Pierre> -      printf_unfiltered ("gdb: unknown target exception 0x%08lx at %s\n",
Pierre> -	current_event.u.Exception.ExceptionRecord.ExceptionCode,
Pierre> -	host_address_to_string (
Pierre> -	  current_event.u.Exception.ExceptionRecord.ExceptionAddress));
Pierre> +	{
Pierre> +	  DEBUG_EXCEPT (("gdb: Unknown FirstChance exception 0x%08lx at %s\n",
Pierre> +	    current_event.u.Exception.ExceptionRecord.ExceptionCode,
Pierre> +	    host_address_to_string (
Pierre> +	      current_event.u.Exception.ExceptionRecord.ExceptionAddress)));
Pierre> + 	  return -1;
Pierre> +	}
Pierre> +      else
Pierre> +	printf_unfiltered ("gdb: unknown target exception 0x%08lx at %s\n",
Pierre> +	  current_event.u.Exception.ExceptionRecord.ExceptionCode,
Pierre> +	  host_address_to_string (
Pierre> +	    current_event.u.Exception.ExceptionRecord.ExceptionAddress));

This one either.

Tom

[RFA-v2] ARI fixes: Remove some sprintf calls

[Pierre Muller]

[-- Attachment #1: Type: text/plain, Size: 2499 bytes --]



> -----Message d'origine-----
> De : gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] De la part de Tom Tromey
> Envoyé : jeudi 8 novembre 2012 22:35
> À : Pierre Muller
> Cc : gdb-patches@sourceware.org
> Objet : Re: [RFA] ARI fixes: Remove some sprintf calls
> 
> >>>>> "Pierre" == Pierre Muller <pierre.muller@ics-cnrs.unistra.fr>
writes:
> 
> Pierre>   I do not commit this as obvious because,
> Pierre> I had to add some local variables to simplify
> Pierre> things for several cases.
> 
> Pierre>   Is this OK?
> 
> I like Andreas' suggestion.

  Me too, implemented below...

Thanks for the suggestion.
It's of course much better to have the size explicitly only at
one location, to avoid problems later.

 
> Also:
> 
> Pierre> +++ windows-nat.c	8 Nov 2012 15:03:15 -0000
> Pierre> @@ -613,6 +613,7 @@ struct safe_symbol_file_add_args
> Pierre>  struct lm_info
> Pierre>  {
> Pierre>    LPVOID load_addr;
> Pierre> +  unsigned long text_offset;
> Pierre>  };
> 
> Pierre>  static struct so_list solib_start, *solib_end;
> Pierre> @@ -778,6 +779,7 @@ windows_make_so (const char *name, LPVOI
> Pierre>      }
> Pierre>  #endif
> 
> Pierre> +
> Pierre>    return so;
> Pierre>  }
> 
> These hunks don't seem related to the patch.
  
  I am lucky not to have committed it as obvious...
Once again, I forgot that I had local, unsubmitted changes
lying around.

Hopefully fixes in this second version.

Is that one OK?

Pierre Muller


2012-11-11  Pierre Muller  <muller@sourceware.org>

        ARI fixes: Avoid sprintf function use rule.
        * charset.c (convert_between_encodings): Use xsnprintf.
        * cli-out.c (cli_field_int): Likewise.
        * cp-namespace.c (cp_lookup_nested_symbol): Likewise.
        * expprint.c (op_name_standard): Likewise.
        * frv-tdep.c (set_variant_num_gprs): Likewise.
        (set_variant_num_fprs): Likewise.
        * m68hc11-tdep.c (m68hc11_initialize_register_info): Likewise.
        * nto-tdep.c (nto_find_and_open_solib): Likewise.
        (nto_init_solib_absolute_prefix): Likewise.
        * source.c (init_source_path): Likewise.
        (print_source_lines_base): Likewise.
        * valprint.c (print_wchar): Likewise.
        * mi/mi-out.c (mi_field_int): Likewise.
        windows-nat.c (windows_pid_to_exec_file): Likewise.
        (windows_create_inferior): Likewise.
        (_initialize_check_for_gdb_ini): Likewise.

[-- Attachment #2: sprintf-v1.patch --]
[-- Type: application/octet-stream, Size: 12535 bytes --]

projecttype:gdb
revision:HEAD
email:muller@ics.u-strasbg.fr

2012-11-11  Pierre Muller  <muller@sourceware.org>

	ARI fixes: Avoid sprintf function use rule.
	* charset.c (convert_between_encodings): Use xsnprintf.
	* cli-out.c (cli_field_int): Likewise.
	* cp-namespace.c (cp_lookup_nested_symbol): Likewise.
	* expprint.c (op_name_standard): Likewise.
	* frv-tdep.c (set_variant_num_gprs): Likewise.
	(set_variant_num_fprs): Likewise.
	* m68hc11-tdep.c (m68hc11_initialize_register_info): Likewise.
	* nto-tdep.c (nto_find_and_open_solib): Likewise.
	(nto_init_solib_absolute_prefix): Likewise.
	* source.c (init_source_path): Likewise.
	(print_source_lines_base): Likewise.
	* valprint.c (print_wchar): Likewise.
	* mi/mi-out.c (mi_field_int): Likewise.
	windows-nat.c (windows_pid_to_exec_file): Likewise.
	(windows_create_inferior): Likewise.
	(_initialize_check_for_gdb_ini): Likewise.

Index: charset.c
===================================================================
RCS file: /cvs/src/src/gdb/charset.c,v
retrieving revision 1.50
diff -u -p -r1.50 charset.c
--- charset.c	17 Apr 2012 15:47:09 -0000	1.50
+++ charset.c	11 Nov 2012 22:42:46 -0000
@@ -531,7 +531,7 @@ convert_between_encodings (const char *f
 		  {
 		    char octal[5];
 
-		    sprintf (octal, "\\%.3o", *inp & 0xff);
+		    xsnprintf (octal, sizeof (octal), "\\%.3o", *inp & 0xff);
 		    obstack_grow_str (output, octal);
 
 		    ++inp;
Index: cli-out.c
===================================================================
RCS file: /cvs/src/src/gdb/cli-out.c,v
retrieving revision 1.34
diff -u -p -r1.34 cli-out.c
--- cli-out.c	4 Jan 2012 08:17:00 -0000	1.34
+++ cli-out.c	11 Nov 2012 22:42:46 -0000
@@ -139,7 +139,7 @@ cli_field_int (struct ui_out *uiout, int
 
   if (data->suppress_output)
     return;
-  sprintf (buffer, "%d", value);
+  xsnprintf (buffer, sizeof (buffer), "%d", value);
 
   /* Always go through the function pointer (virtual function call).
      We may have been extended.  */
Index: cp-namespace.c
===================================================================
RCS file: /cvs/src/src/gdb/cp-namespace.c,v
retrieving revision 1.58
diff -u -p -r1.58 cp-namespace.c
--- cp-namespace.c	13 Jun 2012 16:10:07 -0000	1.58
+++ cp-namespace.c	11 Nov 2012 22:42:46 -0000
@@ -688,6 +688,7 @@ cp_lookup_nested_symbol (struct type *pa
 	   just like members of namespaces; in particular,
 	   lookup_symbol_namespace works when looking them up.  */
 
+	int size;
 	const char *parent_name = type_name_no_tag_or_error (saved_parent_type);
 	struct symbol *sym
 	  = cp_lookup_symbol_in_namespace (parent_name, nested_name,
@@ -703,9 +704,9 @@ cp_lookup_nested_symbol (struct type *pa
 	   namespace seach is is already not C++ compliant and more
 	   assumptions could make it too magic.  */
 
-	concatenated_name = alloca (strlen (parent_name) + 2
-				    + strlen (nested_name) + 1);
-	sprintf (concatenated_name, "%s::%s",
+	size = strlen (parent_name) + 2 + strlen (nested_name) + 1;
+	concatenated_name = alloca (size);
+	xsnprintf (concatenated_name, size, "%s::%s",
 		 parent_name, nested_name);
 	sym = lookup_static_symbol_aux (concatenated_name, VAR_DOMAIN);
 	if (sym != NULL)
Index: expprint.c
===================================================================
RCS file: /cvs/src/src/gdb/expprint.c,v
retrieving revision 1.62
diff -u -p -r1.62 expprint.c
--- expprint.c	2 Nov 2012 00:14:37 -0000	1.62
+++ expprint.c	11 Nov 2012 22:42:46 -0000
@@ -670,7 +670,7 @@ op_name_standard (enum exp_opcode opcode
       {
 	static char buf[30];
 
-	sprintf (buf, "<unknown %d>", opcode);
+	xsnprintf (buf, sizeof (buf), "<unknown %d>", opcode);
 	return buf;
       }
 #define OP(name)	\
Index: frv-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/frv-tdep.c,v
retrieving revision 1.138
diff -u -p -r1.138 frv-tdep.c
--- frv-tdep.c	18 May 2012 21:02:47 -0000	1.138
+++ frv-tdep.c	11 Nov 2012 22:42:46 -0000
@@ -233,7 +233,7 @@ set_variant_num_gprs (struct gdbarch_tde
     {
       char buf[20];
 
-      sprintf (buf, "gr%d", r);
+      xsnprintf (buf, sizeof (buf), "gr%d", r);
       var->register_names[first_gpr_regnum + r] = xstrdup (buf);
     }
 }
@@ -252,7 +252,7 @@ set_variant_num_fprs (struct gdbarch_tde
     {
       char buf[20];
 
-      sprintf (buf, "fr%d", r);
+      xsnprintf (buf, sizeof (buf), "fr%d", r);
       var->register_names[first_fpr_regnum + r] = xstrdup (buf);
     }
 }
Index: m68hc11-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/m68hc11-tdep.c,v
retrieving revision 1.140
diff -u -p -r1.140 m68hc11-tdep.c
--- m68hc11-tdep.c	25 Sep 2012 12:48:53 -0000	1.140
+++ m68hc11-tdep.c	11 Nov 2012 22:42:47 -0000
@@ -247,7 +247,7 @@ m68hc11_initialize_register_info (void)
     {
       char buf[10];
 
-      sprintf (buf, "_.d%d", i - SOFT_D1_REGNUM + 1);
+      xsnprintf (buf, sizeof (buf), "_.d%d", i - SOFT_D1_REGNUM + 1);
       m68hc11_get_register_info (&soft_regs[i], buf);
     }
 
Index: nto-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/nto-tdep.c,v
retrieving revision 1.46
diff -u -p -r1.46 nto-tdep.c
--- nto-tdep.c	9 Nov 2012 19:58:00 -0000	1.46
+++ nto-tdep.c	11 Nov 2012 22:42:47 -0000
@@ -89,7 +89,7 @@ nto_find_and_open_solib (char *solib, un
   char *buf, *arch_path, *nto_root, *endian;
   const char *base;
   const char *arch;
-  int ret;
+  int archlen, len, ret;
 #define PATH_FMT \
   "%s/lib:%s/usr/lib:%s/usr/photon/lib:%s/usr/photon/dll:%s/lib/dll"
 
@@ -116,20 +116,22 @@ nto_find_and_open_solib (char *solib, un
 
   /* In case nto_root is short, add strlen(solib)
      so we can reuse arch_path below.  */
-  arch_path =
-    alloca (strlen (nto_root) + strlen (arch) + strlen (endian) + 2 +
-	    strlen (solib));
-  sprintf (arch_path, "%s/%s%s", nto_root, arch, endian);
-
-  buf = alloca (strlen (PATH_FMT) + strlen (arch_path) * 5 + 1);
-  sprintf (buf, PATH_FMT, arch_path, arch_path, arch_path, arch_path,
-	   arch_path);
+
+  archlen = strlen (nto_root) + strlen (arch) + strlen (endian) + 2
+	+ strlen (solib);
+  arch_path = alloca (archlen);
+  xsnprintf (arch_path, archlen, "%s/%s%s", nto_root, arch, endian);
+
+  len = strlen (PATH_FMT) + strlen (arch_path) * 5 + 1;
+  buf = alloca (len);
+  xsnprintf (buf, len, PATH_FMT, arch_path, arch_path, arch_path, arch_path,
+	     arch_path);
 
   base = lbasename (solib);
   ret = openp (buf, 1, base, o_flags, temp_pathname);
   if (ret < 0 && base != solib)
     {
-      sprintf (arch_path, "/%s", solib);
+      xsnprintf (arch_path, archlen, "/%s", solib);
       ret = open (arch_path, o_flags, 0);
       if (temp_pathname)
 	{
@@ -170,9 +172,9 @@ nto_init_solib_absolute_prefix (void)
 	       == BFD_ENDIAN_BIG ? "be" : "le";
     }
 
-  sprintf (arch_path, "%s/%s%s", nto_root, arch, endian);
+  xsnprintf (arch_path, sizeof (arch_path), "%s/%s%s", nto_root, arch, endian);
 
-  sprintf (buf, "set solib-absolute-prefix %s", arch_path);
+  xsnprintf (buf, sizeof (buf), "set solib-absolute-prefix %s", arch_path);
   execute_command (buf, 0);
 }
 
Index: source.c
===================================================================
RCS file: /cvs/src/src/gdb/source.c,v
retrieving revision 1.140
diff -u -p -r1.140 source.c
--- source.c	9 Nov 2012 13:23:32 -0000	1.140
+++ source.c	11 Nov 2012 22:42:48 -0000
@@ -384,7 +384,7 @@ init_source_path (void)
 {
   char buf[20];
 
-  sprintf (buf, "$cdir%c$cwd", DIRNAME_SEPARATOR);
+  xsnprintf (buf, sizeof (buf), "$cdir%c$cwd", DIRNAME_SEPARATOR);
   source_path = xstrdup (buf);
   forget_cached_source_info ();
 }
@@ -1292,8 +1292,9 @@ print_source_lines_base (struct symtab *
 
       if (!noerror)
 	{
-	  char *name = alloca (strlen (s->filename) + 100);
-	  sprintf (name, "%d\t%s", line, s->filename);
+	  int len = strlen (s->filename) + 100;
+	  char *name = alloca (len);
+	  xsnprintf (name, len, "%d\t%s", line, s->filename);
 	  print_sys_errmsg (name, errno);
 	}
       else
@@ -1344,13 +1345,13 @@ print_source_lines_base (struct symtab *
       if (c == EOF)
 	break;
       last_line_listed = current_source_line;
-      sprintf (buf, "%d\t", current_source_line++);
+      xsnprintf (buf, sizeof (buf), "%d\t", current_source_line++);
       ui_out_text (uiout, buf);
       do
 	{
 	  if (c < 040 && c != '\t' && c != '\n' && c != '\r')
 	    {
-	      sprintf (buf, "^%c", c + 0100);
+	      xsnprintf (buf, sizeof (buf), "^%c", c + 0100);
 	      ui_out_text (uiout, buf);
 	    }
 	  else if (c == 0177)
@@ -1367,7 +1368,7 @@ print_source_lines_base (struct symtab *
 	    }
 	  else
 	    {
-	      sprintf (buf, "%c", c);
+	      xsnprintf (buf, sizeof (buf), "%c", c);
 	      ui_out_text (uiout, buf);
 	    }
 	}
Index: valprint.c
===================================================================
RCS file: /cvs/src/src/gdb/valprint.c,v
retrieving revision 1.130
diff -u -p -r1.130 valprint.c
--- valprint.c	10 Nov 2012 20:19:00 -0000	1.130
+++ valprint.c	11 Nov 2012 22:42:49 -0000
@@ -1974,9 +1974,10 @@ print_wchar (gdb_wint_t w, const gdb_byt
 		/* If the value fits in 3 octal digits, print it that
 		   way.  Otherwise, print it as a hex escape.  */
 		if (value <= 0777)
-		  sprintf (octal, "\\%.3o", (int) (value & 0777));
+		  xsnprintf (octal, sizeof (octal), "\\%.3o",
+			     (int) (value & 0777));
 		else
-		  sprintf (octal, "\\x%lx", (long) value);
+		  xsnprintf (octal, sizeof (octal), "\\x%lx", (long) value);
 		append_string_as_wide (octal, output);
 	      }
 	    /* If we somehow have extra bytes, print them now.  */
@@ -1984,7 +1985,7 @@ print_wchar (gdb_wint_t w, const gdb_byt
 	      {
 		char octal[5];
 
-		sprintf (octal, "\\%.3o", orig[i] & 0xff);
+		xsnprintf (octal, sizeof (octal), "\\%.3o", orig[i] & 0xff);
 		append_string_as_wide (octal, output);
 		++i;
 	      }
Index: mi/mi-out.c
===================================================================
RCS file: /cvs/src/src/gdb/mi/mi-out.c,v
retrieving revision 1.43
diff -u -p -r1.43 mi-out.c
--- mi/mi-out.c	6 Mar 2012 22:48:53 -0000	1.43
+++ mi/mi-out.c	11 Nov 2012 22:42:49 -0000
@@ -198,7 +198,7 @@ mi_field_int (struct ui_out *uiout, int 
   if (data->suppress_output)
     return;
 
-  sprintf (buffer, "%d", value);
+  xsnprintf (buffer, sizeof (buffer), "%d", value);
   mi_field_string (uiout, fldno, width, alignment, fldname, buffer);
 }
 
Index: windows-nat.c
===================================================================
RCS file: /cvs/src/src/gdb/windows-nat.c,v
retrieving revision 1.235
diff -u -p -r1.235 windows-nat.c
--- windows-nat.c	9 Nov 2012 19:58:02 -0000	1.235
+++ windows-nat.c	11 Nov 2012 22:51:49 -0000
@@ -1895,7 +1895,7 @@ windows_pid_to_exec_file (int pid)
   /* Try to find exe name as symlink target of /proc/<pid>/exe.  */
   int nchars;
   char procexe[sizeof ("/proc/4294967295/exe")];
-  sprintf (procexe, "/proc/%u/exe", pid);
+  xsnprintf (procexe, sizeof (procexe), "/proc/%u/exe", pid);
   nchars = readlink (procexe, path, sizeof(path));
   if (nchars > 0 && nchars < sizeof (path))
     {
@@ -2093,10 +2093,10 @@ windows_create_inferior (struct target_o
       cygallargs = (wchar_t *) alloca (len * sizeof (wchar_t));
       swprintf (cygallargs, len, L" -c 'exec %s %s'", exec_file, allargs);
 #else
-      cygallargs = (char *)
-	alloca (sizeof (" -c 'exec  '") + strlen (exec_file)
-				    + strlen (allargs) + 2);
-      sprintf (cygallargs, " -c 'exec %s %s'", exec_file, allargs);
+      len = sizeof (" -c 'exec  '") + strlen (exec_file)
+	    + strlen (allargs) + 2;
+      cygallargs = (char *) alloca (len);
+      xsnprintf (cygallargs, len, " -c 'exec %s %s'", exec_file, allargs);
 #endif
       toexec = shell;
       flags |= DEBUG_PROCESS;
@@ -2724,8 +2724,8 @@ _initialize_check_for_gdb_ini (void)
 	{
 	  int len = strlen (oldini);
 	  char *newini = alloca (len + 1);
-	  sprintf (newini, "%.*s.gdbinit",
-	    (int) (len - (sizeof ("gdb.ini") - 1)), oldini);
+	  xsnprintf (newini, len + 1, "%.*s.gdbinit",
+		     (int) (len - (sizeof ("gdb.ini") - 1)), oldini);
 	  warning (_("obsolete '%s' found. Rename to '%s'."), oldini, newini);
 	}
     }

Re: [RFA-v2] ARI fixes: Remove some sprintf calls

[Joel Brobecker]

> 2012-11-11  Pierre Muller  <muller@sourceware.org>
> 
>         ARI fixes: Avoid sprintf function use rule.
>         * charset.c (convert_between_encodings): Use xsnprintf.
>         * cli-out.c (cli_field_int): Likewise.
>         * cp-namespace.c (cp_lookup_nested_symbol): Likewise.
>         * expprint.c (op_name_standard): Likewise.
>         * frv-tdep.c (set_variant_num_gprs): Likewise.
>         (set_variant_num_fprs): Likewise.
>         * m68hc11-tdep.c (m68hc11_initialize_register_info): Likewise.
>         * nto-tdep.c (nto_find_and_open_solib): Likewise.
>         (nto_init_solib_absolute_prefix): Likewise.
>         * source.c (init_source_path): Likewise.
>         (print_source_lines_base): Likewise.
>         * valprint.c (print_wchar): Likewise.
>         * mi/mi-out.c (mi_field_int): Likewise.
>         windows-nat.c (windows_pid_to_exec_file): Likewise.
>         (windows_create_inferior): Likewise.
>         (_initialize_check_for_gdb_ini): Likewise.

This is a nice improvement.

I'm wondering if there is any way to provoke an error if we ever
use sprintf again... I am asking because I know that it's easy to
ignore the ARI. I kind of remember not being able to do that, but
perhaps wrong memory. #poison, maybe?

A few nits... I think it's good to go, after the trivial nits are
corrected.

Thanks for doing this!

> Index: nto-tdep.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/nto-tdep.c,v
> retrieving revision 1.46
> diff -u -p -r1.46 nto-tdep.c
> --- nto-tdep.c	9 Nov 2012 19:58:00 -0000	1.46
> +++ nto-tdep.c	11 Nov 2012 22:42:47 -0000
> @@ -89,7 +89,7 @@ nto_find_and_open_solib (char *solib, un
>    char *buf, *arch_path, *nto_root, *endian;
>    const char *base;
>    const char *arch;
> -  int ret;
> +  int archlen, len, ret;

Can we use "arch_len" instead if "archlen". This would be more in
line with "arch_path", and since both are related... It's also more
in line with the GNU Coding Style, I believe, where we use underscores
to separate words in identifier names.

> +  archlen = strlen (nto_root) + strlen (arch) + strlen (endian) + 2
> +	+ strlen (solib);

Another tiny nit. GCS require that we put the RHS expression inside
parentheses (to help tools format it correctly). Thus:

  arch_len = (strlen (nto_root) + strlen (arch) + strlen (endian) + 2
              + strlen (solib));


>        if (!noerror)
>  	{
> -	  char *name = alloca (strlen (s->filename) + 100);
> -	  sprintf (name, "%d\t%s", line, s->filename);
> +	  int len = strlen (s->filename) + 100;
> +	  char *name = alloca (len);
> +	  xsnprintf (name, len, "%d\t%s", line, s->filename);

Can you add an empty line between variable defs and the rest
of the code?

> +++ windows-nat.c	11 Nov 2012 22:51:49 -0000
> @@ -1895,7 +1895,7 @@ windows_pid_to_exec_file (int pid)
>    /* Try to find exe name as symlink target of /proc/<pid>/exe.  */
>    int nchars;
>    char procexe[sizeof ("/proc/4294967295/exe")];
> -  sprintf (procexe, "/proc/%u/exe", pid);
> +  xsnprintf (procexe, sizeof (procexe), "/proc/%u/exe", pid);

Same here, please?

>  #else
> -      cygallargs = (char *)
> -	alloca (sizeof (" -c 'exec  '") + strlen (exec_file)
> -				    + strlen (allargs) + 2);
> -      sprintf (cygallargs, " -c 'exec %s %s'", exec_file, allargs);
> +      len = sizeof (" -c 'exec  '") + strlen (exec_file)
> +	    + strlen (allargs) + 2;

Can you enclose the RSH expression between parentheses?

-- 
Joel

Re: [RFA-v2] ARI fixes: Remove some sprintf calls

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

Joel> I'm wondering if there is any way to provoke an error if we ever
Joel> use sprintf again... I am asking because I know that it's easy to
Joel> ignore the ARI. I kind of remember not being able to do that, but
Joel> perhaps wrong memory. #poison, maybe?

If all uses are removed we could probably use #pragma GCC poison in
defs.h.

Tom

RE: [RFA-v2] ARI fixes: Remove some sprintf calls

[Pierre Muller]

[-- Attachment #1: Type: text/plain, Size: 4772 bytes --]



> -----Message d'origine-----
> De : gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] De la part de Joel Brobecker
> Envoyé : lundi 12 novembre 2012 17:08
> À : Pierre Muller
> Cc : 'Tom Tromey'; 'Andreas Schwab'; gdb-patches@sourceware.org
> Objet : Re: [RFA-v2] ARI fixes: Remove some sprintf calls
> 
> > 2012-11-11  Pierre Muller  <muller@sourceware.org>
> >
> >         ARI fixes: Avoid sprintf function use rule.
> >         * charset.c (convert_between_encodings): Use xsnprintf.
> >         * cli-out.c (cli_field_int): Likewise.
> >         * cp-namespace.c (cp_lookup_nested_symbol): Likewise.
> >         * expprint.c (op_name_standard): Likewise.
> >         * frv-tdep.c (set_variant_num_gprs): Likewise.
> >         (set_variant_num_fprs): Likewise.
> >         * m68hc11-tdep.c (m68hc11_initialize_register_info): Likewise.
> >         * nto-tdep.c (nto_find_and_open_solib): Likewise.
> >         (nto_init_solib_absolute_prefix): Likewise.
> >         * source.c (init_source_path): Likewise.
> >         (print_source_lines_base): Likewise.
> >         * valprint.c (print_wchar): Likewise.
> >         * mi/mi-out.c (mi_field_int): Likewise.
> >         windows-nat.c (windows_pid_to_exec_file): Likewise.
> >         (windows_create_inferior): Likewise.
> >         (_initialize_check_for_gdb_ini): Likewise.
> 
> This is a nice improvement.
  Thanks.
 
> I'm wondering if there is any way to provoke an error if we ever
> use sprintf again... I am asking because I know that it's easy to
> ignore the ARI. I kind of remember not being able to do that, but
> perhaps wrong memory. #poison, maybe?

  As long as we can get also compilers that don't know about #poison
to also generate an error if sprint is used, everything is fine.

 
> A few nits... I think it's good to go, after the trivial nits are
> corrected.

  I committed the patch after the modification you
suggested below.
 
> Thanks for doing this!
> 
> > Index: nto-tdep.c
> > ===================================================================
> > RCS file: /cvs/src/src/gdb/nto-tdep.c,v
> > retrieving revision 1.46
> > diff -u -p -r1.46 nto-tdep.c
> > --- nto-tdep.c	9 Nov 2012 19:58:00 -0000	1.46
> > +++ nto-tdep.c	11 Nov 2012 22:42:47 -0000
> > @@ -89,7 +89,7 @@ nto_find_and_open_solib (char *solib, un
> >    char *buf, *arch_path, *nto_root, *endian;
> >    const char *base;
> >    const char *arch;
> > -  int ret;
> > +  int archlen, len, ret;
> 
> Can we use "arch_len" instead if "archlen". This would be more in
> line with "arch_path", and since both are related... It's also more
> in line with the GNU Coding Style, I believe, where we use underscores
> to separate words in identifier names.
  Done. 
> > +  archlen = strlen (nto_root) + strlen (arch) + strlen (endian) + 2
> > +	+ strlen (solib);
> 
> Another tiny nit. GCS require that we put the RHS expression inside
> parentheses (to help tools format it correctly). Thus:

  I hope to correctly understand that you mean
that this is for multi-line expressions only...

 
>   arch_len = (strlen (nto_root) + strlen (arch) + strlen (endian) + 2
>               + strlen (solib));
 Done. 
> 
> >        if (!noerror)
> >  	{
> > -	  char *name = alloca (strlen (s->filename) + 100);
> > -	  sprintf (name, "%d\t%s", line, s->filename);
> > +	  int len = strlen (s->filename) + 100;
> > +	  char *name = alloca (len);
> > +	  xsnprintf (name, len, "%d\t%s", line, s->filename);
> 
> Can you add an empty line between variable defs and the rest
> of the code?

  I didn't add them if they were not present...
There are of course lots of other places were those are missing,
but you are right that we should correct those error
if we modify the corresponding code.
 
> > +++ windows-nat.c	11 Nov 2012 22:51:49 -0000
> > @@ -1895,7 +1895,7 @@ windows_pid_to_exec_file (int pid)
> >    /* Try to find exe name as symlink target of /proc/<pid>/exe.  */
> >    int nchars;
> >    char procexe[sizeof ("/proc/4294967295/exe")];
> > -  sprintf (procexe, "/proc/%u/exe", pid);
> > +  xsnprintf (procexe, sizeof (procexe), "/proc/%u/exe", pid);
> 
> Same here, please?
Done. 
> >  #else
> > -      cygallargs = (char *)
> > -	alloca (sizeof (" -c 'exec  '") + strlen (exec_file)
> > -				    + strlen (allargs) + 2);
> > -      sprintf (cygallargs, " -c 'exec %s %s'", exec_file, allargs);
> > +      len = sizeof (" -c 'exec  '") + strlen (exec_file)
> > +	    + strlen (allargs) + 2;
> 
> Can you enclose the RSH expression between parentheses?

  Final committed patch is attached.
ChangeLog is unchanged.


Thanks for the approval,

Pierre Muller
as ARI maintainer

[-- Attachment #2: sprintf-v2.patch --]
[-- Type: application/octet-stream, Size: 12563 bytes --]

projecttype:gdb
revision:HEAD
email:muller@ics.u-strasbg.fr

2012-11-13  Pierre Muller  <muller@sourceware.org>

	ARI fixes: Avoid sprintf function use rule.
	* charset.c (convert_between_encodings): Use xsnprintf.
	* cli-out.c (cli_field_int): Likewise.
	* cp-namespace.c (cp_lookup_nested_symbol): Likewise.
	* expprint.c (op_name_standard): Likewise.
	* frv-tdep.c (set_variant_num_gprs): Likewise.
	(set_variant_num_fprs): Likewise.
	* m68hc11-tdep.c (m68hc11_initialize_register_info): Likewise.
	* nto-tdep.c (nto_find_and_open_solib): Likewise.
	(nto_init_solib_absolute_prefix): Likewise.
	* source.c (init_source_path): Likewise.
	(print_source_lines_base): Likewise.
	* valprint.c (print_wchar): Likewise.
	* mi/mi-out.c (mi_field_int): Likewise.
	windows-nat.c (windows_pid_to_exec_file): Likewise.
	(windows_create_inferior): Likewise.
	(_initialize_check_for_gdb_ini): Likewise.


Index: charset.c
===================================================================
RCS file: /cvs/src/src/gdb/charset.c,v
retrieving revision 1.50
diff -u -p -r1.50 charset.c
--- charset.c	17 Apr 2012 15:47:09 -0000	1.50
+++ charset.c	13 Nov 2012 08:34:33 -0000
@@ -531,7 +531,7 @@ convert_between_encodings (const char *f
 		  {
 		    char octal[5];
 
-		    sprintf (octal, "\\%.3o", *inp & 0xff);
+		    xsnprintf (octal, sizeof (octal), "\\%.3o", *inp & 0xff);
 		    obstack_grow_str (output, octal);
 
 		    ++inp;
Index: cli-out.c
===================================================================
RCS file: /cvs/src/src/gdb/cli-out.c,v
retrieving revision 1.34
diff -u -p -r1.34 cli-out.c
--- cli-out.c	4 Jan 2012 08:17:00 -0000	1.34
+++ cli-out.c	13 Nov 2012 08:34:34 -0000
@@ -139,7 +139,7 @@ cli_field_int (struct ui_out *uiout, int
 
   if (data->suppress_output)
     return;
-  sprintf (buffer, "%d", value);
+  xsnprintf (buffer, sizeof (buffer), "%d", value);
 
   /* Always go through the function pointer (virtual function call).
      We may have been extended.  */
Index: cp-namespace.c
===================================================================
RCS file: /cvs/src/src/gdb/cp-namespace.c,v
retrieving revision 1.58
diff -u -p -r1.58 cp-namespace.c
--- cp-namespace.c	13 Jun 2012 16:10:07 -0000	1.58
+++ cp-namespace.c	13 Nov 2012 08:34:34 -0000
@@ -688,6 +688,7 @@ cp_lookup_nested_symbol (struct type *pa
 	   just like members of namespaces; in particular,
 	   lookup_symbol_namespace works when looking them up.  */
 
+	int size;
 	const char *parent_name = type_name_no_tag_or_error (saved_parent_type);
 	struct symbol *sym
 	  = cp_lookup_symbol_in_namespace (parent_name, nested_name,
@@ -703,9 +704,9 @@ cp_lookup_nested_symbol (struct type *pa
 	   namespace seach is is already not C++ compliant and more
 	   assumptions could make it too magic.  */
 
-	concatenated_name = alloca (strlen (parent_name) + 2
-				    + strlen (nested_name) + 1);
-	sprintf (concatenated_name, "%s::%s",
+	size = strlen (parent_name) + 2 + strlen (nested_name) + 1;
+	concatenated_name = alloca (size);
+	xsnprintf (concatenated_name, size, "%s::%s",
 		 parent_name, nested_name);
 	sym = lookup_static_symbol_aux (concatenated_name, VAR_DOMAIN);
 	if (sym != NULL)
Index: expprint.c
===================================================================
RCS file: /cvs/src/src/gdb/expprint.c,v
retrieving revision 1.62
diff -u -p -r1.62 expprint.c
--- expprint.c	2 Nov 2012 00:14:37 -0000	1.62
+++ expprint.c	13 Nov 2012 08:34:34 -0000
@@ -670,7 +670,7 @@ op_name_standard (enum exp_opcode opcode
       {
 	static char buf[30];
 
-	sprintf (buf, "<unknown %d>", opcode);
+	xsnprintf (buf, sizeof (buf), "<unknown %d>", opcode);
 	return buf;
       }
 #define OP(name)	\
Index: frv-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/frv-tdep.c,v
retrieving revision 1.138
diff -u -p -r1.138 frv-tdep.c
--- frv-tdep.c	18 May 2012 21:02:47 -0000	1.138
+++ frv-tdep.c	13 Nov 2012 08:34:35 -0000
@@ -233,7 +233,7 @@ set_variant_num_gprs (struct gdbarch_tde
     {
       char buf[20];
 
-      sprintf (buf, "gr%d", r);
+      xsnprintf (buf, sizeof (buf), "gr%d", r);
       var->register_names[first_gpr_regnum + r] = xstrdup (buf);
     }
 }
@@ -252,7 +252,7 @@ set_variant_num_fprs (struct gdbarch_tde
     {
       char buf[20];
 
-      sprintf (buf, "fr%d", r);
+      xsnprintf (buf, sizeof (buf), "fr%d", r);
       var->register_names[first_fpr_regnum + r] = xstrdup (buf);
     }
 }
Index: m68hc11-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/m68hc11-tdep.c,v
retrieving revision 1.140
diff -u -p -r1.140 m68hc11-tdep.c
--- m68hc11-tdep.c	25 Sep 2012 12:48:53 -0000	1.140
+++ m68hc11-tdep.c	13 Nov 2012 08:34:35 -0000
@@ -247,7 +247,7 @@ m68hc11_initialize_register_info (void)
     {
       char buf[10];
 
-      sprintf (buf, "_.d%d", i - SOFT_D1_REGNUM + 1);
+      xsnprintf (buf, sizeof (buf), "_.d%d", i - SOFT_D1_REGNUM + 1);
       m68hc11_get_register_info (&soft_regs[i], buf);
     }
 
Index: nto-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/nto-tdep.c,v
retrieving revision 1.46
diff -u -p -r1.46 nto-tdep.c
--- nto-tdep.c	9 Nov 2012 19:58:00 -0000	1.46
+++ nto-tdep.c	13 Nov 2012 08:34:35 -0000
@@ -89,7 +89,7 @@ nto_find_and_open_solib (char *solib, un
   char *buf, *arch_path, *nto_root, *endian;
   const char *base;
   const char *arch;
-  int ret;
+  int arch_len, len, ret;
 #define PATH_FMT \
   "%s/lib:%s/usr/lib:%s/usr/photon/lib:%s/usr/photon/dll:%s/lib/dll"
 
@@ -116,20 +116,22 @@ nto_find_and_open_solib (char *solib, un
 
   /* In case nto_root is short, add strlen(solib)
      so we can reuse arch_path below.  */
-  arch_path =
-    alloca (strlen (nto_root) + strlen (arch) + strlen (endian) + 2 +
-	    strlen (solib));
-  sprintf (arch_path, "%s/%s%s", nto_root, arch, endian);
-
-  buf = alloca (strlen (PATH_FMT) + strlen (arch_path) * 5 + 1);
-  sprintf (buf, PATH_FMT, arch_path, arch_path, arch_path, arch_path,
-	   arch_path);
+
+  arch_len = (strlen (nto_root) + strlen (arch) + strlen (endian) + 2
+	      + strlen (solib));
+  arch_path = alloca (arch_len);
+  xsnprintf (arch_path, arch_len, "%s/%s%s", nto_root, arch, endian);
+
+  len = strlen (PATH_FMT) + strlen (arch_path) * 5 + 1;
+  buf = alloca (len);
+  xsnprintf (buf, len, PATH_FMT, arch_path, arch_path, arch_path, arch_path,
+	     arch_path);
 
   base = lbasename (solib);
   ret = openp (buf, 1, base, o_flags, temp_pathname);
   if (ret < 0 && base != solib)
     {
-      sprintf (arch_path, "/%s", solib);
+      xsnprintf (arch_path, arch_len, "/%s", solib);
       ret = open (arch_path, o_flags, 0);
       if (temp_pathname)
 	{
@@ -170,9 +172,9 @@ nto_init_solib_absolute_prefix (void)
 	       == BFD_ENDIAN_BIG ? "be" : "le";
     }
 
-  sprintf (arch_path, "%s/%s%s", nto_root, arch, endian);
+  xsnprintf (arch_path, sizeof (arch_path), "%s/%s%s", nto_root, arch, endian);
 
-  sprintf (buf, "set solib-absolute-prefix %s", arch_path);
+  xsnprintf (buf, sizeof (buf), "set solib-absolute-prefix %s", arch_path);
   execute_command (buf, 0);
 }
 
Index: source.c
===================================================================
RCS file: /cvs/src/src/gdb/source.c,v
retrieving revision 1.140
diff -u -p -r1.140 source.c
--- source.c	9 Nov 2012 13:23:32 -0000	1.140
+++ source.c	13 Nov 2012 08:34:36 -0000
@@ -384,7 +384,7 @@ init_source_path (void)
 {
   char buf[20];
 
-  sprintf (buf, "$cdir%c$cwd", DIRNAME_SEPARATOR);
+  xsnprintf (buf, sizeof (buf), "$cdir%c$cwd", DIRNAME_SEPARATOR);
   source_path = xstrdup (buf);
   forget_cached_source_info ();
 }
@@ -1292,8 +1292,10 @@ print_source_lines_base (struct symtab *
 
       if (!noerror)
 	{
-	  char *name = alloca (strlen (s->filename) + 100);
-	  sprintf (name, "%d\t%s", line, s->filename);
+	  int len = strlen (s->filename) + 100;
+	  char *name = alloca (len);
+
+	  xsnprintf (name, len, "%d\t%s", line, s->filename);
 	  print_sys_errmsg (name, errno);
 	}
       else
@@ -1344,13 +1346,13 @@ print_source_lines_base (struct symtab *
       if (c == EOF)
 	break;
       last_line_listed = current_source_line;
-      sprintf (buf, "%d\t", current_source_line++);
+      xsnprintf (buf, sizeof (buf), "%d\t", current_source_line++);
       ui_out_text (uiout, buf);
       do
 	{
 	  if (c < 040 && c != '\t' && c != '\n' && c != '\r')
 	    {
-	      sprintf (buf, "^%c", c + 0100);
+	      xsnprintf (buf, sizeof (buf), "^%c", c + 0100);
 	      ui_out_text (uiout, buf);
 	    }
 	  else if (c == 0177)
@@ -1367,7 +1369,7 @@ print_source_lines_base (struct symtab *
 	    }
 	  else
 	    {
-	      sprintf (buf, "%c", c);
+	      xsnprintf (buf, sizeof (buf), "%c", c);
 	      ui_out_text (uiout, buf);
 	    }
 	}
Index: valprint.c
===================================================================
RCS file: /cvs/src/src/gdb/valprint.c,v
retrieving revision 1.130
diff -u -p -r1.130 valprint.c
--- valprint.c	10 Nov 2012 20:19:00 -0000	1.130
+++ valprint.c	13 Nov 2012 08:34:36 -0000
@@ -1974,9 +1974,10 @@ print_wchar (gdb_wint_t w, const gdb_byt
 		/* If the value fits in 3 octal digits, print it that
 		   way.  Otherwise, print it as a hex escape.  */
 		if (value <= 0777)
-		  sprintf (octal, "\\%.3o", (int) (value & 0777));
+		  xsnprintf (octal, sizeof (octal), "\\%.3o",
+			     (int) (value & 0777));
 		else
-		  sprintf (octal, "\\x%lx", (long) value);
+		  xsnprintf (octal, sizeof (octal), "\\x%lx", (long) value);
 		append_string_as_wide (octal, output);
 	      }
 	    /* If we somehow have extra bytes, print them now.  */
@@ -1984,7 +1985,7 @@ print_wchar (gdb_wint_t w, const gdb_byt
 	      {
 		char octal[5];
 
-		sprintf (octal, "\\%.3o", orig[i] & 0xff);
+		xsnprintf (octal, sizeof (octal), "\\%.3o", orig[i] & 0xff);
 		append_string_as_wide (octal, output);
 		++i;
 	      }
Index: mi/mi-out.c
===================================================================
RCS file: /cvs/src/src/gdb/mi/mi-out.c,v
retrieving revision 1.43
diff -u -p -r1.43 mi-out.c
--- mi/mi-out.c	6 Mar 2012 22:48:53 -0000	1.43
+++ mi/mi-out.c	13 Nov 2012 08:34:37 -0000
@@ -198,7 +198,7 @@ mi_field_int (struct ui_out *uiout, int 
   if (data->suppress_output)
     return;
 
-  sprintf (buffer, "%d", value);
+  xsnprintf (buffer, sizeof (buffer), "%d", value);
   mi_field_string (uiout, fldno, width, alignment, fldname, buffer);
 }
 
Index: windows-nat.c
===================================================================
RCS file: /cvs/src/src/gdb/windows-nat.c,v
retrieving revision 1.235
diff -u -p -r1.235 windows-nat.c
--- windows-nat.c	9 Nov 2012 19:58:02 -0000	1.235
+++ windows-nat.c	13 Nov 2012 09:28:25 -0000
@@ -1895,7 +1895,8 @@ windows_pid_to_exec_file (int pid)
   /* Try to find exe name as symlink target of /proc/<pid>/exe.  */
   int nchars;
   char procexe[sizeof ("/proc/4294967295/exe")];
-  sprintf (procexe, "/proc/%u/exe", pid);
+
+  xsnprintf (procexe, sizeof (procexe), "/proc/%u/exe", pid);
   nchars = readlink (procexe, path, sizeof(path));
   if (nchars > 0 && nchars < sizeof (path))
     {
@@ -2093,10 +2094,10 @@ windows_create_inferior (struct target_o
       cygallargs = (wchar_t *) alloca (len * sizeof (wchar_t));
       swprintf (cygallargs, len, L" -c 'exec %s %s'", exec_file, allargs);
 #else
-      cygallargs = (char *)
-	alloca (sizeof (" -c 'exec  '") + strlen (exec_file)
-				    + strlen (allargs) + 2);
-      sprintf (cygallargs, " -c 'exec %s %s'", exec_file, allargs);
+      len = (sizeof (" -c 'exec  '") + strlen (exec_file)
+	     + strlen (allargs) + 2);
+      cygallargs = (char *) alloca (len);
+      xsnprintf (cygallargs, len, " -c 'exec %s %s'", exec_file, allargs);
 #endif
       toexec = shell;
       flags |= DEBUG_PROCESS;
@@ -2724,8 +2725,9 @@ _initialize_check_for_gdb_ini (void)
 	{
 	  int len = strlen (oldini);
 	  char *newini = alloca (len + 1);
-	  sprintf (newini, "%.*s.gdbinit",
-	    (int) (len - (sizeof ("gdb.ini") - 1)), oldini);
+
+	  xsnprintf (newini, len + 1, "%.*s.gdbinit",
+		     (int) (len - (sizeof ("gdb.ini") - 1)), oldini);
 	  warning (_("obsolete '%s' found. Rename to '%s'."), oldini, newini);
 	}
     }

Re: [RFA-v2] ARI fixes: Remove some sprintf calls

[Joel Brobecker]

> > Another tiny nit. GCS require that we put the RHS expression inside
> > parentheses (to help tools format it correctly). Thus:
> 
>   I hope to correctly understand that you mean
> that this is for multi-line expressions only...

Yes, that's correct :).

-- 
Joel