tequilaOS/platform_system_core
2
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity

Fix SEGV in libziparchive with malformed zip file.

Browse source 
d77c99ebc3 changed MappedFile to return a
bogus zero-length mapping on failure rather than nullptr. None of the
calling code was changed, though, and it seems like doing so would be a
bad idea. Revert that part of the change.

Add missing tests, and tidy up some of the logging. Also remove
single-use or obfuscatory constants from the tests.

The new "empty.zip" was created by using zip(1) to create a zip file
with one entry, then using `zip -d` to remove it.

The new "zero-size-cd.zip" was created by using zip(1) to create a zip
file containing a single empty file, and then hex editing the two byte
"size of the central directory" field in the "end of central directory
record" structure at the end of the file. (This is equivalent to, but
much smaller than, the example zip file provided by the bug reporter.)

Bug: http://b/145925341
Test: treehugger
Change-Id: Iff64673bce7dae886ccbc9dd6c2bbe18de19f9d2
This commit is contained in:
Elliott Hughes 2019-12-16 16:16:16 -08:00
parent 5a07ae1422
commit fba2a1a1ec
9 changed files with 81 additions and 86 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

8
adb/fastdeploy/deploypatchgenerator/apk_archive.cpp
View file

@ -36,7 +36,7 @@ struct FileRegion {
FileRegion(borrowed_fd fd, off64_t offset, size_t length)
: mapped_(android::base::MappedFile::FromOsHandle(adb_get_os_handle(fd), offset, length,
PROT_READ)) {
if (mapped_.data() != nullptr) {
if (mapped_ != nullptr) {
return;
}
@ -50,14 +50,14 @@ struct FileRegion {
}
}
const char* data() const { return mapped_.data() ? mapped_.data() : buffer_.data(); }
size_t size() const { return mapped_.data() ? mapped_.size() : buffer_.size(); }
const char* data() const { return mapped_ ? mapped_->data() : buffer_.data(); }
size_t size() const { return mapped_ ? mapped_->size() : buffer_.size(); }
private:
FileRegion() = default;
DISALLOW_COPY_AND_ASSIGN(FileRegion);
android::base::MappedFile mapped_;
std::unique_ptr<android::base::MappedFile> mapped_;
std::string buffer_;
};
} // namespace

7
base/include/android-base/mapped_file.h
View file

@ -53,7 +53,8 @@ class MappedFile {
/**
* Same thing, but using the raw OS file handle instead of a CRT wrapper.
*/
static MappedFile FromOsHandle(os_handle h, off64_t offset, size_t length, int prot);
static std::unique_ptr<MappedFile> FromOsHandle(os_handle h, off64_t offset, size_t length,
int prot);
/**
* Removes the mapping.
@ -69,10 +70,6 @@ class MappedFile {
char* data() const { return base_ + offset_; }
size_t size() const { return size_; }
bool isValid() const { return base_ != nullptr; }
explicit operator bool() const { return isValid(); }
private:
DISALLOW_IMPLICIT_CONSTRUCTORS(MappedFile);

25
base/mapped_file.cpp
View file

@ -38,15 +38,14 @@ static off64_t InitPageSize() {
std::unique_ptr<MappedFile> MappedFile::FromFd(borrowed_fd fd, off64_t offset, size_t length,
int prot) {
#if defined(_WIN32)
auto file =
FromOsHandle(reinterpret_cast<HANDLE>(_get_osfhandle(fd.get())), offset, length, prot);
return FromOsHandle(reinterpret_cast<HANDLE>(_get_osfhandle(fd.get())), offset, length, prot);
#else
auto file = FromOsHandle(fd.get(), offset, length, prot);
return FromOsHandle(fd.get(), offset, length, prot);
#endif
return file ? std::make_unique<MappedFile>(std::move(file)) : std::unique_ptr<MappedFile>{};
}
MappedFile MappedFile::FromOsHandle(os_handle h, off64_t offset, size_t length, int prot) {
std::unique_ptr<MappedFile> MappedFile::FromOsHandle(os_handle h, off64_t offset, size_t length,
int prot) {
static const off64_t page_size = InitPageSize();
size_t slop = offset % page_size;
off64_t file_offset = offset - slop;
@ -59,28 +58,30 @@ MappedFile MappedFile::FromOsHandle(os_handle h, off64_t offset, size_t length,
// http://b/119818070 "app crashes when reading asset of zero length".
// Return a MappedFile that's only valid for reading the size.
if (length == 0 && ::GetLastError() == ERROR_FILE_INVALID) {
return MappedFile{const_cast<char*>(kEmptyBuffer), 0, 0, nullptr};
return std::unique_ptr<MappedFile>(
new MappedFile(const_cast<char*>(kEmptyBuffer), 0, 0, nullptr));
}
return MappedFile(nullptr, 0, 0, nullptr);
return nullptr;
}
void* base = MapViewOfFile(handle, (prot & PROT_WRITE) ? FILE_MAP_ALL_ACCESS : FILE_MAP_READ, 0,
file_offset, file_length);
if (base == nullptr) {
CloseHandle(handle);
return MappedFile(nullptr, 0, 0, nullptr);
return nullptr;
}
return MappedFile{static_cast<char*>(base), length, slop, handle};
return std::unique_ptr<MappedFile>(
new MappedFile(static_cast<char*>(base), length, slop, handle));
#else
void* base = mmap(nullptr, file_length, prot, MAP_SHARED, h, file_offset);
if (base == MAP_FAILED) {
// http://b/119818070 "app crashes when reading asset of zero length".
// mmap fails with EINVAL for a zero length region.
if (errno == EINVAL && length == 0) {
return MappedFile{const_cast<char*>(kEmptyBuffer), 0, 0};
return std::unique_ptr<MappedFile>(new MappedFile(const_cast<char*>(kEmptyBuffer), 0, 0));
}
return MappedFile(nullptr, 0, 0);
return nullptr;
}
return MappedFile{static_cast<char*>(base), length, slop};
return std::unique_ptr<MappedFile>(new MappedFile(static_cast<char*>(base), length, slop));
#endif
}

2
base/mapped_file_test.cpp
View file

@ -44,8 +44,6 @@ TEST(mapped_file, zero_length_mapping) {
ASSERT_TRUE(tf.fd != -1);
auto m = android::base::MappedFile::FromFd(tf.fd, 4096, 0, PROT_READ);
ASSERT_NE(nullptr, m);
EXPECT_TRUE((bool)*m);
EXPECT_EQ(0u, m->size());
EXPECT_NE(nullptr, m->data());
}

BIN
libziparchive/testdata/empty.zip vendored Normal file
View file

Binary file not shown.

BIN
libziparchive/testdata/zero-size-cd.zip vendored Normal file
View file

Binary file not shown.

55
libziparchive/zip_archive.cc
View file

@ -265,14 +265,10 @@ static int32_t MapCentralDirectory0(const char* debug_file_name, ZipArchive* arc
ALOGV("+++ num_entries=%" PRIu32 " dir_size=%" PRIu32 " dir_offset=%" PRIu32, eocd->num_records,
eocd->cd_size, eocd->cd_start_offset);
/*
* It all looks good. Create a mapping for the CD, and set the fields
* in archive.
*/
// It all looks good. Create a mapping for the CD, and set the fields
// in archive.
if (!archive->InitializeCentralDirectory(static_cast<off64_t>(eocd->cd_start_offset),
static_cast<size_t>(eocd->cd_size))) {
ALOGE("Zip: failed to intialize central directory.\n");
return kMmapFailed;
}
@ -354,7 +350,7 @@ static int32_t ParseZipArchive(ZipArchive* archive) {
if (archive->hash_table == nullptr) {
ALOGW("Zip: unable to allocate the %u-entry hash_table, entry size: %zu",
archive->hash_table_size, sizeof(ZipStringOffset));
return -1;
return kAllocationFailed;
}
/*
@ -365,24 +361,25 @@ static int32_t ParseZipArchive(ZipArchive* archive) {
const uint8_t* ptr = cd_ptr;
for (uint16_t i = 0; i < num_entries; i++) {
if (ptr > cd_end - sizeof(CentralDirectoryRecord)) {
ALOGW("Zip: ran off the end (at %" PRIu16 ")", i);
ALOGW("Zip: ran off the end (item #%" PRIu16 ", %zu bytes of central directory)", i,
cd_length);
#if defined(__ANDROID__)
android_errorWriteLog(0x534e4554, "36392138");
#endif
return -1;
return kInvalidFile;
}
const CentralDirectoryRecord* cdr = reinterpret_cast<const CentralDirectoryRecord*>(ptr);
if (cdr->record_signature != CentralDirectoryRecord::kSignature) {
ALOGW("Zip: missed a central dir sig (at %" PRIu16 ")", i);
return -1;
return kInvalidFile;
}
const off64_t local_header_offset = cdr->local_file_header_offset;
if (local_header_offset >= archive->directory_offset) {
ALOGW("Zip: bad LFH offset %" PRId64 " at entry %" PRIu16,
static_cast<int64_t>(local_header_offset), i);
return -1;
return kInvalidFile;
}
const uint16_t file_name_length = cdr->file_name_length;
@ -394,12 +391,12 @@ static int32_t ParseZipArchive(ZipArchive* archive) {
ALOGW("Zip: file name for entry %" PRIu16
" exceeds the central directory range, file_name_length: %" PRIu16 ", cd_length: %zu",
i, file_name_length, cd_length);
return -1;
return kInvalidEntryName;
}
// Check that file name is valid UTF-8 and doesn't contain NUL (U+0000) characters.
if (!IsValidEntryName(file_name, file_name_length)) {
ALOGW("Zip: invalid file name at entry %" PRIu16, i);
return -1;
return kInvalidEntryName;
}
// Add the CDE filename to the hash table.
@ -414,7 +411,7 @@ static int32_t ParseZipArchive(ZipArchive* archive) {
ptr += sizeof(CentralDirectoryRecord) + file_name_length + extra_length + comment_length;
if ((ptr - cd_ptr) > static_cast<int64_t>(cd_length)) {
ALOGW("Zip: bad CD advance (%tu vs %zu) at entry %" PRIu16, ptr - cd_ptr, cd_length, i);
return -1;
return kInvalidFile;
}
}
@ -422,7 +419,7 @@ static int32_t ParseZipArchive(ZipArchive* archive) {
if (!archive->mapped_zip.ReadAtOffset(reinterpret_cast<uint8_t*>(&lfh_start_bytes),
sizeof(uint32_t), 0)) {
ALOGW("Zip: Unable to read header for entry at offset == 0.");
return -1;
return kInvalidFile;
}
if (lfh_start_bytes != LocalFileHeader::kSignature) {
@ -430,7 +427,7 @@ static int32_t ParseZipArchive(ZipArchive* archive) {
#if defined(__ANDROID__)
android_errorWriteLog(0x534e4554, "64211847");
#endif
return -1;
return kInvalidFile;
}
ALOGV("+++ zip good scan %" PRIu16 " entries", num_entries);
@ -439,16 +436,8 @@ static int32_t ParseZipArchive(ZipArchive* archive) {
}
static int32_t OpenArchiveInternal(ZipArchive* archive, const char* debug_file_name) {
int32_t result = -1;
if ((result = MapCentralDirectory(debug_file_name, archive)) != 0) {
return result;
}
if ((result = ParseZipArchive(archive))) {
return result;
}
return 0;
int32_t result = MapCentralDirectory(debug_file_name, archive);
return result != 0 ? result : ParseZipArchive(archive);
}
int32_t OpenArchiveFd(int fd, const char* debug_file_name, ZipArchiveHandle* handle,
@ -1185,7 +1174,7 @@ off64_t MappedZipFile::GetFileLength() const {
return result;
} else {
if (base_ptr_ == nullptr) {
ALOGE("Zip: invalid file map\n");
ALOGE("Zip: invalid file map");
return -1;
}
return static_cast<off64_t>(data_length_);
@ -1196,12 +1185,12 @@ off64_t MappedZipFile::GetFileLength() const {
bool MappedZipFile::ReadAtOffset(uint8_t* buf, size_t len, off64_t off) const {
if (has_fd_) {
if (!android::base::ReadFullyAtOffset(fd_, buf, len, off)) {
ALOGE("Zip: failed to read at offset %" PRId64 "\n", off);
ALOGE("Zip: failed to read at offset %" PRId64, off);
return false;
}
} else {
if (off < 0 || off > static_cast<off64_t>(data_length_)) {
ALOGE("Zip: invalid offset: %" PRId64 ", data length: %" PRId64 "\n", off, data_length_);
ALOGE("Zip: invalid offset: %" PRId64 ", data length: %" PRId64, off, data_length_);
return false;
}
memcpy(buf, static_cast<const uint8_t*>(base_ptr_) + off, len);
@ -1219,13 +1208,17 @@ bool ZipArchive::InitializeCentralDirectory(off64_t cd_start_offset, size_t cd_s
if (mapped_zip.HasFd()) {
directory_map = android::base::MappedFile::FromFd(mapped_zip.GetFileDescriptor(),
cd_start_offset, cd_size, PROT_READ);
if (!directory_map) return false;
if (!directory_map) {
ALOGE("Zip: failed to map central directory (offset %" PRId64 ", size %zu): %s",
cd_start_offset, cd_size, strerror(errno));
return false;
}
CHECK_EQ(directory_map->size(), cd_size);
central_directory.Initialize(directory_map->data(), 0 /*offset*/, cd_size);
} else {
if (mapped_zip.GetBasePtr() == nullptr) {
ALOGE("Zip: Failed to map central directory, bad mapped_zip base pointer\n");
ALOGE("Zip: Failed to map central directory, bad mapped_zip base pointer");
return false;
}
if (static_cast<off64_t>(cd_start_offset) + static_cast<off64_t>(cd_size) >

6
libziparchive/zip_archive_private.h
View file

@ -42,6 +42,7 @@ static const char* kErrorMessages[] = {
"Invalid entry name",
"I/O error",
"File mapping failed",
"Allocation failed",
};
enum ErrorCodes : int32_t {
@ -87,7 +88,10 @@ enum ErrorCodes : int32_t {
// We were not able to mmap the central directory or entry contents.
kMmapFailed = -12,
kLastErrorCode = kMmapFailed,
// An allocation failed.
kAllocationFailed = -13,
kLastErrorCode = kAllocationFailed,
};
class MappedZipFile {

64
libziparchive/zip_archive_test.cc
View file

@ -36,13 +36,9 @@
static std::string test_data_dir = android::base::GetExecutableDirectory() + "/testdata";
static const std::string kMissingZip = "missing.zip";
static const std::string kValidZip = "valid.zip";
static const std::string kLargeZip = "large.zip";
static const std::string kBadCrcZip = "bad_crc.zip";
static const std::string kCrashApk = "crash.apk";
static const std::string kBadFilenameZip = "bad_filename.zip";
static const std::string kUpdateZip = "dummy-update.zip";
static const std::vector<uint8_t> kATxtContents{'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'a',
'b', 'c', 'd', 'e', 'f', 'g', 'h', '\n'};
@ -52,13 +48,6 @@ static const std::vector<uint8_t> kATxtContentsCompressed{'K', 'L', 'J', 'N', 'I
static const std::vector<uint8_t> kBTxtContents{'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', '\n'};
static const std::string kATxtName("a.txt");
static const std::string kBTxtName("b.txt");
static const std::string kNonexistentTxtName("nonexistent.txt");
static const std::string kEmptyTxtName("empty.txt");
static const std::string kLargeCompressTxtName("compress.txt");
static const std::string kLargeUncompressTxtName("uncompress.txt");
static int32_t OpenArchiveWrapper(const std::string& name, ZipArchiveHandle* handle) {
const std::string abs_path = test_data_dir + "/" + name;
return OpenArchive(abs_path.c_str(), handle);
@ -69,19 +58,31 @@ TEST(ziparchive, Open) {
ASSERT_EQ(0, OpenArchiveWrapper(kValidZip, &handle));
CloseArchive(handle);
ASSERT_EQ(-1, OpenArchiveWrapper(kBadFilenameZip, &handle));
ASSERT_EQ(kInvalidEntryName, OpenArchiveWrapper("bad_filename.zip", &handle));
CloseArchive(handle);
}
TEST(ziparchive, OutOfBound) {
ZipArchiveHandle handle;
ASSERT_EQ(-8, OpenArchiveWrapper(kCrashApk, &handle));
ASSERT_EQ(kInvalidOffset, OpenArchiveWrapper("crash.apk", &handle));
CloseArchive(handle);
}
TEST(ziparchive, EmptyArchive) {
ZipArchiveHandle handle;
ASSERT_EQ(kEmptyArchive, OpenArchiveWrapper("empty.zip", &handle));
CloseArchive(handle);
}
TEST(ziparchive, ZeroSizeCentralDirectory) {
ZipArchiveHandle handle;
ASSERT_EQ(kInvalidFile, OpenArchiveWrapper("zero-size-cd.zip", &handle));
CloseArchive(handle);
}
TEST(ziparchive, OpenMissing) {
ZipArchiveHandle handle;
ASSERT_NE(0, OpenArchiveWrapper(kMissingZip, &handle));
ASSERT_NE(0, OpenArchiveWrapper("missing.zip", &handle));
// Confirm the file descriptor is not going to be mistaken for a valid one.
ASSERT_EQ(-1, GetFileDescriptor(handle));
@ -200,7 +201,7 @@ TEST(ziparchive, FindEntry) {
ASSERT_EQ(0, OpenArchiveWrapper(kValidZip, &handle));
ZipEntry data;
ASSERT_EQ(0, FindEntry(handle, kATxtName, &data));
ASSERT_EQ(0, FindEntry(handle, "a.txt", &data));
// Known facts about a.txt, from zipinfo -v.
ASSERT_EQ(63, data.offset);
@ -211,7 +212,7 @@ TEST(ziparchive, FindEntry) {
ASSERT_EQ(static_cast<uint32_t>(0x438a8005), data.mod_time);
// An entry that doesn't exist. Should be a negative return code.
ASSERT_LT(FindEntry(handle, kNonexistentTxtName, &data), 0);
ASSERT_LT(FindEntry(handle, "this file does not exist", &data), 0);
CloseArchive(handle);
}
@ -259,7 +260,7 @@ TEST(ziparchive, ExtractToMemory) {
// An entry that's deflated.
ZipEntry data;
ASSERT_EQ(0, FindEntry(handle, kATxtName, &data));
ASSERT_EQ(0, FindEntry(handle, "a.txt", &data));
const uint32_t a_size = data.uncompressed_length;
ASSERT_EQ(a_size, kATxtContents.size());
uint8_t* buffer = new uint8_t[a_size];
@ -268,7 +269,7 @@ TEST(ziparchive, ExtractToMemory) {
delete[] buffer;
// An entry that's stored.
ASSERT_EQ(0, FindEntry(handle, kBTxtName, &data));
ASSERT_EQ(0, FindEntry(handle, "b.txt", &data));
const uint32_t b_size = data.uncompressed_length;
ASSERT_EQ(b_size, kBTxtContents.size());
buffer = new uint8_t[b_size];
@ -323,7 +324,7 @@ TEST(ziparchive, EmptyEntries) {
ASSERT_EQ(0, OpenArchiveFd(tmp_file.fd, "EmptyEntriesTest", &handle, false));
ZipEntry entry;
ASSERT_EQ(0, FindEntry(handle, kEmptyTxtName, &entry));
ASSERT_EQ(0, FindEntry(handle, "empty.txt", &entry));
ASSERT_EQ(static_cast<uint32_t>(0), entry.uncompressed_length);
uint8_t buffer[1];
ASSERT_EQ(0, ExtractToMemory(handle, &entry, buffer, 1));
@ -403,7 +404,7 @@ TEST(ziparchive, ExtractToFile) {
ASSERT_EQ(0, OpenArchiveWrapper(kValidZip, &handle));
ZipEntry entry;
ASSERT_EQ(0, FindEntry(handle, kATxtName, &entry));
ASSERT_EQ(0, FindEntry(handle, "a.txt", &entry));
ASSERT_EQ(0, ExtractEntryToFile(handle, &entry, tmp_file.fd));
// Assert that the first 8 bytes of the file haven't been clobbered.
@ -425,7 +426,7 @@ TEST(ziparchive, ExtractToFile) {
#if !defined(_WIN32)
TEST(ziparchive, OpenFromMemory) {
const std::string zip_path = test_data_dir + "/" + kUpdateZip;
const std::string zip_path = test_data_dir + "/dummy-update.zip";
android::base::unique_fd fd(open(zip_path.c_str(), O_RDONLY | O_BINARY));
ASSERT_NE(-1, fd);
struct stat sb;
@ -510,27 +511,27 @@ static void ZipArchiveStreamTestUsingMemory(const std::string& zip_file,
}
TEST(ziparchive, StreamCompressed) {
ZipArchiveStreamTestUsingContents(kValidZip, kATxtName, kATxtContents, false);
ZipArchiveStreamTestUsingContents(kValidZip, "a.txt", kATxtContents, false);
}
TEST(ziparchive, StreamUncompressed) {
ZipArchiveStreamTestUsingContents(kValidZip, kBTxtName, kBTxtContents, false);
ZipArchiveStreamTestUsingContents(kValidZip, "b.txt", kBTxtContents, false);
}
TEST(ziparchive, StreamRawCompressed) {
ZipArchiveStreamTestUsingContents(kValidZip, kATxtName, kATxtContentsCompressed, true);
ZipArchiveStreamTestUsingContents(kValidZip, "a.txt", kATxtContentsCompressed, true);
}
TEST(ziparchive, StreamRawUncompressed) {
ZipArchiveStreamTestUsingContents(kValidZip, kBTxtName, kBTxtContents, true);
ZipArchiveStreamTestUsingContents(kValidZip, "b.txt", kBTxtContents, true);
}
TEST(ziparchive, StreamLargeCompressed) {
ZipArchiveStreamTestUsingMemory(kLargeZip, kLargeCompressTxtName);
ZipArchiveStreamTestUsingMemory(kLargeZip, "compress.txt");
}
TEST(ziparchive, StreamLargeUncompressed) {
ZipArchiveStreamTestUsingMemory(kLargeZip, kLargeUncompressTxtName);
ZipArchiveStreamTestUsingMemory(kLargeZip, "uncompress.txt");
}
TEST(ziparchive, StreamCompressedBadCrc) {
@ -539,7 +540,7 @@ TEST(ziparchive, StreamCompressedBadCrc) {
ZipEntry entry;
std::vector<uint8_t> read_data;
ZipArchiveStreamTest(handle, kATxtName, false, false, &entry, &read_data);
ZipArchiveStreamTest(handle, "a.txt", false, false, &entry, &read_data);
CloseArchive(handle);
}
@ -550,7 +551,7 @@ TEST(ziparchive, StreamUncompressedBadCrc) {
ZipEntry entry;
std::vector<uint8_t> read_data;
ZipArchiveStreamTest(handle, kBTxtName, false, false, &entry, &read_data);
ZipArchiveStreamTest(handle, "b.txt", false, false, &entry, &read_data);
CloseArchive(handle);
}
@ -647,7 +648,8 @@ TEST(ziparchive, ErrorCodeString) {
// Out of bounds.
ASSERT_STREQ("Unknown return code", ErrorCodeString(1));
ASSERT_STREQ("Unknown return code", ErrorCodeString(-13));
ASSERT_STRNE("Unknown return code", ErrorCodeString(kLastErrorCode));
ASSERT_STREQ("Unknown return code", ErrorCodeString(kLastErrorCode - 1));
ASSERT_STREQ("I/O error", ErrorCodeString(kIoError));
}
@ -698,7 +700,7 @@ TEST(ziparchive, BrokenLfhSignature) {
ASSERT_TRUE(android::base::WriteFully(tmp_file.fd, &kZipFileWithBrokenLfhSignature[0],
kZipFileWithBrokenLfhSignature.size()));
ZipArchiveHandle handle;
ASSERT_EQ(-1, OpenArchiveFd(tmp_file.fd, "LeadingNonZipBytes", &handle, false));
ASSERT_EQ(kInvalidFile, OpenArchiveFd(tmp_file.fd, "LeadingNonZipBytes", &handle, false));
}
class VectorReader : public zip_archive::Reader {