[Bug libstdc++/17215] New: __basic_file<char>::close ignores errors

[Bug libstdc++/17215] New: __basic_file<char>::close ignores errors

[ken at xorian dot net]

__basic_file<char>::close ignores the return value of fclose(3).  This
call can fail and set errno to a variety of values (ENOSPC, EINTR,
etc.).  Aside from not notifying the client code (e.g. by setting
failbit in an enclosing fstream), this can also cause a memory leak as
the FILE data structure won't be de-allocated by a failed fclose and
__basic_file<char>::close discards it regardless.

Similarly, in the case where _M_cfile_created is false,
__basic_file<char>::close ignores the return value of this->sync(),
which is just a call to fflush(3).  This is not quite as bad (as it
can't cause a memory leak), but is still arguably wrong.

-- 
           Summary: __basic_file<char>::close ignores errors
           Product: gcc
           Version: 3.4.1
            Status: UNCONFIRMED
          Severity: normal
          Priority: P2
         Component: libstdc++
        AssignedTo: unassigned at gcc dot gnu dot org
        ReportedBy: ken at xorian dot net
                CC: gcc-bugs at gcc dot gnu dot org


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=17215

[Bug libstdc++/17215] __basic_file<char>::close ignores errors

[ken at xorian dot net]

------- Additional Comments From ken at xorian dot net  2004-08-27 18:07 -------
Created an attachment (id=6996)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=6996&action=view)
Propsed patch


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=17215

[Bug libstdc++/17215] __basic_file<char>::close ignores errors

[pcarlini at suse dot de]

------- Additional Comments From pcarlini at suse dot de  2004-08-27 19:25 -------
Hi,

I think that basically both the problem is real and your solution correct,
thanks. However, a few details:

> __basic_file<char>::close ignores the return value of fclose(3).  This
> call can fail and set errno to a variety of values (ENOSPC, EINTR,
> etc.).

Notice that the C standard, the reference for the samantics of fclose in C++,
tells nothing about errno, see C99, 7.19.5.1.

>              Aside from not notifying the client code (e.g. by setting
> failbit in an enclosing fstream),

Agreed.

>                                  this can also cause a memory leak as
> the FILE data structure won't be de-allocated by a failed fclose and
> __basic_file<char>::close discards it regardless.

I doubt it. Again, see 7.19.5.1: the semantics of fclose in the C standard
is not the same of close (in Posix, or what else): 7.19.5.1, p2, clearly
points out that even when fclose fails there are no risks of leaks, since
the buffers are deallocated anyway. Also, the stream is disassociated from
the file and you cannot perform additional operations on it. All of this
implies, in my opinion, that your fix should be tweaked to do _M_cfile = 0
unconditionally.

By the way, is your patch already regression tested? Can you think of any
testcases?

Thanks,
Paolo.

-- 
           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |WAITING


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=17215

[Bug libstdc++/17215] __basic_file<char>::close ignores errors

[ken at xorian dot net]

------- Additional Comments From ken at xorian dot net  2004-08-27 22:49 -------
> I doubt it. Again, see 7.19.5.1: the semantics of fclose in the C
> standard is not the same of close (in Posix, or what else):
> 7.19.5.1, p2, clearly points out that even when fclose fails there
> are no risks of leaks, since the buffers are deallocated
> anyway. Also, the stream is disassociated from the file and you
> cannot perform additional operations on it. All of this implies, in
> my opinion, that your fix should be tweaked to do _M_cfile = 0
> unconditionally.

I admit I had not checked C99 on this, and I find what you point out
to be rather distressing actually.  Several other references (the
Single UNIX Specification, man pages on Linux and Tru64) say that
fclose can fail with errno=EINTR.  I had assumed that in the event of
that kind of temporary failure (where the normal thing to do is to
repeat the operation) that one would be able to call fclose on the
same stream to complete the write(2) or close(2) that had been
interrupted.

If a C run-time library were to implement fclose as C99 seems to
describe, then such a transient error would be totally unrecoverable.
That would make stdio and by extension std::fstream unusable for any
application that required data integrity.

It may be worth mentioning that I noticed the problem this bug report
is about because I had fstream::open fail with errno=EINTR.  I've been
trying to identify all the changes I need to make to at the
application level to properly protect against this causing spurious
failures.  Since fstream::open can fail in this way, it seemed
sensible to add the same protection and repeating of the operation on
EINTR around fstream::close.  (I did check the C++ spec, which states
that basic_filebuf::close will fail is fclose fails.)

> By the way, is your patch already regression tested? Can you think
> of any testcases?

I did not run any of the regression tests included with gcc on it,
though I've done some very minimal testing using it with some code I'm
working with.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=17215

[Bug libstdc++/17215] __basic_file<char>::close ignores errors

[pcarlini at suse dot de]

-- 
           What    |Removed                     |Added
----------------------------------------------------------------------------
         AssignedTo|unassigned at gcc dot gnu   |pcarlini at suse dot de
                   |dot org                     |
             Status|WAITING                     |ASSIGNED


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=17215

[Bug libstdc++/17215] __basic_file<char>::close ignores errors

[cvs-commit at gcc dot gnu dot org]

------- Additional Comments From cvs-commit at gcc dot gnu dot org  2004-08-30 11:34 -------
Subject: Bug 17215

CVSROOT:	/cvs/gcc
Module name:	gcc
Changes by:	paolo@gcc.gnu.org	2004-08-30 11:33:54

Modified files:
	libstdc++-v3   : ChangeLog 
	libstdc++-v3/config/io: basic_file_stdio.cc 

Log message:
	2004-08-30  Paolo Carlini  <pcarlini@suse.de>
	Kenneth C. Schalk  <ken@xorian.net>
	
	PR libstdc++/17215
	* config/io/basic_file_stdio.cc (__basic_file<char>::close()):
	Check the return value of fclose/sync, loop on EINTR.
	(__basic_file<char>::sys_open): Likewise, for sync.

Patches:
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/libstdc++-v3/ChangeLog.diff?cvsroot=gcc&r1=1.2646&r2=1.2647
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/libstdc++-v3/config/io/basic_file_stdio.cc.diff?cvsroot=gcc&r1=1.32&r2=1.33



-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=17215

[Bug libstdc++/17215] __basic_file<char>::close ignores errors

[pcarlini at suse dot de]

-- 
           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |3.4.3


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=17215

[Bug libstdc++/17215] [3.4 only] __basic_file<char>::close ignores errors

[bkoz at gcc dot gnu dot org]

------- Additional Comments From bkoz at gcc dot gnu dot org  2004-09-02 17:41 -------

There really should be a test case for this in the testsuite. Kenneth, can you
post the code you used to notice this?

thanks,
benjamin

-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=17215

[Bug libstdc++/17215] [3.4 only] __basic_file<char>::close ignores errors

[ken at xorian dot net]

------- Additional Comments From ken at xorian dot net  2004-09-02 18:09 -------
Unfortunately, I haven't had much luck with generating a "real" EINTR.
I've tried various methods (such as forking a separate process that
sits in a loop killing the main process with a signal whose handler
does nothing), but I haven't come up with a method that works.

The way I noticed it originally was with a method for "faking" EINTR.
I'll attach the single C file (fake_eintr.c) which can be compiled
into a shared library like so:

gcc -shared -o fake_eintr.so fake_eintr.c -ldl -lc

And used as an LD_PRELOAD (on Linux at least).  This is, however,
unsatisfying as a method of testing this as it doesn't reflect state
changes that would happen inside the C run-time library when a real
EINTR occurs.  (For example, ferror(3) would return false after a
faked EINTR from my flose(3) wrapper.)


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=17215

[Bug libstdc++/17215] [3.4 only] __basic_file<char>::close ignores errors

[mmitchel at gcc dot gnu dot org]

------- Additional Comments From mmitchel at gcc dot gnu dot org  2004-11-01 00:45 -------
Postponed until GCC 3.4.4.

-- 
           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|3.4.3                       |3.4.4


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=17215

[Bug libstdc++/17215] [3.4 only] __basic_file<char>::close ignores errors

[cvs-commit at gcc dot gnu dot org]

------- Additional Comments From cvs-commit at gcc dot gnu dot org  2004-11-08 00:41 -------
Subject: Bug 17215

CVSROOT:	/cvs/gcc
Module name:	gcc
Branch: 	gcc-3_4-branch
Changes by:	paolo@gcc.gnu.org	2004-11-08 00:41:15

Modified files:
	libstdc++-v3   : ChangeLog 
	libstdc++-v3/config/io: basic_file_stdio.cc 
	libstdc++-v3/include/bits: fstream.tcc 

Log message:
	2004-11-07  Paolo Carlini  <pcarlini@suse.de>
	Andrea Arcangeli  <andrea@suse.de>
	
	* config/io/basic_file_stdio.cc (__basic_file<>::close)): Don't
	call unnecessarily sync, that is fflush: the library, since 3.4.0
	does not use buffered fread/fwrite.
	* include/bits/fstream.tcc (basic_filebuf<>::overflow): Likewise.
	
	2004-11-07  Paolo Carlini  <pcarlini@suse.de>
	Kenneth C. Schalk  <ken@xorian.net>
	
	PR libstdc++/17215
	* config/io/basic_file_stdio.cc (__basic_file<char>::close()):
	Check the return value of fclose/sync, loop on EINTR.
	(__basic_file<char>::sys_open): Likewise, for sync.

Patches:
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/libstdc++-v3/ChangeLog.diff?cvsroot=gcc&only_with_tag=gcc-3_4-branch&r1=1.2224.2.194&r2=1.2224.2.195
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/libstdc++-v3/config/io/basic_file_stdio.cc.diff?cvsroot=gcc&only_with_tag=gcc-3_4-branch&r1=1.25.4.6&r2=1.25.4.7
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/libstdc++-v3/include/bits/fstream.tcc.diff?cvsroot=gcc&only_with_tag=gcc-3_4-branch&r1=1.116.4.7&r2=1.116.4.8



-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=17215

[Bug libstdc++/17215] [3.4 only] __basic_file<char>::close ignores errors

[pcarlini at suse dot de]

------- Additional Comments From pcarlini at suse dot de  2004-11-08 00:42 -------
Fixed for 3.4.4.

-- 
           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|                            |FIXED


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=17215