From d1befee19e8a764c961914754c66e4158a9f8161 Mon Sep 17 00:00:00 2001
From: ridiculousfish <rf@fishshell.com>
Date: Sat, 8 May 2021 13:11:38 -0700
Subject: [PATCH] Fix some potential leaks in history file contents

If history is corrupt and cannot be read, fish would return an error
without munmaping the file. Ensure it is properly munmapped.
---
 src/history.h        |  1 -
 src/history_file.cpp | 54 +++++++++++++++++++++++---------------------
 src/history_file.h   | 11 ++++++---
 3 files changed, 36 insertions(+), 30 deletions(-)

diff --git a/src/history.h b/src/history.h
index 8b8dc4bb8..3bcb8a48f 100644
--- a/src/history.h
+++ b/src/history.h
@@ -126,7 +126,6 @@ class history_item_t {
 
 typedef std::deque<history_item_t> history_item_list_t;
 
-class history_file_contents_t;
 struct history_impl_t;
 
 class history_t {
diff --git a/src/history_file.cpp b/src/history_file.cpp
index 8ce8028bf..9a741c39e 100644
--- a/src/history_file.cpp
+++ b/src/history_file.cpp
@@ -48,19 +48,6 @@ static bool read_from_fd(int fd, void *address, size_t len) {
     return true;
 }
 
-/// Try to infer the history file type based on inspecting the data.
-static maybe_t<history_file_type_t> infer_file_type(const void *data, size_t len) {
-    maybe_t<history_file_type_t> result{};
-    if (len > 0) {  // old fish started with a #
-        if (static_cast<const char *>(data)[0] == '#') {
-            result = history_type_fish_1_x;
-        } else {  // assume new fish
-            result = history_type_fish_2_0;
-        }
-    }
-    return result;
-}
-
 static void replace_all(std::string *str, const char *needle, const char *replacement) {
     size_t needle_len = std::strlen(needle), replacement_len = std::strlen(replacement);
     size_t offset = 0;
@@ -110,10 +97,20 @@ static void unescape_yaml_fish_2_0(std::string *str) {
 
 history_file_contents_t::~history_file_contents_t() { munmap(const_cast<char *>(start_), length_); }
 
-history_file_contents_t::history_file_contents_t(const char *mmap_start, size_t mmap_length,
-                                                 history_file_type_t type)
-    : start_(mmap_start), length_(mmap_length), type_(type) {
-    assert(mmap_start != MAP_FAILED && "Invalid mmap address");
+history_file_contents_t::history_file_contents_t(const char *mmap_start, size_t mmap_length)
+    : start_(mmap_start), length_(mmap_length) {
+    assert(mmap_start != MAP_FAILED && mmap_length > 0 && "Invalid mmap params");
+}
+
+/// Try to infer the history file type based on inspecting the data.
+bool history_file_contents_t::infer_file_type() {
+    assert(length_ >= 0 && "File shoudl never be empty");
+    if (start_[0] == '#') {
+        this->type_ = history_type_fish_1_x;
+    } else {  // assume new fish
+        this->type_ = history_type_fish_2_0;
+    }
+    return true;
 }
 
 std::unique_ptr<history_file_contents_t> history_file_contents_t::create(int fd) {
@@ -124,11 +121,11 @@ std::unique_ptr<history_file_contents_t> history_file_contents_t::create(int fd)
 
     // Read the file, possibly using mmap.
     void *mmap_start = nullptr;
-    if (should_mmap(fd)) {
+    bool mmap_file_directly = should_mmap(fd);
+    if (mmap_file_directly) {
         // We feel confident to map the file directly. Note this is still risky: if another
         // process truncates the file we risk SIGBUS.
         mmap_start = mmap(nullptr, size_t(len), PROT_READ, MAP_PRIVATE, fd, 0);
-        if (mmap_start == MAP_FAILED) return nullptr;
     } else {
         // We don't want to map the file. mmap some private memory and then read into it. We use
         // mmap instead of malloc so that the destructor can always munmap().
@@ -138,16 +135,21 @@ std::unique_ptr<history_file_contents_t> history_file_contents_t::create(int fd)
 #else
             mmap(0, size_t(len), PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
 #endif
-        if (mmap_start == MAP_FAILED) return nullptr;
-        if (!read_from_fd(fd, mmap_start, len)) return nullptr;
+    }
+    if (mmap_start == MAP_FAILED) return nullptr;
+
+    // Construct the contents right away so it can clean up on error.
+    std::unique_ptr<history_file_contents_t> result(
+        new history_file_contents_t(static_cast<const char *>(mmap_start), len));
+
+    // If we mapped anonymous memory, we have to read from the file.
+    if (!mmap_file_directly && !read_from_fd(fd, mmap_start, len)) {
+        return nullptr;
     }
 
     // Check the file type.
-    auto mtype = infer_file_type(mmap_start, len);
-    if (!mtype) return nullptr;
-
-    return std::unique_ptr<history_file_contents_t>(
-        new history_file_contents_t(static_cast<const char *>(mmap_start), len, *mtype));
+    if (!result->infer_file_type()) return nullptr;
+    return result;
 }
 
 history_item_t history_file_contents_t::decode_item(size_t offset) const {
diff --git a/src/history_file.h b/src/history_file.h
index bdd24cfc8..53f1257fd 100644
--- a/src/history_file.h
+++ b/src/history_file.h
@@ -53,16 +53,21 @@ class history_file_contents_t {
 
    private:
     // The memory mapped pointer.
-    const char *start_;
+    const char *const start_;
 
     // The mapped length.
     const size_t length_;
 
     // The type of the mapped file.
-    const history_file_type_t type_;
+    // This is set at construction and not changed after.
+    history_file_type_t type_{};
 
     // Private constructor; use the static create() function.
-    history_file_contents_t(const char *mmap_start, size_t mmap_length, history_file_type_t type);
+    history_file_contents_t(const char *mmap_start, size_t mmap_length);
+
+    // Try to infer the file type to populate type_.
+    // \return true on success, false on error.
+    bool infer_file_type();
 
     history_file_contents_t(history_file_contents_t &&) = delete;
     void operator=(history_file_contents_t &&) = delete;