Skip to content

Commit

Permalink
Don't crash in wxFFile::Eof() and Error() if file is closed
Browse files Browse the repository at this point in the history
Assert and return false instead, this is more developer-friendly.

Add unit tests to check that these functions really work as expected when
called on a closed file.

Closes wxWidgets#17828.
  • Loading branch information
protopopov1122 authored and vadz committed Apr 1, 2017
1 parent a05b1f3 commit 9b1afaa
Show file tree
Hide file tree
Showing 5 changed files with 79 additions and 11 deletions.
1 change: 1 addition & 0 deletions docs/changes.txt
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@ All:
wxEvtHandler and/or wxTrackable in C++11 code (Raul Tambre, mmarsan).
- Update bundled expat to 2.2.0 (Catalin Raceanu).
- Add wxCMD_LINE_HIDDEN wxCmdLineParser flag (Lauri Nurmi).
- Don't crash in wxFFile::Eof() or Error() on closed file (jprotopopov).

All (GUI):

Expand Down
6 changes: 3 additions & 3 deletions include/wx/ffile.h
Original file line number Diff line number Diff line change
Expand Up @@ -76,13 +76,13 @@ class WXDLLIMPEXP_BASE wxFFile
wxFileOffset Length() const;

// simple accessors: note that Eof() and Error() may only be called if
// IsOpened()!
// IsOpened(). Otherwise they assert and return false.
// is file opened?
bool IsOpened() const { return m_fp != NULL; }
// is end of file reached?
bool Eof() const { return feof(m_fp) != 0; }
bool Eof() const;
// has an error occurred?
bool Error() const { return ferror(m_fp) != 0; }
bool Error() const;
// get the file name
const wxString& GetName() const { return m_name; }
// type such as disk or pipe
Expand Down
10 changes: 2 additions & 8 deletions interface/wx/ffile.h
Original file line number Diff line number Diff line change
Expand Up @@ -89,10 +89,7 @@ class wxFFile
Note that the behaviour of the file descriptor based class wxFile is different as
wxFile::Eof() will return @true here as soon as the last byte of the file has been read.
Also note that this method may only be called for opened files and may crash if
the file is not opened.
@todo THIS METHOD MAY CRASH? DOESN'T SOUND GOOD
Also note that this method may only be called for opened files. Otherwise it asserts and returns false.
@see IsOpened()
*/
Expand All @@ -102,10 +99,7 @@ class wxFFile
Returns @true if an error has occurred on this file, similar to the standard
@c ferror() function.
Please note that this method may only be called for opened files and may crash
if the file is not opened.
@todo THIS METHOD MAY CRASH? DOESN'T SOUND GOOD
Please note that this method may only be called for opened files. Otherwise it asserts and returns false.
@see IsOpened()
*/
Expand Down
14 changes: 14 additions & 0 deletions src/common/ffile.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -281,4 +281,18 @@ wxFileOffset wxFFile::Length() const
return wxInvalidOffset;
}

bool wxFFile::Eof() const
{
wxCHECK_MSG( IsOpened(), false,
wxT("wxFFile::Eof(): file is closed!") );
return feof(m_fp) != 0;
}

bool wxFFile::Error() const
{
wxCHECK_MSG( IsOpened(), false,
wxT("wxFFile::Error(): file is closed!") );
return ferror(m_fp) != 0;
}

#endif // wxUSE_FFILE
59 changes: 59 additions & 0 deletions tests/file/filefn.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,8 @@ class FileFunctionsTestCase : public CppUnit::TestCase
CPPUNIT_TEST( RenameFile );
CPPUNIT_TEST( ConcatenateFiles );
CPPUNIT_TEST( GetCwd );
CPPUNIT_TEST( FileEof );
CPPUNIT_TEST( FileError );
CPPUNIT_TEST_SUITE_END();

void GetTempFolder();
Expand All @@ -60,6 +62,8 @@ class FileFunctionsTestCase : public CppUnit::TestCase
void RenameFile();
void ConcatenateFiles();
void GetCwd();
void FileEof();
void FileError();

// Helper methods
void DoCreateFile(const wxString& filePath);
Expand Down Expand Up @@ -423,6 +427,61 @@ void FileFunctionsTestCase::GetCwd()
CPPUNIT_ASSERT( !cwd.IsEmpty() );
}

void FileFunctionsTestCase::FileEof()
{
const wxString filename(wxT("horse.bmp"));
const wxString msg = wxString::Format(wxT("File: %s"), filename.c_str());
const char *pUnitMsg = (const char*) msg.mb_str(wxConvUTF8);
wxFFile file(filename, wxT("r"));
// wxFFile::Eof must be false at start
CPPUNIT_ASSERT_MESSAGE( pUnitMsg, !file.Eof() );
CPPUNIT_ASSERT_MESSAGE( pUnitMsg, file.SeekEnd() );
// wxFFile::Eof returns true only after attempt to read last byte
char array[1];
CPPUNIT_ASSERT_MESSAGE( pUnitMsg, file.Read(array, 1) == 0 );
CPPUNIT_ASSERT_MESSAGE( pUnitMsg, file.Eof() );

CPPUNIT_ASSERT_MESSAGE( pUnitMsg, file.Close() );
// wxFFile::Eof after close should not cause crash but fail instead
bool failed = true;
try
{
file.Eof();
failed = false;
}
catch (...)
{
}
CPPUNIT_ASSERT_MESSAGE( pUnitMsg, failed );
}

void FileFunctionsTestCase::FileError()
{
const wxString filename(wxT("horse.bmp"));
const wxString msg = wxString::Format(wxT("File: %s"), filename.c_str());
const char *pUnitMsg = (const char*) msg.mb_str(wxConvUTF8);
wxFFile file(filename, wxT("r"));
// wxFFile::Error must be false at start assuming file "horse.bmp" exists.
CPPUNIT_ASSERT_MESSAGE( pUnitMsg, !file.Error() );
// Attempt to write to file opened in readonly mode should cause error
CPPUNIT_ASSERT_MESSAGE( pUnitMsg, !file.Write(filename) );
CPPUNIT_ASSERT_MESSAGE( pUnitMsg, file.Error() );

CPPUNIT_ASSERT_MESSAGE( pUnitMsg, file.Close() );
// wxFFile::Error after close should not cause crash but fail instead
bool failed = true;
try
{
file.Error();
failed = false;
}
catch (...)
{
}
CPPUNIT_ASSERT_MESSAGE( pUnitMsg, failed );
}


/*
TODO: other file functions to test:
Expand Down

0 comments on commit 9b1afaa

Please sign in to comment.