This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Set __file_ to 0 in basic_filebuf::close() even if fclose fails
ClosedPublic

Authored by phosek on Jul 19 2019, 12:46 AM.

Details

Summary

This issue was detected by ASan in one of our tests. This test manually
invokes basic_filebuf::cloe(). fclose(h.release() returned a non-zero
exit status, so
file_ wasn't set to 0. Later when basic_filebuf
destructor ran, we would enter the if (__file_) block again leading to
heap-use-after-free error.

The POSIX specification for fclose says that independently of the return
value, fclose closes the underlying file descriptor and any further
access (including another call to fclose()) to the stream results in
undefined behavior. This is exactly what happened in our test case.

To avoid this issue, we have to always set __file_ to 0 independently of
the fclose return value.

Diff Detail

Repository
rL LLVM

Event Timeline

phosek created this revision.Jul 19 2019, 12:46 AM
ldionne requested changes to this revision.Jul 19 2019, 5:02 AM

Is it possible to add a test case?

This revision now requires changes to proceed.Jul 19 2019, 5:02 AM

Is it possible to add a test case?

Yes, but I need to figure out a way to make fclose fail reliably inside the test.

phosek updated this revision to Diff 210927.Jul 19 2019, 4:08 PM

Test added, would it be possible to take a look?

ldionne accepted this revision.Jul 22 2019, 9:38 AM

LGTM if you leave a comment explaining the test.

libcxx/test/std/input.output/file.streams/fstreams/filebuf.members/close.pass.cpp
11 ↗(On Diff #210927)

typo: close();

This revision is now accepted and ready to land.Jul 22 2019, 9:38 AM
phosek updated this revision to Diff 211170.Jul 22 2019, 12:50 PM
phosek marked an inline comment as done.
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 22 2019, 12:54 PM