From f85f6a0127455d47cd7eefd4fc80117ee6e8e12c Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sat, 8 May 2021 16:02:49 -0700 Subject: [PATCH] Enforce that history items must end with trailing newlines This helps prevent seeing partially written items from other sessions, in preparation to reducing the amount of flocking done. --- src/history_file.cpp | 50 ++++++++++++++++++----------------- src/history_file.h | 7 +++-- tests/history_sample_fish_1_x | 1 + tests/history_sample_fish_2_0 | 1 + 4 files changed, 33 insertions(+), 26 deletions(-) diff --git a/src/history_file.cpp b/src/history_file.cpp index 3d232e505..8e2d3e5cf 100644 --- a/src/history_file.cpp +++ b/src/history_file.cpp @@ -11,10 +11,10 @@ static history_item_t decode_item_fish_2_0(const char *base, size_t len); static history_item_t decode_item_fish_1_x(const char *begin, size_t length); -static size_t offset_of_next_item_fish_2_0(const history_file_contents_t &contents, - size_t *inout_cursor, time_t cutoff_timestamp); -static size_t offset_of_next_item_fish_1_x(const char *begin, size_t mmap_length, - size_t *inout_cursor); +static maybe_t offset_of_next_item_fish_2_0(const history_file_contents_t &contents, + size_t *inout_cursor, time_t cutoff_timestamp); +static maybe_t offset_of_next_item_fish_1_x(const char *begin, size_t mmap_length, + size_t *inout_cursor); // Check if we should mmap the fd. // Don't try mmap() on non-local filesystems. @@ -142,6 +142,11 @@ history_file_contents_t::history_file_contents_t(std::unique_ptr assert(region_ && start_ && length_ > 0 && "Invalid params"); } +history_file_contents_t::history_file_contents_t(const char *start, size_t length) + : start_(start), length_(length) { + // Construct from explicit data, not backed by a file. This is used in tests. +} + /// Try to infer the history file type based on inspecting the data. bool history_file_contents_t::infer_file_type() { assert(length_ > 0 && "File should never be empty"); @@ -189,19 +194,13 @@ history_item_t history_file_contents_t::decode_item(size_t offset) const { } maybe_t history_file_contents_t::offset_of_next_item(size_t *cursor, time_t cutoff) const { - auto offset = size_t(-1); switch (this->type()) { case history_type_fish_2_0: - offset = offset_of_next_item_fish_2_0(*this, cursor, cutoff); - break; + return offset_of_next_item_fish_2_0(*this, cursor, cutoff); case history_type_fish_1_x: - offset = offset_of_next_item_fish_1_x(this->begin(), this->length(), cursor); - break; + return offset_of_next_item_fish_1_x(this->begin(), this->length(), cursor); } - if (offset == size_t(-1)) { - return none(); - } - return offset; + return none(); } /// Read one line, stripping off any newline, and updating cursor. Note that our input string is NOT @@ -366,10 +365,10 @@ static const char *next_line(const char *start, const char *end) { /// Pass the file contents and a pointer to a cursor size_t, initially 0. /// If custoff_timestamp is nonzero, skip items created at or after that timestamp. /// Returns (size_t)-1 when done. -static size_t offset_of_next_item_fish_2_0(const history_file_contents_t &contents, - size_t *inout_cursor, time_t cutoff_timestamp) { +static maybe_t offset_of_next_item_fish_2_0(const history_file_contents_t &contents, + size_t *inout_cursor, time_t cutoff_timestamp) { size_t cursor = *inout_cursor; - auto result = size_t(-1); + maybe_t result = none(); const size_t length = contents.length(); const char *const begin = contents.begin(); const char *const end = contents.end(); @@ -449,12 +448,10 @@ static size_t offset_of_next_item_fish_2_0(const history_file_contents_t &conten } // We made it through the gauntlet. - result = line_start - begin; - break; //!OCLINT(avoid branching statement as last in loop) + *inout_cursor = cursor; + return line_start - begin; } - - *inout_cursor = cursor; - return result; + return none(); } void append_history_item_to_buffer(const history_item_t &item, std::string *buffer) { @@ -564,9 +561,9 @@ static history_item_t decode_item_fish_1_x(const char *begin, size_t length) { /// Same as offset_of_next_item_fish_2_0, but for fish 1.x (pre fishfish). /// Adapted from history_populate_from_mmap in history.c -static size_t offset_of_next_item_fish_1_x(const char *begin, size_t mmap_length, - size_t *inout_cursor) { - if (mmap_length == 0 || *inout_cursor >= mmap_length) return static_cast(-1); +static maybe_t offset_of_next_item_fish_1_x(const char *begin, size_t mmap_length, + size_t *inout_cursor) { + if (mmap_length == 0 || *inout_cursor >= mmap_length) return none(); const char *end = begin + mmap_length; const char *pos; @@ -592,6 +589,11 @@ static size_t offset_of_next_item_fish_1_x(const char *begin, size_t mmap_length } } + if (pos == end && !all_done) { + // No trailing newline, treat this item as incomplete and ignore it. + return none(); + } + *inout_cursor = (pos - begin); return result; } diff --git a/src/history_file.h b/src/history_file.h index 29bc57442..201d2fb11 100644 --- a/src/history_file.h +++ b/src/history_file.h @@ -12,6 +12,7 @@ #include "maybe.h" class history_item_t; +class history_tests_t; // History file types. enum history_file_type_t { history_type_fish_2_0, history_type_fish_1_x }; @@ -57,7 +58,8 @@ class history_file_contents_t { const std::unique_ptr region_{}; // The memory mapped pointer and length. - // These just alias the mapped region. + // The ptr aliases our region. The length may be slightly smaller, if there is a trailing + // incomplete history item. const char *const start_; const size_t length_; @@ -65,8 +67,9 @@ class history_file_contents_t { // This is set at construction and not changed after. history_file_type_t type_{}; - // Private constructor; use the static create() function. + // Private constructors; use the static create() function. explicit history_file_contents_t(std::unique_ptr region); + history_file_contents_t(const char *start, size_t length); // Try to infer the file type to populate type_. // \return true on success, false on error. diff --git a/tests/history_sample_fish_1_x b/tests/history_sample_fish_1_x index dd09d4cb6..aaeedac03 100644 --- a/tests/history_sample_fish_1_x +++ b/tests/history_sample_fish_1_x @@ -10,3 +10,4 @@ end echo #abc # 1339520884 #def +echo no trailing newline ignore me \ No newline at end of file diff --git a/tests/history_sample_fish_2_0 b/tests/history_sample_fish_2_0 index f44bbfc0c..437cc2237 100644 --- a/tests/history_sample_fish_2_0 +++ b/tests/history_sample_fish_2_0 @@ -4,3 +4,4 @@ when: 1339717377 - cmd: echo this has\\\nbackslashes when: 1339717385 +- cmd: I should be ignored no trailing newline \ No newline at end of file