Re: /dev/fd/n bug in gawk 3.1.5

[Aharon Robbins]

> Date: Mon, 03 Jul 2006 16:54:19 -0400
> From: "Andrew J. Schorr" <address@hidden>
> Subject: Re: /dev/fd/n bug in gawk 3.1.5
> To: Aharon Robbins <address@hidden>
>
> Hi,
>
> Note that the pid test was failing silently: "make check" was
> reporting "ALL TESTS PASSED" even though gawk was SEGV'ing
> in the pid test.

I noticed this on RHEL 4 system at work where the test failed
noisily. I'm guessing a difference in versions of bash...

> Attached is a patch to the pid test
> that fixes this problem (i.e. the test will now fail noisily).
> The problem was that the SEGV was aborting the script
> prematurely, and leaving a zero-length output file, which was
> precisely correct.  The fix was to make sure that test/pid.awk
> produces some output.  That way we get a comparison failure
> if the output file is empty.

Actually, the correct fix is to keep gawk from SEGV-ing in the
first place. Below is full diff of io.c relative to 3.1.5.

I will probably incorporate your changes also as extra insurance.

Thanks,

Arnold
-----------------------------------------------------
Mon Jul  3 00:27:59 2006  Arnold D. Robbins  <address@hidden>

        * io.c (INTERNAL_HANDLE): New constant for use by `iop_alloc'
        when allocating an internal IOBUF.
        (pidopen, useropen): Use it.
        (iop_alloc): Add check for it and just return iop.

Sun Jun 18 22:27:25 2006  Arnold D. Robbins  <address@hidden>

        Repair internal names like /dev/user, /dev/pid, as well as /dev/fd/N,
        which have been broken for a long time but noone noticed.

        * io.c (is_internal): new macro to check for internal file like 
`/dev/user'.
        (spec_setup): Reduce to two parameters, allocate logic is always true.
        Add IOP_NO_FREE to flag.
        (pidopen, useropen): Return `IOBUF *' instead of int. Fix
        logic to test if `iop' parameter is NULL and if so to allocate it.
        (specfdopen,): Return `IOBUF *' instead of int. Fix
        logic to test if `iop' parameter is NULL and if so to allocate it.
        Don't set IOP_NO_FREE in flag.
        (iop_open): Remove `IOBUF iob' field from `struct internal' and its use
        and the use of `spec_setup' from the code here. Change the check in the
        call to the open function to look for NULL.
        (get_a_record): Use `is_internal' in initial check for filling the
        buffer to not try to call `read' on internal files. If true, set
        the IOP_AT_EOF in the flag and return EOF.

Fri Aug 12 13:10:33 2005  Arnold D. Robbins  <address@hidden>

        * io.c (iop_alloc): Only free `iop' if it was malloc'ed in
        the first place.

--- ../gawk-3.1.5/io.c  2005-07-26 21:07:43.000000000 +0300
+++ io.c        2006-07-03 00:27:51.000000000 +0300
@@ -103,6 +103,9 @@
 
 typedef enum { CLOSE_ALL, CLOSE_TO, CLOSE_FROM } two_way_close_type;
 
+/* For internal files, /dev/pid, etc. */
+#define INTERNAL_HANDLE                (-42)
+
 /* Several macros make the code a bit clearer:                              */
 /*                                                                          */
 /*                                                                          */
@@ -110,6 +113,7 @@
 #define at_eof(iop)     ((iop->flag & IOP_AT_EOF) != 0)
 #define has_no_data(iop)        (iop->dataend == NULL)
 #define no_data_left(iop)      (iop->off >= iop->dataend)
+#define is_internal(iop) ((iop->flag & IOP_IS_INTERNAL) != 0)
 /* The key point to the design is to split out the code that searches through 
*/
 /* a buffer looking for the record and the terminator into separate routines, 
*/
 /* with a higher-level routine doing the reading of data and buffer 
management. */
@@ -163,10 +167,10 @@
 static int gawk_pclose P((struct redirect *rp));
 static int do_pathopen P((const char *file));
 static int str2mode P((const char *mode));
-static void spec_setup P((IOBUF *iop, int len, int allocate));
-static int specfdopen P((IOBUF *iop, const char *name, const char *mode));
-static int pidopen P((IOBUF *iop, const char *name, const char *mode));
-static int useropen P((IOBUF *iop, const char *name, const char *mode));
+static void spec_setup P((IOBUF *iop, int len));
+static IOBUF *specfdopen P((IOBUF *iop, const char *name, const char *mode));
+static IOBUF *pidopen P((IOBUF *iop, const char *name, const char *mode));
+static IOBUF *useropen P((IOBUF *iop, const char *name, const char *mode));
 static int two_way_open P((const char *str, struct redirect *rp));
 static int pty_vs_pipe P((const char *command));
 
@@ -1422,30 +1426,24 @@
 /* spec_setup --- setup an IOBUF for a special internal file */
 
 static void
-spec_setup(IOBUF *iop, int len, int allocate)
+spec_setup(IOBUF *iop, int len)
 {
        char *cp;
 
-       if (allocate) {
-               emalloc(cp, char *, len+2, "spec_setup");
-               iop->buf = cp;
-       } else {
-               len = strlen(iop->buf);
-               iop->buf[len++] = '\n'; /* get_a_record clobbered it */
-               iop->buf[len] = '\0';   /* just in case */
-       }
+       emalloc(cp, char *, len+2, "spec_setup");
+       iop->buf = cp;
        iop->off = iop->buf;
        iop->count = 0;
        iop->size = len;
        iop->end = iop->buf + len;
        iop->dataend = iop->end;
        iop->fd = -1;
-       iop->flag = IOP_IS_INTERNAL | IOP_AT_START;
+       iop->flag = IOP_IS_INTERNAL | IOP_AT_START | IOP_NO_FREE;
 }
 
 /* specfdopen --- open an fd special file */
 
-static int
+static IOBUF *
 specfdopen(IOBUF *iop, const char *name, const char *mode)
 {
        int fd;
@@ -1453,17 +1451,14 @@
 
        fd = devopen(name, mode);
        if (fd == INVALID_HANDLE)
-               return INVALID_HANDLE;
-       tp = iop_alloc(fd, name, NULL);
+               return NULL;
+       tp = iop_alloc(fd, name, iop);
        if (tp == NULL) {
                /* don't leak fd's */
                close(fd);
-               return INVALID_HANDLE;
+               return NULL;
        }
-       *iop = *tp;
-       iop->flag |= IOP_NO_FREE;
-       free(tp);
-       return 0;
+       return tp;
 }
 
 #ifdef GETPGRP_VOID
@@ -1474,7 +1469,7 @@
 
 /* pidopen --- "open" /dev/pid, /dev/ppid, and /dev/pgrpid */
 
-static int
+static IOBUF *
 pidopen(IOBUF *iop, const char *name, const char *mode ATTRIBUTE_UNUSED)
 {
        char tbuf[BUFSIZ];
@@ -1483,6 +1478,12 @@
 
        warning(_("use `PROCINFO[\"%s\"]' instead of `%s'"), cp, name);
 
+       if (iop == NULL) {
+               iop = iop_alloc(INTERNAL_HANDLE, name, iop);
+               if (iop == NULL)
+                       return NULL;
+       }
+
        if (name[6] == 'g')
                sprintf(tbuf, "%d\n", (int) getpgrp(getpgrp_arg()));
        else if (name[6] == 'i')
@@ -1490,9 +1491,9 @@
        else
                sprintf(tbuf, "%d\n", (int) getppid());
        i = strlen(tbuf);
-       spec_setup(iop, i, TRUE);
+       spec_setup(iop, i);
        strcpy(iop->buf, tbuf);
-       return 0;
+       return iop;
 }
 
 /* useropen --- "open" /dev/user */
@@ -1507,7 +1508,7 @@
  * supplementary group set.
  */
 
-static int
+static IOBUF *
 useropen(IOBUF *iop, const char *name ATTRIBUTE_UNUSED, const char *mode 
ATTRIBUTE_UNUSED)
 {
        char tbuf[BUFSIZ], *cp;
@@ -1515,6 +1516,12 @@
 
        warning(_("use `PROCINFO[...]' instead of `/dev/user'"));
 
+       if (iop == NULL) {
+               iop = iop_alloc(INTERNAL_HANDLE, name, iop);
+               if (iop == NULL)
+                       return NULL;
+       }
+
        sprintf(tbuf, "%d %d %d %d", (int) getuid(), (int) geteuid(), (int) 
getgid(), (int) getegid());
 
        cp = tbuf + strlen(tbuf);
@@ -1529,9 +1536,9 @@
        *cp++ = '\0';
 
        i = strlen(tbuf);
-       spec_setup(iop, i, TRUE);
+       spec_setup(iop, i);
        strcpy(iop->buf, tbuf);
-       return 0;
+       return iop;
 }
 
 /* iop_open --- handle special and regular files for input */
@@ -1544,8 +1551,7 @@
        static struct internal {
                const char *name;
                int compare;
-               int (*fp) P((IOBUF *, const char *, const char *));
-               IOBUF iob;
+               IOBUF *(*fp) P((IOBUF *, const char *, const char *));
        } table[] = {
                { "/dev/fd/",           8,      specfdopen },
                { "/dev/stdin",         10,     specfdopen },
@@ -1570,12 +1576,7 @@
 
                for (i = 0; i < devcount; i++) {
                        if (STREQN(name, table[i].name, table[i].compare)) {
-                               iop = & table[i].iob;
-
-                               if (iop->buf != NULL) {
-                                       spec_setup(iop, 0, FALSE);
-                                       return iop;
-                               } else if ((*table[i].fp)(iop, name, mode) == 0)
+                               if ((iop = (*table[i].fp)(iop, name, mode)) != 
NULL)
                                        return iop;
                                else {
                                        warning(_("could not open `%s', mode 
`%s'"),
@@ -2480,9 +2481,12 @@
 {
        struct stat sbuf;
        struct open_hook *oh;
+       int iop_malloced = FALSE;
 
-       if (iop == NULL)
+       if (iop == NULL) {
                emalloc(iop, IOBUF *, sizeof(IOBUF), "iop_alloc");
+               iop_malloced = TRUE;
+       }
        memset(iop, '\0', sizeof(IOBUF));
        iop->flag = 0;
        iop->fd = fd;
@@ -2494,8 +2498,12 @@
                        break;
        }
 
+       if (iop->fd == INTERNAL_HANDLE)
+               return iop;
+
        if (iop->fd == INVALID_HANDLE) {
-               free(iop);
+               if (iop_malloced)
+                       free(iop);
                return NULL;
        }
        if (isatty(iop->fd))
@@ -2503,7 +2511,7 @@
        iop->readsize = iop->size = optimal_bufsize(iop->fd, & sbuf);
        iop->sbuf = sbuf;
        if (do_lint && S_ISREG(sbuf.st_mode) && sbuf.st_size == 0)
-                       lintwarn(_("data file `%s' is empty"), name);
+               lintwarn(_("data file `%s' is empty"), name);
        errno = 0;
        iop->count = iop->scanoff = 0;
        emalloc(iop->buf, char *, iop->size += 2, "iop_alloc");
@@ -2906,6 +2914,10 @@
 
         /* <fill initial buffer>=                                              
     */
         if (has_no_data(iop) || no_data_left(iop)) {
+               if (is_internal(iop)) {
+                       iop->flag |= IOP_AT_EOF;
+                       return EOF;
+               }
                 iop->count = read(iop->fd, iop->buf, iop->readsize);
                 if (iop->count == 0) {
                         iop->flag |= IOP_AT_EOF;