From ea4f9a4ba2e6d49d67097368fc8b083d3402c629 Mon Sep 17 00:00:00 2001 From: "Ignacio R. Morelle" Date: Sun, 9 Oct 2016 23:19:45 -0300 Subject: [PATCH] campaignd: Add wrapper for atomic commits of crucial files As the 2016-10-07~09 downtime incident shows, it is paramount to take further steps in guaranteeing that the server can't corrupt its own data files (especially the add-ons database) when receiving inappropriately-timed signals. This commit adds and deploys an ostream wrapper that requires callers to explicitly commit the results to disk when finished writing to the stream, so that only then the real destination file is overwritten with the working copy (a temporary in the same dir). This way, there should never be a situation in which the destination is left in an inconsistent state due to a signal or exception. The temporary receives a predictable name right now in the interest of simplicity, since we are more or less in control of the target directory anyway. We definitely don't want it to be an unlinked file since that would make it impossible for admins to inspect and compare the temporary against the original afterwards. The code makes some assumptions about the nature of the return value of filesystem::ostream_file() which will never be broken in this stable branch, which is why one helper function is in campaignd land rather than in the global filesystem API for now. This should probably be rectified when forward-porting to master. Maybe. Nothing of this will work reliably on Windows but we don't care. There's only one machine in the world where we support running campaignd at this time and it runs Linux. --- src/CMakeLists.txt | 1 + src/SConscript | 1 + src/campaign_server/campaign_server.cpp | 11 ++- src/campaign_server/fs_commit.cpp | 123 ++++++++++++++++++++++++ src/campaign_server/fs_commit.hpp | 97 +++++++++++++++++++ 5 files changed, 229 insertions(+), 4 deletions(-) create mode 100644 src/campaign_server/fs_commit.cpp create mode 100644 src/campaign_server/fs_commit.hpp diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index 935280557de4..422ec1faea0c 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -1101,6 +1101,7 @@ set(campaignd_SRC campaign_server/addon_utils.cpp campaign_server/blacklist.cpp campaign_server/campaign_server.cpp + campaign_server/fs_commit.cpp server/input_stream.cpp ${network_implementation_files} loadscreen_empty.cpp diff --git a/src/SConscript b/src/SConscript index d235c5367649..e020e754b400 100644 --- a/src/SConscript +++ b/src/SConscript @@ -580,6 +580,7 @@ client_env.WesnothProgram("wesnoth", wesnoth_objects, have_client_prereqs) campaignd_sources = Split(""" campaign_server/addon_utils.cpp campaign_server/blacklist.cpp + campaign_server/fs_commit.cpp server/input_stream.cpp """) diff --git a/src/campaign_server/campaign_server.cpp b/src/campaign_server/campaign_server.cpp index 5402df4f05f0..149ed65d3f0b 100644 --- a/src/campaign_server/campaign_server.cpp +++ b/src/campaign_server/campaign_server.cpp @@ -32,6 +32,7 @@ #include "campaign_server/addon_utils.hpp" #include "campaign_server/blacklist.hpp" #include "campaign_server/control.hpp" +#include "campaign_server/fs_commit.hpp" #include "version.hpp" #include "util.hpp" #include "hash.hpp" @@ -271,8 +272,9 @@ void server::load_blacklist() void server::write_config() { DBG_CS << "writing configuration and add-ons list to disk...\n"; - filesystem::scoped_ostream out = filesystem::ostream_file(cfg_file_); - write(*out, cfg_); + filesystem::atomic_commit out(cfg_file_); + write(*out.ostream(), cfg_); + out.commit(); DBG_CS << "... done\n"; } @@ -794,9 +796,10 @@ void server::handle_upload(const server::request& req) add_license(data); { - filesystem::scoped_ostream campaign_file = filesystem::ostream_file(filename); - config_writer writer(*campaign_file, true, compress_level_); + filesystem::atomic_commit campaign_file(cfg_file_); + config_writer writer(*campaign_file.ostream(), true, compress_level_); writer.write(data); + campaign_file.commit(); } (*campaign)["size"] = filesystem::file_size(filename); diff --git a/src/campaign_server/fs_commit.cpp b/src/campaign_server/fs_commit.cpp new file mode 100644 index 000000000000..bbdcb4511347 --- /dev/null +++ b/src/campaign_server/fs_commit.cpp @@ -0,0 +1,123 @@ +/* + Copyright (C) 2016 by Ignacio R. Morelle + Part of the Battle for Wesnoth Project http://www.wesnoth.org/ + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 2 of the License, or + (at your option) any later version. + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY. + + See the COPYING file for more details. +*/ + +#include "campaign_server/fs_commit.hpp" + +#include "log.hpp" +#include "serialization/parser.hpp" + +#include +#include +#include + +#include + +#ifndef _WIN32 +#include +#include +#endif + +static lg::log_domain log_filesystem("filesystem"); + +#define DBG_FS LOG_STREAM(debug, log_filesystem) +#define LOG_FS LOG_STREAM(info, log_filesystem) +#define WRN_FS LOG_STREAM(warn, log_filesystem) +#define ERR_FS LOG_STREAM(err, log_filesystem) + +namespace filesystem +{ + +namespace +{ + +inline void atomic_fail(const std::string& step_description) +{ + const std::string errno_desc = std::strerror(errno); + ERR_FS << "Atomic commit failed (" << step_description << "): " + << errno_desc << '\n'; + throw filesystem::io_exception(std::string("Atomic commit failed (") + step_description + ")"); +} + +#ifndef _WIN32 + +/** + * Returns the POSIX file descriptor associated with the stream. + * + * This only makes sense for valid streams returned by ostream_file(). Anything + * else will yield 0. + */ +int get_stream_file_descriptor(std::ostream& os) +{ + // NOTE: This is insider knowledge of filesystem::ostream_file(), but it will + // do for 1.12 at least. + typedef boost::iostreams::stream fd_stream_type; + fd_stream_type* const real = dynamic_cast(&os); + return real ? (*real)->handle() : 0; +} + +#endif // ! _WIN32 + +} // unnamed namespace + +atomic_commit::atomic_commit(const std::string& filename) + : temp_name_(filename + ".new") + , dest_name_(filename) + , out_(filesystem::ostream_file(temp_name_)) +#ifndef _WIN32 + , outfd_(filesystem::get_stream_file_descriptor(*out_)) +#endif +{ + LOG_FS << "Atomic write guard created for " << dest_name_ << " using " << temp_name_ << '\n'; +} + +atomic_commit::~atomic_commit() +{ + if(!temp_name_.empty()) { + ERR_FS << "Temporary file for atomic write leaked: " << temp_name_ << '\n'; + } +} + +void atomic_commit::commit() +{ + if(temp_name_.empty()) { + ERR_FS << "Attempted to commit " << dest_name_ << " more than once!\n"; + return; + } + +#ifdef _WIN32 + // WARNING: + // Obviously not atomic at all. Perhaps there's an alternate way to achieve + // the same more securely using the Win32 API, but I don't think anyone + // cares about running campaignd on this platform, let alone making it + // resilient against environment errors. This is just here for reference. + if(filesystem::file_exists(dest_name_) && std::remove(dest_name_.c_str()) != 0) { + atomic_fail("remove"); + } +#else + if(fsync(outfd_) != 0) { + atomic_fail("fsync"); + } +#endif + + if(std::rename(temp_name_.c_str(), dest_name_.c_str()) != 0) { + atomic_fail("rename"); + } + + LOG_FS << "Atomic commit succeeded: " << temp_name_ << " -> " << dest_name_ << '\n'; + + temp_name_.clear(); +} + + +} // namespace filesystem \ No newline at end of file diff --git a/src/campaign_server/fs_commit.hpp b/src/campaign_server/fs_commit.hpp new file mode 100644 index 000000000000..f30ce6ac7714 --- /dev/null +++ b/src/campaign_server/fs_commit.hpp @@ -0,0 +1,97 @@ +/* + Copyright (C) 2016 by Ignacio R. Morelle + Part of the Battle for Wesnoth Project http://www.wesnoth.org/ + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 2 of the License, or + (at your option) any later version. + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY. + + See the COPYING file for more details. +*/ + +/** + * @file + * Atomic filesystem commit functions. + */ + +#ifndef CAMPAIGN_SERVER_FS_COMMIT_HPP_INCLUDED +#define CAMPAIGN_SERVER_FS_COMMIT_HPP_INCLUDED + +#include "filesystem.hpp" + +#include + +class config; + +namespace filesystem +{ + +/** + * Wrapper class that guarantees that file commit atomicity. + * + * It is possible for a signal or exception to cause a file write to be aborted + * in the middle of the process, leaving potentially inconsistent contents + * behind which may be read by the same or another application later and result + * in errors or further data loss. + * + * This wrapper prevents this by providing callers with an interface to request + * a write stream that will be actually associated to a temporary file. Once + * the caller is done with the file, it should call the commit() method to + * complete the process so that the temporary replaces the original in an + * atomic step. + * + * The rationale for using an explicit commit() method instead of the class + * destructor is that the destructor will also be invoked during exception + * handling, which could still cause the destination file to end up in an + * inconsistent state. + * + * Note that if the destructor is invoked before commit() is, the temporary + * file will be left behind. This is deliberate so as to provide a way for + * the user to look at the resulting file and optionally try to reconcile it + * against the original. + */ +class atomic_commit : private boost::noncopyable +{ +public: + /** + * Constructor. + * + * @throws filesystem::io_exception if the operation fails in some way. + */ + atomic_commit(const std::string& filename); + + ~atomic_commit(); + + /** + * Returns the write stream associated with the file. + * + * Before commit() is invoked, this refers to the temporary file; afterwards, + * to the destination. + */ + scoped_ostream& ostream() + { + return out_; + } + + /** + * Commits the new file contents to disk atomically. + * + * @throws filesystem::io_exception if the operation fails in some way. + */ + void commit(); + +private: + std::string temp_name_; + std::string dest_name_; + scoped_ostream out_; +#ifndef _WIN32 + int outfd_; +#endif +}; + +} + +#endif