[PATCH sed] ck_fclose should unlink *before* calling do_ck_fclose.

[NeilBrown]

If do_ck_fclose gets an error from fclose() it will call
panic() which will try to close everything on the list.
As the file which fclose was called on is still on the list,
it will be fclose()ed again, which is undefined behaviour and
can result in a process crash.

So unlink from the list *before* calling do_ck_fclose().

If the error happens at fflush time instead of fclose time,
the fp will not end up being closed. This is not a serious
problem, but it could be fixed by calling ck_fflush(cur->fp)
before unlinking, and do_ck_fclose(cur->fp) afterwards.


diff --git a/sed/utils.c b/sed/utils.c
index 0a5351105e32..0e451bc24c92 100644
--- a/sed/utils.c
+++ b/sed/utils.c
@@ -297,8 +297,8 @@ ck_fclose(stream)
{
if (!stream || stream == cur->fp)
{
- do_ck_fclose (cur->fp);
prev->link = cur->link;
+ do_ck_fclose (cur->fp);
free(cur->name);
free(cur);
}

[Stanislav Brabec]

Post by NeilBrown
If do_ck_fclose gets an error from fclose() it will call
panic() which will try to close everything on the list.

There is a question: Why this code is needed at all? Is there a
platform, where exit(4) later in panic() keeps open file descriptors?

exit(3p) says:
The exit() function shall then flush all open streams with
unwritten buffered data, close all open streams, and remove all
files created by tmpfile().

It was added here:

2004-03-13 Paolo Bonzini <***@gnu.org>

Exit as soon as possible on an I/O error, and with
a better error message.
...
(panic): Unlink temporary files before exiting.


And even: Why the fflush() is called before fclose()? fclose() should
always flush. Isn't the error of fclose() sufficient? (Especially if
both end in panic().)

fclose(3p) says:
DESCRIPTION
The fclose() function shall cause the stream pointed to by
stream to be flushed and the associated file to be closed. Any
unwritten buffered data for the stream shall be written to the
file; any unread buffered data shall be discarded.

It was added here:

2002-06-08 Paolo Bonzini <***@gnu.org>
* lib/utils.c (ck_fclose): Work on stdout as well if stream == NULL
and flush before closing to check for errors


Well, and the check for EBADF in ck_fflush() looks like a hack that
could hide a problem elsewhere.

It was added here:

2002-10-28 Paolo Bonzini <***@gnu.org>

* lib/utils.c: Don't fail for EBADF in fflush
--
Best Regards / S pozdravem,

Stanislav Brabec
software developer
---------------------------------------------------------------------
SUSE LINUX, s. r. o. e-mail: ***@suse.cz
Lihovarská 1060/12 tel: [phone]
190 00 Praha 9 fax: [phone]
Czech Republic http://www.suse.cz/
PGP: 830B 40D5 9E05 35D8 5E27 6FA3 717C 209F A04F CD76

[Stanislav Brabec]

Post by NeilBrown
If do_ck_fclose gets an error from fclose() it will call
panic() which will try to close everything on the list.

There is a question: Why this code is needed at all? Is there a
platform, where exit(4) later in panic() keeps open file descriptors?

exit(3p) says:
The exit() function shall then flush all open streams with
unwritten buffered data, close all open streams, and remove all
files created by tmpfile().

It was added here:

2004-03-13 Paolo Bonzini <***@gnu.org>

Exit as soon as possible on an I/O error, and with
a better error message.
...
(panic): Unlink temporary files before exiting.


And even: Why the fflush() is called before fclose()? fclose() should
always flush. Isn't the error of fclose() sufficient? (Especially if
both end in panic().)

fclose(3p) says:
DESCRIPTION
The fclose() function shall cause the stream pointed to by
stream to be flushed and the associated file to be closed. Any
unwritten buffered data for the stream shall be written to the
file; any unread buffered data shall be discarded.

It was added here:

2002-06-08 Paolo Bonzini <***@gnu.org>
* lib/utils.c (ck_fclose): Work on stdout as well if stream == NULL
and flush before closing to check for errors


Well, and the check for EBADF in ck_fflush() looks like a hack that
could hide a problem elsewhere.

It was added here:

2002-10-28 Paolo Bonzini <***@gnu.org>

* lib/utils.c: Don't fail for EBADF in fflush
--
Best Regards / S pozdravem,

Stanislav Brabec
software developer
---------------------------------------------------------------------
SUSE LINUX, s. r. o. e-mail: ***@suse.cz
Lihovarská 1060/12 tel: [phone]
190 00 Praha 9 fax: [phone]
Czech Republic http://www.suse.cz/
PGP: 830B 40D5 9E05 35D8 5E27 6FA3 717C 209F A04F CD76

[Eric Blake]

Post by Stanislav Brabec

Post by NeilBrown
If do_ck_fclose gets an error from fclose() it will call
panic() which will try to close everything on the list.

There is a question: Why this code is needed at all? Is there a
platform, where exit(4) later in panic() keeps open file descriptors?

exit() is guaranteed to close fds, but NOT guaranteed to affect return
status if there was an error detected during the close. We'd rather
close things manually, so that we can force a non-zero exit status via
_exit() if the close failed (which, for some filesystems such as NFS, is
all too real of a possibility).

Post by Stanislav Brabec
And even: Why the fflush() is called before fclose()? fclose() should
always flush. Isn't the error of fclose() sufficient? (Especially if
both end in panic().)

Again, fclose() is guaranteed to attempt a flush, but not guaranteed to
do sane reporting of errors in that process. Doing a manual fflush()
first gives us better error reporting.

[Stanislav Brabec]

Post by Eric Blake

Post by Stanislav Brabec

Post by NeilBrown
If do_ck_fclose gets an error from fclose() it will call
panic() which will try to close everything on the list.

There is a question: Why this code is needed at all? Is there a
platform, where exit(4) later in panic() keeps open file descriptors?

exit() is guaranteed to close fds, but NOT guaranteed to affect return
status if there was an error detected during the close.

Well, panic() always calls exit(4) at the end, so it always returns
non-zero return code.

And, by the way, panic() does not respond to failed fclose() inside
itself.

The patch proposed by NeilBrown will fix the crash when panic is trying
to fclose() on a file where fclose() was already called and failed.

But yes, as temporary files are not created by tmpfile(), we need to
unlink them before exit, so we need the panic() or atexit(). And failed
unlink() is reported.

Post by Eric Blake
We'd rather
close things manually, so that we can force a non-zero exit status via
_exit() if the close failed (which, for some filesystems such as NFS, is
all too real of a possibility).

The panic() itself is a trap for failed fclose() in the whole rest of
sed.

Post by Eric Blake

Post by Stanislav Brabec
And even: Why the fflush() is called before fclose()? fclose() should
always flush. Isn't the error of fclose() sufficient? (Especially if
both end in panic().)

Again, fclose() is guaranteed to attempt a flush, but not guaranteed to
do sane reporting of errors in that process. Doing a manual fflush()
first gives us better error reporting.

Is there any such platform? I would consider zero returned by fclose()
after data loss while flushing as a serious bug in the underlying OS.

(All errors mentioned in fflush(3p) are also mentioned by fclose(3p).)
--
Best Regards / S pozdravem,

Stanislav Brabec
software developer
---------------------------------------------------------------------
SUSE LINUX, s. r. o. e-mail: ***@suse.cz
Lihovarská 1060/12 tel: [phone]
190 00 Praha 9 fax: [phone]
Czech Republic http://www.suse.cz/
PGP: 830B 40D5 9E05 35D8 5E27 6FA3 717C 209F A04F CD76