From a1cf60700c1e9bdb06c4cc7a4770231ba63e3c41 Mon Sep 17 00:00:00 2001 From: "sky@google.com" Date: Mon, 9 Mar 2009 18:27:34 +0000 Subject: Fixes possible crash in SessionBackend. I believe what's happening here is we fail to create the file, and so current_session_file_.get() is NULL and we crash. current_session_file_.get() is NULL if OpenAndWriteHeader returns NULL (which is does if the full header isn't written correctly. Here's how I'm changing the code: . The file is now truncated instead of closed/reopened. Hopefully this avoids the possibility of a scanner locking the file and the delete failing. . Added a unit test for coverage of truncation. . The file is opened in exclusive access. There is no reason why a scanner should open this file. . Added null checks. BUG=8476 TEST=none Review URL: http://codereview.chromium.org/39275 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@11262 0039d316-1c4b-4281-b951-d872f2087c98 --- chrome/browser/sessions/session_backend.cc | 25 +++++++++++++++---- chrome/browser/sessions/session_backend.h | 15 +++++++++-- .../browser/sessions/session_backend_unittest.cc | 29 ++++++++++++++++++++++ 3 files changed, 62 insertions(+), 7 deletions(-) (limited to 'chrome/browser/sessions') diff --git a/chrome/browser/sessions/session_backend.cc b/chrome/browser/sessions/session_backend.cc index ebadc31..13fe522 100644 --- a/chrome/browser/sessions/session_backend.cc +++ b/chrome/browser/sessions/session_backend.cc @@ -213,9 +213,14 @@ void SessionBackend::AppendCommands( std::vector* commands, bool reset_first) { Init(); - if ((reset_first && !empty_file_) || !current_session_file_->IsOpen()) + // Make sure and check current_session_file_, if opening the file failed + // current_session_file_ will be NULL. + if ((reset_first && !empty_file_) || !current_session_file_.get() || + !current_session_file_->IsOpen()) { ResetFile(); - if (current_session_file_->IsOpen() && + } + // Need to check current_session_file_ again, ResetFile may fail. + if (current_session_file_.get() && current_session_file_->IsOpen() && !AppendCommandsToFile(current_session_file_.get(), *commands)) { current_session_file_.reset(NULL); } @@ -313,7 +318,16 @@ bool SessionBackend::AppendCommandsToFile(net::FileStream* file, void SessionBackend::ResetFile() { DCHECK(inited_); - current_session_file_.reset(OpenAndWriteHeader(GetCurrentSessionPath())); + if (current_session_file_.get()) { + // File is already open, truncate it. We truncate instead of closing and + // reopening to avoid the possibility of scanners locking the file out + // from under us once we close it. If truncation fails, we'll try to + // recreate. + if (current_session_file_->Truncate(sizeof_header()) != sizeof_header()) + current_session_file_.reset(NULL); + } + if (!current_session_file_.get()) + current_session_file_.reset(OpenAndWriteHeader(GetCurrentSessionPath())); empty_file_ = true; } @@ -321,7 +335,8 @@ net::FileStream* SessionBackend::OpenAndWriteHeader(const FilePath& path) { DCHECK(!path.empty()); net::FileStream* file = new net::FileStream(); file->Open(path, base::PLATFORM_FILE_CREATE_ALWAYS | - base::PLATFORM_FILE_WRITE); + base::PLATFORM_FILE_WRITE | base::PLATFORM_FILE_EXCLUSIVE_WRITE | + base::PLATFORM_FILE_EXCLUSIVE_READ); if (!file->IsOpen()) return NULL; int32 header[2]; @@ -329,7 +344,7 @@ net::FileStream* SessionBackend::OpenAndWriteHeader(const FilePath& path) { header[1] = kFileCurrentVersion; int wrote = file->Write(reinterpret_cast(&header), sizeof(header), NULL); - if (wrote != sizeof(header)) + if (wrote != sizeof_header()) return NULL; return file; } diff --git a/chrome/browser/sessions/session_backend.h b/chrome/browser/sessions/session_backend.h index 496a3ec..bfd446f 100644 --- a/chrome/browser/sessions/session_backend.h +++ b/chrome/browser/sessions/session_backend.h @@ -81,8 +81,12 @@ class SessionBackend : public base::RefCountedThreadSafe { void MoveCurrentSessionToLastSession(); private: - // Recreates the current file such that it only contains the header and - // NO commands. + // If current_session_file_ is open, it is truncated so that it is essentially + // empty (only contains the header). If current_session_file_ isn't open, it + // is is opened and the header is written to it. After this + // current_session_file_ contains no commands. + // NOTE: current_session_file_ may be NULL if the file couldn't be opened or + // the header couldn't be written. void ResetFile(); // Opens the current file and writes the header. On success a handle to @@ -93,6 +97,13 @@ class SessionBackend : public base::RefCountedThreadSafe { bool AppendCommandsToFile(net::FileStream* file, const std::vector& commands); + // Returns the size of the header. The header is the first bytes written to + // the file, and is used to identify the file as one written by us. + int32 sizeof_header() const { + int32 header[2]; + return sizeof(header); + } + const BaseSessionService::SessionType type_; // Returns the path to the last file. diff --git a/chrome/browser/sessions/session_backend_unittest.cc b/chrome/browser/sessions/session_backend_unittest.cc index fb424f2..e5ba17d4 100644 --- a/chrome/browser/sessions/session_backend_unittest.cc +++ b/chrome/browser/sessions/session_backend_unittest.cc @@ -180,3 +180,32 @@ TEST_F(SessionBackendTest, EmptyCommand) { AssertCommandEqualsData(empty_command, commands[0]); STLDeleteElements(&commands); } + +// Writes a command, appends another command with reset to true, then reads +// making sure we only get back the second command. +TEST_F(SessionBackendTest, Truncate) { + scoped_refptr backend( + new SessionBackend(BaseSessionService::SESSION_RESTORE, path_)); + struct TestData first_data = { 1, "a" }; + std::vector commands; + commands.push_back(CreateCommandFromData(first_data)); + backend->AppendCommands(new SessionCommands(commands), false); + commands.clear(); + + // Write another command, this time resetting the file when appending. + struct TestData second_data = { 2, "b" }; + commands.push_back(CreateCommandFromData(second_data)); + backend->AppendCommands(new SessionCommands(commands), true); + commands.clear(); + + // Read it back in. + backend = NULL; + backend = new SessionBackend(BaseSessionService::SESSION_RESTORE, path_); + backend->ReadLastSessionCommandsImpl(&commands); + + // And make sure we get back the expected data. + ASSERT_EQ(1U, commands.size()); + AssertCommandEqualsData(second_data, commands[0]); + + STLDeleteElements(&commands); +} -- cgit v1.1