From 7d4a30af56a7376f1c0cf55673abd5776e21f8a1 Mon Sep 17 00:00:00 2001 From: Ali Mohammad Pur Date: Fri, 20 Jan 2023 21:00:40 +0330 Subject: [PATCH] LibCore: Avoid logical OOB read in AllocatingMemoryStream::offset_of() The previous impl was trimming the last chunk to the free space instead of the used space, which yielded an OOB read if the needle wasn't found. --- Tests/LibCore/TestLibCoreStream.cpp | 24 +++++++++++++++++++++ Userland/Libraries/LibCore/MemoryStream.cpp | 2 +- 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/Tests/LibCore/TestLibCoreStream.cpp b/Tests/LibCore/TestLibCoreStream.cpp index a6e3963337d..9467e1b2ab9 100644 --- a/Tests/LibCore/TestLibCoreStream.cpp +++ b/Tests/LibCore/TestLibCoreStream.cpp @@ -657,6 +657,30 @@ TEST_CASE(allocating_memory_stream_offset_of) } } +TEST_CASE(allocating_memory_stream_offset_of_oob) +{ + Core::Stream::AllocatingMemoryStream stream; + // NOTE: This test is to make sure that offset_of() doesn't read past the end of the "initialized" data. + // So we have to assume some things about the behaviour of this class: + // - The chunk size is 4096 bytes. + // - A chunk is moved to the end when it's fully read from + // - A free chunk is used as-is, no new ones are allocated if one exists. + + // First, fill exactly one chunk. + for (size_t i = 0; i < 256; ++i) + MUST(stream.write_entire_buffer("AAAAAAAAAAAAAAAA"sv.bytes())); + + // Then discard it all. + MUST(stream.discard(4096)); + // Now we can write into this chunk again, knowing that it's initialized to all 'A's. + MUST(stream.write_entire_buffer("Well Hello Friends! :^)"sv.bytes())); + + { + auto offset = MUST(stream.offset_of("A"sv.bytes())); + EXPECT(!offset.has_value()); + } +} + TEST_CASE(allocating_memory_stream_10kb) { auto file = MUST(Core::Stream::File::open("/usr/Tests/LibCore/10kb.txt"sv, Core::Stream::OpenMode::Read)); diff --git a/Userland/Libraries/LibCore/MemoryStream.cpp b/Userland/Libraries/LibCore/MemoryStream.cpp index 625a1edcb9c..1d4d0361862 100644 --- a/Userland/Libraries/LibCore/MemoryStream.cpp +++ b/Userland/Libraries/LibCore/MemoryStream.cpp @@ -219,7 +219,7 @@ ErrorOr> AllocatingMemoryStream::offset_of(ReadonlyBytes needle } // Trimming is done first to ensure that we don't unintentionally shift around if the first and last chunks are the same. - search_spans[chunk_count - 1] = search_spans[chunk_count - 1].trim(chunk_count * chunk_size - m_write_offset); + search_spans[chunk_count - 1] = search_spans[chunk_count - 1].trim(m_write_offset % chunk_size); search_spans[0] = search_spans[0].slice(m_read_offset); return AK::memmem(search_spans.begin(), search_spans.end(), needle);