diff --git a/src/basicio.cpp b/src/basicio.cpp index ea0cb9d1..8b8a48dc 100644 --- a/src/basicio.cpp +++ b/src/basicio.cpp @@ -1291,7 +1291,7 @@ namespace Exiv2 { #if defined(_MSC_VER) int MemIo::seek( int64_t offset, Position pos ) { - uint64_t newIdx = 0; + int64_t newIdx = 0; switch (pos) { case BasicIo::cur: newIdx = p_->idx_ + offset; break; @@ -1299,7 +1299,15 @@ namespace Exiv2 { case BasicIo::end: newIdx = p_->size_ + offset; break; } - p_->idx_ = static_cast(newIdx); //not very sure about this. need more test!! - note by Shawn fly2xj@gmail.com //TODO + if (newIdx < 0) + return 1; + + if (static_cast(newIdx) > p_->size_) { + p_->eof_ = true; + return 1; + } + + p_->idx_ = static_cast(newIdx); p_->eof_ = false; return 0; } @@ -1314,7 +1322,14 @@ namespace Exiv2 { case BasicIo::end: newIdx = p_->size_ + offset; break; } - if (newIdx < 0) return 1; + if (newIdx < 0) + return 1; + + if (newIdx > p_->size_) { + p_->eof_ = true; + return 1; + } + p_->idx_ = newIdx; p_->eof_ = false; return 0; diff --git a/unitTests/CMakeLists.txt b/unitTests/CMakeLists.txt index 6b73d81d..f4340834 100644 --- a/unitTests/CMakeLists.txt +++ b/unitTests/CMakeLists.txt @@ -19,6 +19,7 @@ endforeach() add_executable(unit_tests mainTestRunner.cpp gtestwrapper.h + test_basicio.cpp test_types.cpp test_tiffheader.cpp test_futils.cpp diff --git a/unitTests/test_basicio.cpp b/unitTests/test_basicio.cpp new file mode 100644 index 00000000..2007b931 --- /dev/null +++ b/unitTests/test_basicio.cpp @@ -0,0 +1,77 @@ +#include +#include + +using namespace Exiv2; + +TEST(MemIo, seek_out_of_bounds_00) +{ + byte buf[1024]; + memset(buf, 0, sizeof(buf)); + + MemIo io(buf, sizeof(buf)); + ASSERT_FALSE(io.eof()); + + // Regression test for bug reported in https://github.com/Exiv2/exiv2/pull/945 + // The problem is that MemIo::seek() does not check that the new offset is + // in bounds. + byte tmp[16]; + ASSERT_EQ(io.seek(0x10000000, BasicIo::beg), 1); + ASSERT_TRUE(io.eof()); + + // The seek was invalid, so the offset didn't change and this read still works. + ASSERT_EQ(io.read(tmp, sizeof(tmp)), sizeof(tmp)); +} + +TEST(MemIo, seek_out_of_bounds_01) +{ + byte buf[1024]; + memset(buf, 0, sizeof(buf)); + + MemIo io(buf, sizeof(buf)); + ASSERT_FALSE(io.eof()); + + byte tmp[16]; + + // Seek to the end of the file. + ASSERT_EQ(io.seek(0, BasicIo::end), 0); + ASSERT_EQ(io.read(tmp, sizeof(tmp)), 0); + + // Try to seek past the end of the file. + ASSERT_EQ(io.seek(0x10000000, BasicIo::end), 1); + ASSERT_TRUE(io.eof()); + ASSERT_EQ(io.read(tmp, sizeof(tmp)), 0); +} + +TEST(MemIo, seek_out_of_bounds_02) +{ + byte buf[1024]; + memset(buf, 0, sizeof(buf)); + + MemIo io(buf, sizeof(buf)); + ASSERT_FALSE(io.eof()); + + byte tmp[16]; + + // Try to seek past the end of the file. + ASSERT_EQ(io.seek(0x10000000, BasicIo::cur), 1); + ASSERT_TRUE(io.eof()); + // The seek was invalid, so the offset didn't change and this read still works. + ASSERT_EQ(io.read(tmp, sizeof(tmp)), sizeof(tmp)); +} + +TEST(MemIo, seek_out_of_bounds_03) +{ + byte buf[1024]; + memset(buf, 0, sizeof(buf)); + + MemIo io(buf, sizeof(buf)); + ASSERT_FALSE(io.eof()); + + byte tmp[16]; + + // Try to seek past the beginning of the file. + ASSERT_EQ(io.seek(-0x10000000, BasicIo::cur), 1); + ASSERT_FALSE(io.eof()); + // The seek was invalid, so the offset didn't change and this read still works. + ASSERT_EQ(io.read(tmp, sizeof(tmp)), sizeof(tmp)); +}