From e3fc6c0bc8a1fe6b22426a128ca45b46ea672052 Mon Sep 17 00:00:00 2001 From: jdymitarai Date: Thu, 10 Sep 2026 11:13:55 +0800 Subject: [PATCH] Handle read errors safely in StackLineReader to prevent buffer overreads 1. Check read <= 0 in LoadFullBuffer and LoadMore to prevent integer underflow and out-of-bounds reads in release builds where assert() is disabled. 2. Gracefully treat read errors as EOF in SkipToNextLine and StackLineReader_NextLine. 3. Support read error simulation in test filesystem harness. 4. Add comprehensive unit tests covering invalid file descriptor, mid-stream read failure, and read errors during line truncation skip mode. --- src/stack_line_reader.c | 17 +++++++---- test/filesystem_for_testing.cc | 21 ++++++++++++-- test/filesystem_for_testing.h | 3 ++ test/stack_line_reader_test.cc | 53 ++++++++++++++++++++++++++++++++++ 4 files changed, 85 insertions(+), 9 deletions(-) diff --git a/src/stack_line_reader.c b/src/stack_line_reader.c index ffc778d3..56bc6cc9 100644 --- a/src/stack_line_reader.c +++ b/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); diff --git a/test/filesystem_for_testing.cc b/test/filesystem_for_testing.cc index 648a53e3..672f17c7 100644 --- a/test/filesystem_for_testing.cc +++ b/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 diff --git a/test/filesystem_for_testing.h b/test/filesystem_for_testing.h index ef717fdf..5a9e5325 100644 --- a/test/filesystem_for_testing.h +++ b/test/filesystem_for_testing.h @@ -32,6 +32,7 @@ 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_; } @@ -39,6 +40,7 @@ class FakeFile { 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; diff --git a/test/stack_line_reader_test.cc b/test/stack_line_reader_test.cc index 9ac5388d..92f4a9ee 100644 --- a/test/stack_line_reader_test.cc +++ b/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