Date:	Wed, 26 Nov 1997 13:16:46 +0200 (IST)
From:	Eli Zaretskii <eliz AT is DOT elta DOT co DOT il>
To:	Molnar Laszlo <molnarl AT cdata DOT tvnet DOT hu>
cc:	djgpp-workers AT delorie DOT com,
	Hans-Bernhard Broeker <broeker AT physik DOT rwth-aachen DOT de>,
	DJ Delorie <dj AT delorie DOT com>
Subject:	Re: popen/pclose update
In-Reply-To:	<347AA34A.4DFC829F@cdata.tvnet.hu>
Message-ID:	<Pine.SUN.3.91.971126131540.960B-100000@is>
MIME-Version:	1.0

On Tue, 25 Nov 1997, Molnar Laszlo wrote:

> 1, l1 might not be freed when something failed

A simple patch (below) will fix this.

> 2, l1->temp_name might not be freed when something failed

In the original code, l1->temp_name is not malloc'ed, so it doesn't
need to be freed.

> 3, l1 might be appended to the list with bogus elements when something
>    failed

Also simple to fix.

> 4, It always allocates memory for l1->command even when not needed.

I'm not sure this isn't a feature.  What if we will need to look at
the command in `pclose' one day, even for popen("foo", "r")?

> 5, pipe_list.fd is not needed.

So?

> Some problems with pclose():
> 
> 1, It fails when it is called with a not popen-ed pointer (broken and
>    too complex list handling)

I think it crashes because it unconditionally dereferences pl, and
only if no matching call to `popen' was done.  Nothing a simple patch
(below) can't fix.

> 2, Because of the popen() problems remove (l1->temp_name) can be a
>    problem.

Is it okay with the patched code below?

> Patching these one by one would require changing at least 30-40 lines,
> that's why I thought that I should made a general cleanup.

The patch below adds 15 lines to the original version and deletes
one.

> My reimplementation does the same things as the original code when
> everything works ok, but I try to handle the possible errors too.

Your code calls `strdup'.  You can't do that in `popen', because
`popen' and `pclose' are POSIX functions whereas `strdup' is not.

> Reorganizing the code also made it shorter (~80 lines vs ~120 lines)
> and cleaner (IMHO). And the autodeleting feature of the temporary
> files is just 3 extra lines.

I will let DJ decide whether to accept your version (after you replace
the calls to `strdup') or the patch below.


diff -c src/libc/posix/stdio/popen.c~0 src/libc/posix/stdio/popen.c
*** src/libc/posix/stdio/popen.c~0	Fri Oct 10 02:04:08 1997
--- src/libc/posix/stdio/popen.c	Wed Nov 26 12:20:08 1997
***************
*** 95,101 ****
--- 95,105 ----
    l1->exit_status = -1;
    strcpy (l1->mode, md);
    if (tmpnam (l1->temp_name) == NULL)
+   {
+     pl = l1->next;
+     free (l1);
      return NULL;
+   }
  
    /* if can save the program name, build temp file */
    if ((l1->command = malloc(strlen(cm)+1)))
***************
*** 129,136 ****
        else
          /* unknown mode */
          l1->fp = NULL;
    }
-   return l1->fp;              /* return == NULL ? ERROR : OK */
  }
  
  int
--- 133,147 ----
        else
          /* unknown mode */
          l1->fp = NULL;
+ 
+     return l1->fp;
+   }
+   else
+   {
+     pl = l1->next;
+     free (l1);
+     return NULL;
    }
  }
  
  int
***************
*** 138,143 ****
--- 149,157 ----
  {
    struct pipe_list *l1, *l2;    /* list pointers */
    int retval=0;			/* function return value */
+ 
+   if (!pl)
+     return -1;
  
    /* if pointer is first node */
    if (pl->fp == pp)

- Raw text -