Handle read errors safely in StackLineReader to prevent buffer overreads

src/stack_line_reader.c

@@ -31,9 +31,12 @@ void StackLineReader_Initialize(StackLineReader* reader, int fd) {
 static int LoadFullBuffer(StackLineReader* reader) {
   const int read = CpuFeatures_ReadFile(reader->fd, reader->buffer,
                                         STACK_LINE_READER_BUFFER_SIZE);
-  assert(read >= 0);
   reader->view.ptr = reader->buffer;
-  reader->view.size = read;
+  if (read <= 0) {
+    reader->view.size = 0;
+    return read < 0 ? -1 : 0;
+  }
+  reader->view.size = (size_t)read;
   return read;
 }
 
@@ -42,9 +45,11 @@ static int LoadMore(StackLineReader* reader) {
   char* const ptr = reader->buffer + reader->view.size;
   const size_t size_to_read = STACK_LINE_READER_BUFFER_SIZE - reader->view.size;
   const int read = CpuFeatures_ReadFile(reader->fd, ptr, size_to_read);
-  assert(read >= 0);
+  if (read <= 0) {
+    return read < 0 ? -1 : 0;
+  }
   assert(read <= (int)size_to_read);
-  reader->view.size += read;
+  reader->view.size += (size_t)read;
   return read;
 }
 
@@ -67,7 +72,7 @@ static int BringToFrontAndLoadMore(StackLineReader* reader) {
 static void SkipToNextLine(StackLineReader* reader) {
   for (;;) {
     const int read = LoadFullBuffer(reader);
-    if (read == 0) {
+    if (read <= 0) {
       break;
     } else {
       const int eol_index = IndexOfEol(reader);
@@ -112,7 +117,7 @@ LineResult StackLineReader_NextLine(StackLineReader* reader) {
     int eol_index = IndexOfEol(reader);
     if (eol_index < 0 && can_load_more) {
       const int read = BringToFrontAndLoadMore(reader);
-      if (read == 0) {
+      if (read <= 0) {
         return CreateEOFLineResult(reader->view);
       }
       eol_index = IndexOfEol(reader);

test/filesystem_for_testing.cc

@@ -38,6 +38,7 @@ void FakeFile::Close() {
 }
 
 int FakeFile::Read(int fd, void* buf, size_t count) {
+  if (read_error_) return -1;
   assert(count < INT_MAX);
   assert(fd == file_descriptor_);
   const size_t remainder = content_.size() - head_index_;
@@ -63,6 +64,16 @@ FakeFile* FakeFilesystem::FindFileOrNull(const std::string& filename) const {
   return itr == files_.end() ? nullptr : itr->second.get();
 }
 
+FakeFile* FakeFilesystem::FindFileOrNull(const int file_descriptor) const {
+  for (const auto& filename_file_pair : files_) {
+    FakeFile* const file_ptr = filename_file_pair.second.get();
+    if (file_ptr->GetFileDescriptor() == file_descriptor) {
+      return file_ptr;
+    }
+  }
+  return nullptr;
+}
+
 FakeFile* FakeFilesystem::FindFileOrDie(const int file_descriptor) const {
   for (const auto& filename_file_pair : files_) {
     FakeFile* const file_ptr = filename_file_pair.second.get();
@@ -91,13 +102,17 @@ extern "C" int CpuFeatures_OpenFile(const char* filename) {
 }
 
 extern "C" void CpuFeatures_CloseFile(int file_descriptor) {
-  kFilesystem->FindFileOrDie(file_descriptor)->Close();
+  auto* const file = kFilesystem->FindFileOrNull(file_descriptor);
+  if (file) {
+    file->Close();
+  }
 }
 
 extern "C" int CpuFeatures_ReadFile(int file_descriptor, void* buffer,
                                     size_t buffer_size) {
-  return kFilesystem->FindFileOrDie(file_descriptor)
-      ->Read(file_descriptor, buffer, buffer_size);
+  auto* const file = kFilesystem->FindFileOrNull(file_descriptor);
+  if (!file) return -1;
+  return file->Read(file_descriptor, buffer, buffer_size);
 }
 
 }  // namespace cpu_features

test/filesystem_for_testing.h

@@ -32,13 +32,15 @@ class FakeFile {
   void Open();
   void Close();
   int Read(int fd, void* buf, size_t count);
+  void SetReadError(bool read_error) { read_error_ = read_error; }
 
   int GetFileDescriptor() const { return file_descriptor_; }
 
  private:
   const int file_descriptor_;
   const std::string content_;
   bool opened_ = false;
+  bool read_error_ = false;
   size_t head_index_ = 0;
 };
 
@@ -48,6 +50,7 @@ class FakeFilesystem {
   FakeFile* CreateFile(const std::string& filename, const char* content);
   FakeFile* FindFileOrDie(const int file_descriptor) const;
   FakeFile* FindFileOrNull(const std::string& filename) const;
+  FakeFile* FindFileOrNull(const int file_descriptor) const;
 
  private:
   int next_file_descriptor_ = 0;

test/stack_line_reader_test.cc

@@ -128,5 +128,58 @@ Another line that is too long)");
   }
 }
 
+TEST(StackLineReaderTest, ReadErrorInvalidFd) {
+  StackLineReader reader;
+  StackLineReader_Initialize(&reader, -1);
+  const auto result = StackLineReader_NextLine(&reader);
+  EXPECT_TRUE(result.eof);
+  EXPECT_TRUE(result.full_line);
+  EXPECT_EQ(result.line, str(""));
+}
+
+TEST(StackLineReaderTest, ReadErrorMidStream) {
+  auto& fs = GetEmptyFilesystem();
+  auto* file = fs.CreateFile("/proc/cpuinfo", "First line\nSecond line\n");
+
+  StackLineReader reader;
+  StackLineReader_Initialize(&reader, file->GetFileDescriptor());
+  {
+    const auto result = StackLineReader_NextLine(&reader);
+    EXPECT_FALSE(result.eof);
+    EXPECT_TRUE(result.full_line);
+    EXPECT_EQ(result.line, str("First line"));
+  }
+  // Simulate read error on subsequent read.
+  file->SetReadError(true);
+  {
+    const auto result = StackLineReader_NextLine(&reader);
+    EXPECT_TRUE(result.eof);
+    EXPECT_TRUE(result.full_line);
+  }
+}
+
+TEST(StackLineReaderTest, ReadErrorInSkipMode) {
+  auto& fs = GetEmptyFilesystem();
+  auto* file =
+      fs.CreateFile("/proc/cpuinfo", "More than 16 characters\nSecond line");
+
+  StackLineReader reader;
+  StackLineReader_Initialize(&reader, file->GetFileDescriptor());
+  {
+    const auto result = StackLineReader_NextLine(&reader);
+    EXPECT_FALSE(result.eof);
+    EXPECT_FALSE(result.full_line);
+    EXPECT_EQ(result.line, str("More than 16 cha"));
+  }
+  // Simulate read error during skip_mode.
+  file->SetReadError(true);
+  {
+    const auto result = StackLineReader_NextLine(&reader);
+    EXPECT_TRUE(result.eof);
+    EXPECT_TRUE(result.full_line);
+    EXPECT_EQ(result.line, str(""));
+  }
+}
+
 }  // namespace
 }  // namespace cpu_features