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

Merge "Fix SEGV in libziparchive with malformed zip file."

Browse source
This commit is contained in:
Elliott Hughes 2019-12-18 16:14:54 +00:00 committed by Gerrit Code Review
commit 264a37d12f
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 {