mg compile-mode stderr handling

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

mg compile-mode stderr handling

Vincent Labrecque
Hi,

I think that since compile_mode() decides to use popen() it probably
should also be expected to deal correctly with stderr.

Fixes a display screwup when a compile_mode() program (lint, grep, gid,
etc) writes to stderr, which could also be fixed by sprinkling missing
2>&1 all over the place but for the reason stated above I think this is
wisest.

Of course given the time it's been since I sent any diff, this should be
checked by many many eyes.

-vincent

Index: grep.c
===================================================================
RCS file: /usr/cvs/src/usr.bin/mg/grep.c,v
retrieving revision 1.27
diff -u -r1.27 grep.c
--- grep.c 3 Apr 2006 00:19:32 -0000 1.27
+++ grep.c 3 May 2006 09:56:26 -0000
@@ -137,7 +137,7 @@
  return (ABORT);
  else if (bufp[0] == '\0')
  return (FALSE);
- (void)snprintf(command, sizeof(command), "%s 2>&1", bufp);
+ (void)snprintf(command, sizeof(command), "%s", bufp);
 
  if ((bp = compile_mode("*lint*", command, path)) == NULL)
  return (FALSE);
@@ -175,7 +175,7 @@
  return (ABORT);
  (void)strlcpy(compile_last_command, bufp, sizeof(compile_last_command));
 
- (void)snprintf(command, sizeof(command), "%s 2>&1", bufp);
+ (void)snprintf(command, sizeof(command), "%s", bufp);
 
  if ((bp = compile_mode("*compile*", command, path)) == NULL)
  return (FALSE);
@@ -239,7 +239,7 @@
  return (ABORT);
  else if (bufp[0] == '\0')
  return (FALSE);
- (void)snprintf(command, sizeof(command), "gid %s", cprompt);
+ (void)snprintf(command, sizeof(command), "gid %s 2>&1", cprompt);
 
  if ((bp = compile_mode("*gid*", command, path)) == NULL)
  return (FALSE);
@@ -258,7 +258,7 @@
  char *buf;
  size_t len;
  int ret;
- char cwd[NFILEN];
+ char cwd[NFILEN], *quiet_command;
  char timestr[NTIME];
  time_t t;
 
@@ -276,10 +276,19 @@
  ewprintf("Can't change dir to %s", path);
  return (NULL);
  }
- if ((fpipe = popen(command, "r")) == NULL) {
+ if (asprintf(&quiet_command, "%s 2>&1", command) < 0) {
+ ewprintf("Out of memory");
+ return (NULL);
+ }
+
+ fpipe = popen(quiet_command, "r");
+ free(quiet_command);
+
+ if (fpipe == NULL) {
  ewprintf("Problem opening pipe");
  return (NULL);
  }
+
  /*
  * We know that our commands are nice and the last line will end with
  * a \n, so we don't need to try to deal with the last line problem

Reply | Threaded
Open this post in threaded view
|

Re: mg compile-mode stderr handling

Kjell-5
> I think that since compile_mode() decides to use popen() it probably
> should also be expected to deal correctly with stderr.

comments inline

> Index: grep.c
> ===================================================================
> RCS file: /usr/cvs/src/usr.bin/mg/grep.c,v
> retrieving revision 1.27
> diff -u -r1.27 grep.c
> --- grep.c 3 Apr 2006 00:19:32 -0000 1.27
> +++ grep.c 3 May 2006 09:56:26 -0000
> @@ -137,7 +137,7 @@
>   return (ABORT);
>   else if (bufp[0] == '\0')
>   return (FALSE);
> - (void)snprintf(command, sizeof(command), "%s 2>&1", bufp);
> + (void)snprintf(command, sizeof(command), "%s", bufp);

There's not really much point in copying this into command[] then.
passing bufp (or cprompt) is sufficient.

>  
>   if ((bp = compile_mode("*lint*", command, path)) == NULL)
>   return (FALSE);
> @@ -175,7 +175,7 @@
>   return (ABORT);
>   (void)strlcpy(compile_last_command, bufp, sizeof(compile_last_command));
>  
> - (void)snprintf(command, sizeof(command), "%s 2>&1", bufp);
> + (void)snprintf(command, sizeof(command), "%s", bufp);

ditto

>  
>   if ((bp = compile_mode("*compile*", command, path)) == NULL)
>   return (FALSE);
> @@ -239,7 +239,7 @@
>   return (ABORT);
>   else if (bufp[0] == '\0')
>   return (FALSE);
> - (void)snprintf(command, sizeof(command), "gid %s", cprompt);
> + (void)snprintf(command, sizeof(command), "gid %s 2>&1", cprompt);

The original would seem to be what you intended

>  
>   if ((bp = compile_mode("*gid*", command, path)) == NULL)
>   return (FALSE);
> @@ -258,7 +258,7 @@
>   char *buf;
>   size_t len;
>   int ret;
> - char cwd[NFILEN];
> + char cwd[NFILEN], *quiet_command;
>   char timestr[NTIME];
>   time_t t;
>  
> @@ -276,10 +276,19 @@
>   ewprintf("Can't change dir to %s", path);
>   return (NULL);
>   }
> - if ((fpipe = popen(command, "r")) == NULL) {
> + if (asprintf(&quiet_command, "%s 2>&1", command) < 0) {
> + ewprintf("Out of memory");
> + return (NULL);
> + }
> +
> + fpipe = popen(quiet_command, "r");
> + free(quiet_command);
> +
> + if (fpipe == NULL) {
>   ewprintf("Problem opening pipe");


I'd be tempted just to make this a static buffer (qcmd[NFILEN]) like
everything else in this particular file, though it makes little practical
difference.

-kj

Reply | Threaded
Open this post in threaded view
|

Re: mg compile-mode stderr handling

Han Boetes
Kjell Wooding wrote:
> I'd be tempted just to make this a static buffer (qcmd[NFILEN])
> like everything else in this particular file, though it makes
> little practical difference.

Static buffers make your binaries larger. ;-)



# Han

Reply | Threaded
Open this post in threaded view
|

Re: mg compile-mode stderr handling

Ted Unangst-2
On 5/3/06, Han Boetes <[hidden email]> wrote:
> Kjell Wooding wrote:
> > I'd be tempted just to make this a static buffer (qcmd[NFILEN])
> > like everything else in this particular file, though it makes
> > little practical difference.
>
> Static buffers make your binaries larger. ;-)

only if you're using an operating system without virtual memory...

bss is only a number in the file.

Reply | Threaded
Open this post in threaded view
|

Re: mg compile-mode stderr handling

Kjell-5
In reply to this post by Vincent Labrecque
> Fixes a display screwup when a compile_mode() program (lint, grep, gid,
> etc) writes to stderr, which could also be fixed by sprinkling missing
> 2>&1 all over the place but for the reason stated above I think this is
> wisest.

Here's a reworked diff, same goals as vincent, except I also
want to get rid of the silly magic numbers in there. If NFILEN isn't
enough length for you, shell out:

Index: grep.c
===================================================================
RCS file: /cvs/src/usr.bin/mg/grep.c,v
retrieving revision 1.28
diff -u -r1.28 grep.c
--- grep.c 2 May 2006 17:10:25 -0000 1.28
+++ grep.c 8 May 2006 21:07:26 -0000
@@ -83,7 +83,6 @@
 static int
 grep(int f, int n)
 {
- char command[NFILEN + 21];
  char cprompt[NFILEN], *bufp;
  struct buffer *bp;
  struct mgwin *wp;
@@ -94,9 +93,10 @@
  return (ABORT);
  else if (bufp[0] == '\0')
  return (FALSE);
- (void)snprintf(command, sizeof(command), "%s /dev/null", bufp);
+ if (strlcat(cprompt, " /dev/null", sizeof(cprompt)) >= sizeof(cprompt))
+ return (FALSE);
 
- if ((bp = compile_mode("*grep*", command)) == NULL)
+ if ((bp = compile_mode("*grep*", cprompt)) == NULL)
  return (FALSE);
  if ((wp = popbuf(bp)) == NULL)
  return (FALSE);
@@ -109,7 +109,6 @@
 static int
 xlint(int f, int n)
 {
- char command[NFILEN + 16];
  char cprompt[NFILEN], *bufp;
  struct buffer *bp;
  struct mgwin *wp;
@@ -120,9 +119,8 @@
  return (ABORT);
  else if (bufp[0] == '\0')
  return (FALSE);
- (void)snprintf(command, sizeof(command), "%s 2>&1", bufp);
 
- if ((bp = compile_mode("*lint*", command)) == NULL)
+ if ((bp = compile_mode("*lint*", cprompt)) == NULL)
  return (FALSE);
  if ((wp = popbuf(bp)) == NULL)
  return (FALSE);
@@ -135,7 +133,6 @@
 static int
 compile(int f, int n)
 {
- char command[NFILEN + 20];
  char cprompt[NFILEN], *bufp;
  struct buffer *bp;
  struct mgwin *wp;
@@ -150,9 +147,7 @@
  return (ABORT);
  (void)strlcpy(compile_last_command, bufp, sizeof(compile_last_command));
 
- (void)snprintf(command, sizeof(command), "%s 2>&1", bufp);
-
- if ((bp = compile_mode("*compile*", command)) == NULL)
+ if ((bp = compile_mode("*compile*", cprompt)) == NULL)
  return (FALSE);
  if ((wp = popbuf(bp)) == NULL)
  return (FALSE);
@@ -167,11 +162,11 @@
 static int
 gid(int f, int n)
 {
- char command[NFILEN + 20];
+ char command[NFILEN];
  char cprompt[NFILEN], c, *bufp;
  struct buffer *bp;
  struct mgwin *wp;
- int i, j;
+ int i, j, len;
 
  /* catch ([^\s(){}]+)[\s(){}]* */
 
@@ -206,7 +201,9 @@
  return (ABORT);
  else if (bufp[0] == '\0')
  return (FALSE);
- (void)snprintf(command, sizeof(command), "gid %s", cprompt);
+ len = snprintf(command, sizeof(command), "gid %s", cprompt);
+ if (len == -1 || len >= sizeof(command))
+ return (FALSE);
 
  if ((bp = compile_mode("*gid*", command)) == NULL)
  return (FALSE);
@@ -224,11 +221,15 @@
  FILE *fpipe;
  char *buf;
  size_t len;
- int ret;
- char cwd[NFILEN];
+ int ret, n;
+ char cwd[NFILEN], qcmd[NFILEN];
  char timestr[NTIME];
  time_t t;
 
+ n = snprintf(qcmd, sizeof(qcmd), "%s 2>&1", command);
+ if (n == -1 || n >= sizeof(qcmd))
+ return (NULL);
+
  bp = bfind(name, TRUE);
  if (bclear(bp) != TRUE)
  return (NULL);
@@ -236,7 +237,7 @@
  if (getbufcwd(bp->b_cwd, sizeof(bp->b_cwd)) != TRUE)
  return (NULL);
  addlinef(bp, "cd %s", bp->b_cwd);
- addline(bp, command);
+ addline(bp, qcmd);
  addline(bp, "");
 
  if (getcwd(cwd, sizeof(cwd)) == NULL)
@@ -245,7 +246,7 @@
  ewprintf("Can't change dir to %s", bp->b_cwd);
  return (NULL);
  }
- if ((fpipe = popen(command, "r")) == NULL) {
+ if ((fpipe = popen(qcmd, "r")) == NULL) {
  ewprintf("Problem opening pipe");
  return (NULL);
  }

Reply | Threaded
Open this post in threaded view
|

Re: mg compile-mode stderr handling

Han Boetes
Kjell Wooding wrote:
> - (void)snprintf(command, sizeof(command), "gid %s", cprompt);
> + len = snprintf(command, sizeof(command), "gid %s", cprompt);
> + if (len == -1 || len > = sizeof(command))
> + return (FALSE);

Shouldn't this be:

        if (len < 0 || len >= sizeof(command))

since sizeof returns an u_int and len is an int? We've had this
discussion before.

Now I don't think sizeof(command) will be able overflow an int,
because of the size of `command.' But it makes the code easier to
audit.



# Han