New ARI warning Sat May 28 01:53:47 UTC 2011

New ARI warning Sat May 28 01:53:47 UTC 2011

[GDB Administrator]

439a440
> gdb/linux-nat.c:2368: code: sprintf: Do not use sprintf, instead use xstrprintf
gdb/linux-nat.c:2368:  sprintf (buffer, ', lwp);

Re: New ARI warning Sat May 28 01:53:47 UTC 2011

[Jan Kratochvil]

On Sat, 28 May 2011 03:53:47 +0200, GDB Administrator wrote:
> 439a440
> > gdb/linux-nat.c:2368: code: sprintf: Do not use sprintf, instead use xstrprintf
> gdb/linux-nat.c:2368:  sprintf (buffer, ', lwp);

This is by me:
  char buffer[MAXPATHLEN];
  sprintf (buffer, "/proc/%ld/status", lwp);

I find the code perfectly correct, as I was told the ARI checks are only
differential I hope it can be kept as is.


Thanks,
Jan

Re: New ARI warning Sat May 28 01:53:47 UTC 2011

[Joel Brobecker]

> This is by me:
>   char buffer[MAXPATHLEN];
>   sprintf (buffer, "/proc/%ld/status", lwp);
> 
> I find the code perfectly correct, as I was told the ARI checks are only
> differential I hope it can be kept as is.

I reached a similar conclusion when I looked at it this morning.
We could make the code marginally better in the sense that we'd
remove the static buffer, but at the cost of making the code
a little more convoluted (once we malloc, we need to make sure
we always free, which probably means a cleanup, etc). So I also
vote for leaving the code as is.

That lead me to consider the removal of this rule.  But in the end,
I think it's useful to be reminded every time we use sprintf that
there is xtrsprintf. Since this hasn't produced too many false
positives, I think it's OK to keep it for now.

In the meantime, I think there is a way to say that this line is OK.
If you put /* ARI: sprintf */ on the sprintf line, that should take
care of the warning...

-- 
Joel

[commit] Re: New ARI warning Sat May 28 01:53:47 UTC 2011

[Jan Kratochvil]

On Mon, 30 May 2011 20:59:33 +0200, Joel Brobecker wrote:
> That lead me to consider the removal of this rule.  But in the end,
> I think it's useful to be reminded every time we use sprintf that
> there is xtrsprintf. Since this hasn't produced too many false
> positives, I think it's OK to keep it for now.

GCC already warns in appropriate cases for sprintf but I find the ARI rule
sure useful in general.


> If you put /* ARI: sprintf */ on the sprintf line, that should take
> care of the warning...

I did not know, thanks.


On Mon, 30 May 2011 21:16:04 +0200, Mark Kettenis wrote:
> Sorry, no.  Please replace this with a call to xsnprintf.

Done, when there are any concerns.

Checked in.


Thanks,
Jan


http://sourceware.org/ml/gdb-cvs/2011-05/msg00245.html

--- src/gdb/ChangeLog	2011/05/30 18:04:31	1.13065
+++ src/gdb/ChangeLog	2011/05/30 19:26:36	1.13066
@@ -1,3 +1,7 @@
+2011-05-30  Jan Kratochvil  <jan.kratochvil@redhat.com>
+
+	* linux-nat.c (linux_lwp_is_zombie): Use xsnprintf.
+
 2011-05-30  Pedro Alves  <pedro@codesourcery.com>
 
 	* continuations.h (continuation_ftype): Add `err' parameter.
--- src/gdb/linux-nat.c	2011/05/27 16:55:38	1.206
+++ src/gdb/linux-nat.c	2011/05/30 19:26:36	1.207
@@ -2365,7 +2365,7 @@
   FILE *procfile;
   int retval = 0;
 
-  sprintf (buffer, "/proc/%ld/status", lwp);
+  xsnprintf (buffer, sizeof (buffer), "/proc/%ld/status", lwp);
   procfile = fopen (buffer, "r");
   if (procfile == NULL)
     {

Re: [commit] Re: New ARI warning Sat May 28 01:53:47 UTC 2011

[Mark Kettenis]

> Date: Mon, 30 May 2011 21:28:42 +0200
> From: Jan Kratochvil <jan.kratochvil@redhat.com>
>
> On Mon, 30 May 2011 21:16:04 +0200, Mark Kettenis wrote:
> > Sorry, no.  Please replace this with a call to xsnprintf.
> 
> Done, when there are any concerns.
> 
> Checked in.

Thanks

Re: [commit] Re: New ARI warning Sat May 28 01:53:47 UTC 2011

[Joel Brobecker]

> --- src/gdb/linux-nat.c	2011/05/27 16:55:38	1.206
> +++ src/gdb/linux-nat.c	2011/05/30 19:26:36	1.207
> @@ -2365,7 +2365,7 @@
>    FILE *procfile;
>    int retval = 0;
>  
> -  sprintf (buffer, "/proc/%ld/status", lwp);
> +  xsnprintf (buffer, sizeof (buffer), "/proc/%ld/status", lwp);

Elegant (duh!). I'll update the ARI documentation to mention xsnprintf
as well - the xstrprintf suggestion really put me in target-lock mode,
and I failed to think beyond it...

-- 
Joel

[ARI/commit] enhance suggestion in "sprintf" rule (was: "Re: [commit] Re: New ARI warning Sat May 28 01:53:47 UTC 2011")

[Joel Brobecker]

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

> [...] I'll update the ARI documentation to mention xsnprintf
> as well - the xstrprintf suggestion really put me in target-lock mode,
> and I failed to think beyond it...

This is what I checked in.  Incidently, I noticed a typo, which
I also fixed (this is the second patch).

-- 
Joel

[-- Attachment #2: ari-sprintf.diff --]
[-- Type: text/x-diff, Size: 619 bytes --]

Index: gdb_ari.sh
===================================================================
RCS file: /cvs/gdbadmin/ss/gdb_ari.sh,v
retrieving revision 1.108
retrieving revision 1.109
diff -u -p -r1.108 -r1.109
--- gdb_ari.sh	21 Mar 2011 23:04:02 -0000	1.108
+++ gdb_ari.sh	30 May 2011 19:53:25 -0000	1.109
@@ -1068,7 +1068,7 @@ Replace set_register_cached() with nothi
 # or safely allocate a fresh buffer.
 
 BEGIN { doc["sprintf"] = "\
-Do not use sprintf, instead use xstrprintf"
+Do not use sprintf, instead use xsnprintf or xstrprintf"
     category["sprintf"] = ari_code
 }
 /(^|[^_[:alnum:]])sprintf[[:space:]]*\(/ {

[-- Attachment #3: ari-vsprint.diff --]
[-- Type: text/x-diff, Size: 803 bytes --]

Index: gdb_ari.sh
===================================================================
RCS file: /cvs/gdbadmin/ss/gdb_ari.sh,v
retrieving revision 1.109
retrieving revision 1.110
diff -u -p -r1.109 -r1.110
--- gdb_ari.sh	30 May 2011 19:53:25 -0000	1.109
+++ gdb_ari.sh	30 May 2011 19:55:37 -0000	1.110
@@ -1075,12 +1075,12 @@ Do not use sprintf, instead use xsnprint
     fail("sprintf")
 }
 
-BEGIN { doc["vsprint"] = "\
-Do not use vsprint(), instead use xstrvprintf"
-    category["vsprint"] = ari_regression
+BEGIN { doc["vsprintf"] = "\
+Do not use vsprintf(), instead use xstrvprintf"
+    category["vsprintf"] = ari_regression
 }
-/(^|[^_[:alnum:]])vsprint[[:space:]]*\(/ {
-    fail("vsprint")
+/(^|[^_[:alnum:]])vsprintf[[:space:]]*\(/ {
+    fail("vsprintf")
 }
 
 BEGIN { doc["asprintf"] = "\

RE: [ARI/commit] enhance suggestion in "sprintf" rule (was: "Re: [commit] Re: New ARI warning Sat May 28 01:53:47 UTC 2011")

[Pierre Muller]

> -----Message d'origine-----
> De : gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] De la part de Joel Brobecker
> Envoyé : lundi 30 mai 2011 21:59
> À : gdb-patches@sourceware.org
> Objet : [ARI/commit] enhance suggestion in "sprintf" rule (was: "Re:
> [commit] Re: New ARI warning Sat May 28 01:53:47 UTC 2011")
> 
> > [...] I'll update the ARI documentation to mention xsnprintf
> > as well - the xstrprintf suggestion really put me in target-lock mode,
> > and I failed to think beyond it...
> 
> This is what I checked in.  Incidently, I noticed a typo, which
> I also fixed (this is the second patch).

  Thanks for this ARI enhancement,
I did notice once that static buffers should use 
xsnprintf function instead of xstrprintf that allocate a
buffer using global memory, but forgot to change ARI doc
for this rule...

  I hope this will facilitate reduction of the number of 
occurrences of this warning:
  Currently, "Don't use sprintf" rule is found
184 times ...

  So please don't hesitate to fix those issues
under the OBVIOUS rule!

Pierre Muller
as ARI maintainer

Re: New ARI warning Sat May 28 01:53:47 UTC 2011

[Mark Kettenis]

> Date: Mon, 30 May 2011 20:34:55 +0200
> From: Jan Kratochvil <jan.kratochvil@redhat.com>
> 
> On Sat, 28 May 2011 03:53:47 +0200, GDB Administrator wrote:
> > 439a440
> > > gdb/linux-nat.c:2368: code: sprintf: Do not use sprintf, instead use xstrprintf
> > gdb/linux-nat.c:2368:  sprintf (buffer, ', lwp);
> 
> This is by me:
>   char buffer[MAXPATHLEN];
>   sprintf (buffer, "/proc/%ld/status", lwp);
> 
> I find the code perfectly correct, as I was told the ARI checks are only
> differential I hope it can be kept as is.

Sorry, no.  Please replace this with a call to xsnprintf.

While this particular call may be safe, people will have to check
again and again that it is whenever they audit the code in the future.
Replacing it with xsnprintf prevents that.