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

Previous Next

Full log

View this message in rfc822 format

From: Stanislav Brabec <sbrabec <at> suse.com>
To: 21913 <at> debbugs.gnu.org
Subject: bug#21913: 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

Previous Next

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