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

["Pierre Muller" <pierre.muller@ics-cnrs.unistra.fr>]

[-- 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);
 	}
     }