Sender: ml AT delorie DOT com
Message-ID: <347AA34A.4DFC829F@cdata.tvnet.hu>
Date: Tue, 25 Nov 1997 11:07:06 +0100
From: Molnar Laszlo <molnarl AT cdata DOT tvnet DOT hu>
MIME-Version: 1.0
To: Eli Zaretskii <eliz AT is DOT elta DOT co DOT il>
CC: djgpp-workers AT delorie DOT com,
        Hans-Bernhard Broeker <broeker AT physik DOT rwth-aachen DOT de>
Subject: Re: popen/pclose update
References: <Pine DOT SUN DOT 3 DOT 91 DOT 971124130225 DOT 23713B-100000 AT is>
Content-Type: text/plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
Precedence: bulk

Eli Zaretskii wrote:
> 
> On Fri, 21 Nov 1997 molnarl AT cdata DOT tvnet DOT hu wrote:
> 
> > It's time to update popen()/pclose() again :-) I've found a bug in
> > pclose: if you write something like this: pclose (stdin), you will
> > get a SIGSEGV.
> 
> This shouldn't require more than a simple change.  No need to rewrite
> everything, IMHO.
> What else is wrong in the original version?
 
And Hans-Bernhard Broeker wrote:
> Could explain in a bit more detail where, and why it 'seems unsafe'?  The
> current code has been working rather well for quite some time now, so I
> don't we should dismiss it for a new implementation on such short notice.
> In other words: if it's not actually *broken*, why replace it completely
> to fix it?

Some problems with popen():

1, l1 might not be freed when something failed
2, l1->temp_name might not be freed when something failed
3, l1 might be appended to the list with bogus elements when something
   failed
4, It always allocates memory for l1->command even when not needed.
5, pipe_list.fd is not needed.

Some problems with pclose():

1, It fails when it is called with a not popen-ed pointer (broken and
   too complex list handling)
2, Because of the popen() problems remove (l1->temp_name) can be a
   problem.

So I think the original implementation only works as expected while
nothing goes wrong. If an error appears in a wrong point, it does not
do the necessary cleanups.

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.

My reimplementation does the same things as the original code when
everything works ok, but I try to handle the possible errors too.
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.

Eli again:
> > However I have a question: if you look at the line with '!!!' in
> > pclose, could you tell me why I have to include that line? I don't
> > understand why fflush is not enough.
> 
> Not enough for what?  What exactly doesn't work when you omit that
> line?
> If the problem is that some of the output is not written to the
> temporary file, then the reason is that `fflush' only flushes the
> buffered output which is kept in libc.  It does not flush DOS internal
> buffers.  You need to call `fsync' or `close' to make all the data
> written to the file.

Yeah, that is my problem. If DOS has all the data in its internal
buffer, why it doesn't show them to the newly opened file? Is it a
feature?

>  Since there's nothing else to be written to the
> temporary file, closing it seems like a good idea, and that's what the
> original code did as well.  What's wrong with that?

Nothing wrong, but I have to use a close on the file handle, then fclose
on that closed file. That's why I wrote "it's ugly". 

Laszlo