Write session files with O_NOFOLLOW and mode 0600

Also make directory_entry::is_file() report the real type so symlinks are skipped.

src/rpc/xmlrpc_tinyxml2.cc

@@ -326,15 +326,20 @@ process_document(const tinyxml2::XMLDocument* doc, tinyxml2::XMLPrinter* printer
     auto& result_list     = result.as_list();
     auto  parent_elements = element_access(doc->RootElement(), {"params", "param", "value", "array", "data"});
     for (auto child = parent_elements->FirstChildElement("value"); child; child = child->NextSiblingElement("value")) {
-      auto sub_method_name = element_access(child, {"struct", "member", "value", "string"})->GetText();
+      auto method_name_member = element_access(child, {"struct", "member"});
+      auto member_name        = method_name_member->FirstChildElement("name");
+
+      if (member_name == nullptr || member_name->GetText() == nullptr ||
+          std::strncmp(member_name->GetText(), "methodName", sizeof("methodName")) != 0)
+        throw rpc_error(XMLRPC_PARSE_ERROR, "multicall struct's first member must be methodName");
+
+      auto sub_method_name = element_access(method_name_member, {"value", "string"})->GetText();
 
       if (sub_method_name == nullptr)
         throw rpc_error(XMLRPC_PARSE_ERROR, "multicall methodName element is empty");
       // If sub_params ends up a nullptr at the end of this if-chian,
       // execute_command will turn it into an empty list
-      auto sub_params = element_access(child, {"struct", "member"});
-      if (sub_params != nullptr)
-        sub_params = sub_params->NextSiblingElement("member");
+      auto sub_params = method_name_member->NextSiblingElement("member");
       if (sub_params != nullptr)
         sub_params = sub_params->FirstChildElement("value");
       if (sub_params != nullptr)

src/session/download_storer.cc

@@ -2,6 +2,7 @@
 
 #include "download_storer.h"
 
+#include <cerrno>
 #include <fcntl.h>
 #include <fstream>
 #include <unistd.h>
@@ -116,28 +117,36 @@ is_correct_format(const std::string& f) {
 
 void
 save_stream(const std::string& path, bool use_fsyncdisk, const std::stringstream& stream) {
-  std::fstream output(path.c_str(), std::ios::out | std::ios::trunc);
+  // Remove any leftover temporary file first so that O_EXCL only ever fails on
+  // an entry that appeared after the unlink, and O_NOFOLLOW keeps a symlink
+  // planted in the session directory from redirecting the write.
+  if (::unlink(path.c_str()) == -1 && errno != ENOENT)
+    throw torrent::storage_error("failed to remove stale file : " + path);
 
   // TODO: If we cannot open more files, wait for some to finish and try again.
-  if (!output.is_open())
-    throw torrent::storage_error("failed to open file for writing : " + path);
-
-  output << stream.rdbuf();
-
-  if (!output.good())
-    throw torrent::storage_error("failed to write stream to file : " + path);
-
-  // The data only reaches the kernel here, so this is where a full disk is seen.
-  output.close();
-
-  if (!output.good())
-    throw torrent::storage_error("failed to flush stream to file : " + path);
-
-  // Ensure that the new file is actually written to the disk
-  int fd = ::open(path.c_str(), O_WRONLY);
+  int fd = ::open(path.c_str(), O_WRONLY | O_CREAT | O_EXCL | O_NOFOLLOW, 0600);
 
   if (fd < 0)
-    throw torrent::storage_error("failed to open file descriptor for fsync : " + path);
+    throw torrent::storage_error("failed to open file for writing : " + path);
+
+  const auto  data      = stream.view();
+  std::size_t remaining = data.size();
+  const char* cursor    = data.data();
+
+  while (remaining != 0) {
+    ssize_t result = ::write(fd, cursor, remaining);
+
+    if (result == -1) {
+      if (errno == EINTR)
+        continue;
+
+      ::close(fd);
+      throw torrent::storage_error("failed to write stream to file : " + path);
+    }
+
+    cursor    += result;
+    remaining -= result;
+  }
 
   if (use_fsyncdisk) {
 #ifdef __APPLE__
@@ -152,6 +161,7 @@ save_stream(const std::string& path, bool use_fsyncdisk, const std::stringstream
     }
   }
 
+  // A full disk may only be seen when the descriptor is closed.
   if (::close(fd) == -1)
     throw torrent::storage_error("failed to close file descriptor : " + path);
 }

src/utils/directory.cc

@@ -5,6 +5,7 @@
 #include <algorithm>
 #include <cstdlib>
 #include <dirent.h>
+#include <fcntl.h>
 #include <functional>
 #include <sys/stat.h>
 #include <torrent/exceptions.h>
@@ -13,6 +14,24 @@
 
 namespace utils {
 
+namespace {
+
+uint8_t
+entry_type_from_mode(mode_t mode) {
+  if (S_ISREG(mode))
+    return DT_REG;
+
+  if (S_ISDIR(mode))
+    return DT_DIR;
+
+  if (S_ISLNK(mode))
+    return DT_LNK;
+
+  return DT_UNKNOWN;
+}
+
+} // namespace
+
 // Keep this?
 bool
 Directory::is_valid() const {
@@ -38,9 +57,6 @@ Directory::update(int flags) {
     return false;
 
   struct dirent* entry;
-#ifdef __sun__
-  struct stat s;
-#endif
 
   while ((entry = readdir(d)) != NULL) {
     if ((flags & update_hide_dot) && entry->d_name[0] == '.')
@@ -49,16 +65,22 @@ Directory::update(int flags) {
     iterator itr = base_type::insert(end(), value_type());
 
 #ifdef __sun__
-    stat(entry->d_name, &s);
     itr->s_fileno = entry->d_ino;
     itr->s_reclen = 0;
-    itr->s_type = s.st_mode;
+    itr->s_type   = DT_UNKNOWN;
 #else
     itr->s_fileno = entry->d_fileno;
     itr->s_reclen = entry->d_reclen;
     itr->s_type   = entry->d_type;
 #endif
 
+    if (itr->s_type == DT_UNKNOWN) {
+      struct stat st;
+
+      if (fstatat(dirfd(d), entry->d_name, &st, AT_SYMLINK_NOFOLLOW) == 0)
+        itr->s_type = entry_type_from_mode(st.st_mode);
+    }
+
 #ifdef DIRENT_NAMLEN_EXISTS_FOOBAR
     itr->s_name   = std::string(entry->d_name, entry->d_name + entry->d_namlen);
 #else

src/utils/directory.h

@@ -2,14 +2,14 @@
 #define RTORRENT_UTILS_DIRECTORY_H
 
 #include <cstdint>
+#include <dirent.h>
 #include <string>
 #include <vector>
 
 namespace utils {
 
 struct directory_entry {
-  // Fix.
-  bool is_file() const { return true; }
+  bool is_file() const { return s_type == DT_REG; }
 
   // The name and types should match POSIX.
   uint32_t            s_fileno;

test/Makefile.am

@@ -69,6 +69,8 @@ rtorrent_Test_Src_SOURCES = $(rtorrent_Test_Common) \
 	src/test_command_string.h \
 	src/test_command_throttle.cc \
 	src/test_command_throttle.h \
+	src/test_session_storer.cc \
+	src/test_session_storer.h \
 	src/test_setup.cc \
 	src/test_setup.h \
 	src/test_ui_download_list.cc \

test/rpc/test_xmlrpc.cc

@@ -192,12 +192,54 @@ TestXmlrpc::test_response_size_limit() {
   CPPUNIT_ASSERT_EQUAL(expected, output);
 }
 
+namespace {
+
+const std::string multicall_method_name =
+  "<member><name>methodName</name><value><string>xmlrpc_reflect</string></value></member>";
+const std::string multicall_params =
+  "<member><name>params</name><value><array><data>"
+  "<value><string></string></value><value><string>a</string></value>"
+  "</data></array></value></member>";
+
+std::string
+multicall_request(const std::string& members) {
+  return "<?xml version=\"1.0\"?><methodCall><methodName>system.multicall</methodName>"
+         "<params><param><value><array><data><value><struct>" + members +
+         "</struct></value></data></array></value></param></params></methodCall>";
+}
+
+}
+
+void
+TestXmlrpc::test_multicall_member_order() {
+  auto call = [this](const std::string& input) {
+    std::string output;
+    m_xmlrpc.process(input.c_str(), input.size(), [&output](const char* c, uint32_t l){ output.append(c, l); return true;});
+    return output;
+  };
+
+  // methodName first is the only order accepted; the positive control proves
+  // the harness drives the real code path rather than a stub.
+  std::string ordered = call(multicall_request(multicall_method_name + multicall_params));
+  CPPUNIT_ASSERT(ordered.find("faultCode") == std::string::npos);
+
+  // params before methodName is rejected with a clear top-level fault,
+  // instead of the "could not find expected element string" of a positional read.
+  std::string expected_fault =
+    "<?xml version=\"1.0\"?><methodResponse><fault><value><struct>"
+    "<member><name>faultCode</name><value><i8>-503</i8></value></member>"
+    "<member><name>faultString</name><value><string>multicall struct's first member must be methodName</string></value></member>"
+    "</struct></value></fault></methodResponse>";
+  CPPUNIT_ASSERT_EQUAL(expected_fault, call(multicall_request(multicall_params + multicall_method_name)));
+}
+
 #else
 
 void TestXmlrpc::test_invalid_utf8() {}
 void TestXmlrpc::test_basics() {}
 void TestXmlrpc::test_size_limit() {}
 void TestXmlrpc::test_response_size_limit() {}
+void TestXmlrpc::test_multicall_member_order() {}
 void TestXmlrpc::setUp() {}
 void TestXmlrpc::tearDown() {}
 

test/rpc/test_xmlrpc.h

@@ -11,6 +11,7 @@ class TestXmlrpc : public test_fixture {
   CPPUNIT_TEST(test_invalid_utf8);
   CPPUNIT_TEST(test_size_limit);
   CPPUNIT_TEST(test_response_size_limit);
+  CPPUNIT_TEST(test_multicall_member_order);
 
   CPPUNIT_TEST_SUITE_END();
 
@@ -24,6 +25,7 @@ public:
   void test_invalid_utf8();
   void test_size_limit();
   void test_response_size_limit();
+  void test_multicall_member_order();
 
 private:
   std::unique_ptr<TestMainThread> m_test_main_thread;

test/src/test_session_storer.cc

@@ -0,0 +1,152 @@
+#include "config.h"
+
+#include "test/src/test_session_storer.h"
+
+#include <cstdlib>
+#include <dirent.h>
+#include <fstream>
+#include <sstream>
+#include <sys/stat.h>
+#include <unistd.h>
+
+#include "session/download_storer.h"
+#include "utils/directory.h"
+
+CPPUNIT_TEST_SUITE_REGISTRATION(TestSessionStorer);
+
+namespace {
+
+const char* entry_name = "0123456789ABCDEF0123456789ABCDEF01234567.torrent";
+const char* link_name  = "FEDCBA9876543210FEDCBA9876543210FEDCBA98.torrent";
+
+void
+write_file(const std::string& path, const std::string& content) {
+  std::ofstream file(path.c_str());
+
+  file << content;
+  file.close();
+
+  CPPUNIT_ASSERT(file.good());
+}
+
+std::string
+read_file(const std::string& path) {
+  std::ifstream    file(path.c_str());
+  std::stringstream buffer;
+
+  buffer << file.rdbuf();
+  return buffer.str();
+}
+
+void
+save_session_files(const std::string& path) {
+  std::stringstream torrent_stream("torrent-data");
+  std::stringstream rtorrent_stream("rtorrent-data");
+  std::stringstream libtorrent_stream("libtorrent-data");
+
+  session::DownloadStorer::save_and_move_streams(path, false, &torrent_stream, &rtorrent_stream, &libtorrent_stream);
+}
+
+unsigned int
+permissions_of(const std::string& path) {
+  struct stat st;
+
+  CPPUNIT_ASSERT_EQUAL(0, ::stat(path.c_str(), &st));
+  return st.st_mode & 07777;
+}
+
+void
+remove_directory(const std::string& path) {
+  DIR* d = ::opendir(path.c_str());
+
+  if (d == NULL)
+    return;
+
+  struct dirent* entry;
+
+  while ((entry = ::readdir(d)) != NULL) {
+    if (entry->d_name[0] == '.' && (entry->d_name[1] == '\0' || (entry->d_name[1] == '.' && entry->d_name[2] == '\0')))
+      continue;
+
+    ::unlink((path + "/" + entry->d_name).c_str());
+  }
+
+  ::closedir(d);
+  ::rmdir(path.c_str());
+}
+
+} // namespace
+
+void
+TestSessionStorer::setUp() {
+  test_fixture::setUp();
+
+  char temp_dir[] = "/tmp/rtorrent_test_session_XXXXXX";
+
+  CPPUNIT_ASSERT(mkdtemp(temp_dir) != nullptr);
+
+  m_temp_dir    = temp_dir;
+  m_session_dir = m_temp_dir + "/session";
+
+  CPPUNIT_ASSERT_EQUAL(0, ::mkdir(m_session_dir.c_str(), 0755));
+}
+
+void
+TestSessionStorer::tearDown() {
+  remove_directory(m_session_dir);
+  remove_directory(m_temp_dir);
+
+  test_fixture::tearDown();
+}
+
+// A symlink planted where the next temporary session file will be written must
+// not redirect the write to the file it points at.
+void
+TestSessionStorer::test_temp_file_symlink_is_not_followed() {
+  auto outside = m_temp_dir + "/outside.txt";
+  auto path    = m_session_dir + "/" + entry_name;
+
+  write_file(outside, "original");
+  CPPUNIT_ASSERT_EQUAL(0, ::symlink(outside.c_str(), (path + ".new").c_str()));
+
+  save_session_files(path);
+
+  CPPUNIT_ASSERT_EQUAL(std::string("original"), read_file(outside));
+  CPPUNIT_ASSERT_EQUAL(std::string("torrent-data"), read_file(path));
+}
+
+// Session files carry tracker announce urls, so they must not be readable by
+// other users regardless of the umask rtorrent was started with.
+void
+TestSessionStorer::test_saved_files_are_owner_only() {
+  auto path       = m_session_dir + "/" + entry_name;
+  auto prev_umask = ::umask(0);
+
+  save_session_files(path);
+  ::umask(prev_umask);
+
+  CPPUNIT_ASSERT_EQUAL(0600u, permissions_of(path));
+  CPPUNIT_ASSERT_EQUAL(0600u, permissions_of(path + ".rtorrent"));
+  CPPUNIT_ASSERT_EQUAL(0600u, permissions_of(path + ".libtorrent_resume"));
+}
+
+// Entries listed for loading must report their real type so that a symlink in
+// the session directory is skipped instead of loaded.
+void
+TestSessionStorer::test_symlinked_entry_is_not_a_file() {
+  write_file(m_temp_dir + "/outside.txt", "d0:e");
+  write_file(m_session_dir + "/" + entry_name, "d0:e");
+
+  CPPUNIT_ASSERT_EQUAL(0, ::symlink((m_temp_dir + "/outside.txt").c_str(), (m_session_dir + "/" + link_name).c_str()));
+
+  auto entries = session::DownloadStorer::get_formated_entries(m_session_dir + "/");
+
+  CPPUNIT_ASSERT_EQUAL(size_t{2}, size_t{entries.size()});
+
+  for (const auto& entry : entries) {
+    if (entry.s_name == entry_name)
+      CPPUNIT_ASSERT(entry.is_file());
+    else
+      CPPUNIT_ASSERT(!entry.is_file());
+  }
+}

test/src/test_session_storer.h

@@ -0,0 +1,25 @@
+#include "test/helpers/test_fixture.h"
+
+#include <string>
+
+class TestSessionStorer : public test_fixture {
+  CPPUNIT_TEST_SUITE(TestSessionStorer);
+
+  CPPUNIT_TEST(test_temp_file_symlink_is_not_followed);
+  CPPUNIT_TEST(test_saved_files_are_owner_only);
+  CPPUNIT_TEST(test_symlinked_entry_is_not_a_file);
+
+  CPPUNIT_TEST_SUITE_END();
+
+public:
+  void setUp();
+  void tearDown();
+
+  void test_temp_file_symlink_is_not_followed();
+  void test_saved_files_are_owner_only();
+  void test_symlinked_entry_is_not_a_file();
+
+private:
+  std::string m_temp_dir;
+  std::string m_session_dir;
+};