GNU bug report logs - #21913
sed/utils.c temporary file handling code review

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 21913 in the body.
You can then email your comments to 21913 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Stanislav Brabec <sbrabec <at> suse.com>
To: <bug-sed <at> gnu.org>
Subject: sed/utils.c temporary file handling code review
Date: Fri, 13 Nov 2015 20:06:17 +0100

While trying to reproduce an obscure crash with temporary file experiencing
file system error, I looked deeper into sed/utils.c. I found several strange
things.

The code handling temporary file seems to be fragile and contains dead chunks.

See end of this mail and please advise what way to fix you propose?


1. ck_fclose() is (theoretically) vulnerable into a double close, because
open_files still contains file being closed while calling do_ck_fclose(). In
some error conditions, do_ck_fclose() can call panic(), which manipulates with
open_files and tries to fclose() all files there, including that one just closed.

However, it seems to never happen, as there are other issues.

Partial fix for that was sent to the bug-sed list last year [1], but it is not
in the sed GIT.

2.  register_open_file() defines third argument temp (which is actually set to
true e. g. in ck_mkstemp()). But the argument is never used, and p->temp is
always set to false.

3. 2. makes panic() part inside "if (open_files->temp)" completely void and
never called.

4. However it is never called, it has no side effects, as the whole
open_file.temp looks completely obsolete. The functionality of it was moved to
atexit() cleanup(), and depends on register_cleanup_file(). And
register_cleanup_file() is really called in the only place, where
register_open_file() would use temp=true (the only use is ck_mkstemp(), and the
only use of ck_mkstemp() uses register_cleanup_file() as well).

Note that register_open_file() can handle (if it will work) arbitrary number of
temporary files, but register_cleanup_file() can handle only one. But it seems
to be no problem.

5. The dead code in panic() uses fclose(); unlink() and it would (if it will be
ever called) trigger crash caused by a code mentioned in 1. The new cleanup()
code in calls unlink() without fclose(), and dues not use open_files list, so
it could not trigger the crash.


What I think should be done with the code?

a1. Completely remove third argument of register_open_file().
or
a2. Fix register_open_file().

b1. Completely remove code that expected open_files->temp being true.
or
b2. Must apply c.

c. Study the code, and if there is still a chance to out-of-sync mentioned in
1., prevent it by reordering of lines to a safe order:
prev->link = cur->link;
open_files = r.link;
do_ck_fclose (cur->fp);

d. (optional) As register_open_file() and register_cleanup_file() do nearly
the same, it is possible to convert cleanup() to use open_files, and remove
register_cleanup_file() completely.

References:
[1]
Date: Tue, 3 Jun 2014 09:27:28 +1000
From: NeilBrown <neilb <at> suse.de>
To: bug-sed <at> gnu.org
Subject: [PATCH sed] ck_fclose should unlink *before* calling do_ck_fclose.

-- 
Best Regards / S pozdravem,

Stanislav Brabec
software developer
---------------------------------------------------------------------
SUSE LINUX, s. r. o.                         e-mail: sbrabec <at> suse.com
Lihovarská 1060/12                            tel: +49 911 7405384547
190 00 Praha 9                                 fax:  +420 284 084 001
Czech Republic                                    http://www.suse.cz/
PGP: 830B 40D5 9E05 35D8 5E27 6FA3 717C 209F A04F CD76

Message #8 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Stanislav Brabec <sbrabec <at> suse.com>
To: <bug-sed <at> gnu.org>
Subject: Re: bug#21913: sed/utils.c temporary file handling code review
Date: Wed, 18 Nov 2015 19:09:15 +0100

Stanislav Brabec wrote:
> While trying to reproduce an obscure crash with temporary file experiencing
> file system error, I looked deeper into sed/utils.c. I found several strange
> things.
> 
Here is a detailed regression map:

commit 9c9919efe2166efd32409054005619062624226c (initial import in 2004)
imported the broken code vulnerable to double fclose() issue and leaving
orphan temporary files in some situations.

commit 9c9919efe2166efd32409054005619062624226c in 2004 introduced the
register_open_file() temporary file bug. No side effects yet.

commit 3a8e165ab02487c372df217c1989e287625ce0ae in 2006 started to really use
broken register_open_file() in ck_mkstemp() with third argument "true". It
caused a regression: keeping orphan files after even more errors than before,
but the regression hides the double fclose() vulnerability.

commit 768901548e280726f160a1da4434f3fde8f9921a in 2015 introduced
register_cleanup_file() that re-implements broken temporary removal feature
of register_open_file(). This change hides the register_open_file() temporary
file bug.

Both mentioned bugs are now present in the code, but probably cannot be
triggered.

-- 
Best Regards / S pozdravem,

Stanislav Brabec
software developer
---------------------------------------------------------------------
SUSE LINUX, s. r. o.                         e-mail: sbrabec <at> suse.com
Lihovarská 1060/12                            tel: +49 911 7405384547
190 00 Praha 9                                 fax:  +420 284 084 001
Czech Republic                                    http://www.suse.cz/
PGP: 830B 40D5 9E05 35D8 5E27 6FA3 717C 209F A04F CD76

Message #11 received at 21913 <at> debbugs.gnu.org (full text, mbox):

From: Assaf Gordon <assafgordon <at> gmail.com>
To: Stanislav Brabec <sbrabec <at> suse.com>, 21913 <at> debbugs.gnu.org
Subject: Re: bug#21913: sed/utils.c temporary file handling code review
Date: Tue, 9 Oct 2018 08:26:13 -0600

tags 21913 moreinfo
close 21913
stop

Hello,

(catching up on old reports)

On 18/11/15 11:09 AM, Stanislav Brabec wrote:
> Stanislav Brabec wrote:
>> While trying to reproduce an obscure crash with temporary file
>> experiencing file system error, I looked deeper into sed/utils.c. I
>> found several strange things.[...]
> Both mentioned bugs are now present in the code, but probably cannot
> be triggered.
Given this conclusion, and with no new information (or new similar bug
reports) for almost 3 years, I'm including to close this bug.

Discussion can continue by replying to this thread, and if more 
information arises we can always re-open it.

regards,
 - assaf

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.