This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [RFA-v2] ARI fixes: Remove some sprintf calls
- From: Joel Brobecker <brobecker at adacore dot com>
- To: Pierre Muller <pierre dot muller at ics-cnrs dot unistra dot fr>
- Cc: 'Tom Tromey' <tromey at redhat dot com>, 'Andreas Schwab' <schwab at linux-m68k dot org>, gdb-patches at sourceware dot org
- Date: Mon, 12 Nov 2012 08:07:41 -0800
- Subject: Re: [RFA-v2] ARI fixes: Remove some sprintf calls
- References: <4297.05661158568$1352387430@news.gmane.org> <87d2znhj7t.fsf@fleche.redhat.com> <000001cdc061$dad58740$908095c0$@muller@ics-cnrs.unistra.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.
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