fix: line termination mismatch in hex files (philips-software#307)

* fix: incorrect range returned by PeekContinuousRange * fix: Discard trailing spaces (\r) in hex files * Update upgrade/pack_builder/BinaryObject.cpp Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * Process review comments * Update upgrade/pack_builder/BinaryObject.cpp Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * fix formatting * Use std::ispace --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
gabrielfrasantos · May 31, 2023 · 47482c3 · 47482c3
1 parent 320356b
commit 47482c3
Show file tree

Hide file tree

Showing 5 changed files with 32 additions and 6 deletions.
diff --git a/infra/stream/StdStringInputStream.cpp b/infra/stream/StdStringInputStream.cpp
@@ -38,8 +38,7 @@ namespace infra
 
     ConstByteRange StdStringInputStreamReader::PeekContiguousRange(std::size_t start)
     {
-        ConstByteRange result(reinterpret_cast<const uint8_t*>(string.data()) + offset + start, reinterpret_cast<const uint8_t*>(string.size()));
-
+        ConstByteRange result(reinterpret_cast<const uint8_t*>(string.data()) + offset + start, reinterpret_cast<const uint8_t*>(string.data()) + string.size());
         return result;
     }
 

diff --git a/infra/stream/test/TestStdStringInputStream.cpp b/infra/stream/test/TestStdStringInputStream.cpp
@@ -50,3 +50,13 @@ TEST(StdStringInputStreamTest, PeekLiteral)
     stream.Consume(1);
     EXPECT_TRUE(stream.Empty());
 }
+
+TEST(StdStringInputStreamTest, PeekContiguousRange_returns_expected_range_size)
+{
+    infra::StdStringInputStream::WithStorage stream(infra::inPlace, "abcdef");
+
+    EXPECT_EQ(6, stream.PeekContiguousRange().size());
+
+    stream.Consume(2);
+    EXPECT_EQ(4, stream.PeekContiguousRange().size());
+}
diff --git a/upgrade/pack_builder/BinaryObject.cpp b/upgrade/pack_builder/BinaryObject.cpp
@@ -2,6 +2,15 @@
 #include "infra/stream/StdStringInputStream.hpp"
 #include <algorithm>
 
+namespace
+{
+    bool StringHasNonSpaces(const std::string_view string)
+    {
+        return !std::all_of(string.begin(), string.end(), [](auto c)
+            { return std::isspace(c); });
+    }
+}
+
 namespace application
 {
     LineException::LineException(const std::string& prependMessage, const std::string& file, int line)
@@ -39,7 +48,7 @@ namespace application
         this->offset = offset;
 
         int lineNumber = 0;
-        for (auto line : data)
+        for (const auto& line : data)
         {
             ++lineNumber;
             if (!line.empty())
@@ -148,7 +157,7 @@ namespace application
             memory.Insert(lineContents.data[i], linearAddress + offset + lineContents.address + i);
     }
 
-    BinaryObject::LineContents::LineContents(std::string line, const std::string& fileName, int lineNumber)
+    BinaryObject::LineContents::LineContents(const std::string& line, const std::string& fileName, int lineNumber)
     {
         infra::StdStringInputStream stream(line, infra::softFail);
 
@@ -178,7 +187,8 @@ namespace application
             throw RecordTooShortException(fileName, lineNumber);
         if (sum != 0)
             throw IncorrectCrcException(fileName, lineNumber);
-        if (!stream.Empty())
+
+        if (!stream.Empty() && StringHasNonSpaces(std::string_view(line).substr(line.size() - stream.Available())))
             throw RecordTooLongException(fileName, lineNumber);
     }
 }
diff --git a/upgrade/pack_builder/BinaryObject.hpp b/upgrade/pack_builder/BinaryObject.hpp
@@ -68,7 +68,7 @@ namespace application
     private:
         struct LineContents
         {
-            LineContents(std::string line, const std::string& fileName, int lineNumber);
+            LineContents(const std::string& line, const std::string& fileName, int lineNumber);
 
             uint8_t recordType;
             uint16_t address;

diff --git a/upgrade/pack_builder/test/TestBinaryObject.cpp b/upgrade/pack_builder/test/TestBinaryObject.cpp
@@ -127,6 +127,13 @@ TEST(BinaryObjectTest, Hex_TooLongRecord)
     EXPECT_THROW(object.AddHex(std::vector<std::string>{ ":0100000001fe0" }, 0, "file"), application::RecordTooLongException);
 }
 
+TEST(BinaryObjectTest, Hex_TrailingLineTerminator)
+{
+    application::BinaryObject object;
+    object.AddHex(std::vector<std::string>{ ":00000001FF\r" }, 0, "file");
+    EXPECT_EQ(application::SparseVector<uint8_t>{}, object.Memory());
+}
+
 TEST(BinaryObjectTest, Bin_AddByte)
 {
     application::BinaryObject object;