diff --git a/libs/utils/ObbFile.cpp b/libs/utils/ObbFile.cpp index 3a4a03a77..fe49300eb 100644 --- a/libs/utils/ObbFile.cpp +++ b/libs/utils/ObbFile.cpp @@ -91,22 +91,24 @@ bool ObbFile::readFrom(const char* filename) fd = ::open(filename, O_RDONLY); if (fd < 0) { + LOGW("couldn't open file %s: %s", filename, strerror(errno)); goto out; } success = readFrom(fd); close(fd); -out: if (!success) { - LOGW("failed to read from %s\n", filename); + LOGW("failed to read from %s (fd=%d)\n", filename, fd); } + +out: return success; } bool ObbFile::readFrom(int fd) { if (fd < 0) { - LOGW("failed to read file\n"); + LOGW("attempt to read from invalid fd\n"); return false; } @@ -149,10 +151,16 @@ bool ObbFile::parseObbFile(int fd) footerSize = get4LE((unsigned char*)footer); if (footerSize > (size_t)fileLength - kFooterTagSize || footerSize > kMaxBufSize) { - LOGW("claimed footer size is too large (0x%08lx; file size is 0x%08llx)\n", + LOGW("claimed footer size is too large (0x%08zx; file size is 0x%08llx)\n", footerSize, fileLength); return false; } + + if (footerSize < kFooterMinSize) { + LOGW("claimed footer size is too small (%08zx; minimum size is 0x%x)\n", + footerSize, kFooterMinSize); + return false; + } } my_off64_t fileOffset = fileLength - footerSize - kFooterTagSize; @@ -161,26 +169,22 @@ bool ObbFile::parseObbFile(int fd) return false; } - size_t readAmount = kMaxBufSize; - if (readAmount > footerSize) - readAmount = footerSize; - - char* scanBuf = (char*)malloc(readAmount); + char* scanBuf = (char*)malloc(footerSize); if (scanBuf == NULL) { LOGW("couldn't allocate scanBuf: %s\n", strerror(errno)); return false; } - actual = TEMP_FAILURE_RETRY(read(fd, scanBuf, readAmount)); + actual = TEMP_FAILURE_RETRY(read(fd, scanBuf, footerSize)); // readAmount is guaranteed to be less than kMaxBufSize - if (actual != (ssize_t)readAmount) { + if (actual != (ssize_t)footerSize) { LOGI("couldn't read ObbFile footer: %s\n", strerror(errno)); free(scanBuf); return false; } #ifdef DEBUG - for (int i = 0; i < readAmount; ++i) { + for (int i = 0; i < footerSize; ++i) { LOGI("char: 0x%02x", scanBuf[i]); } #endif @@ -197,7 +201,8 @@ bool ObbFile::parseObbFile(int fd) uint32_t packageNameLen = get4LE((unsigned char*)scanBuf + kPackageNameLenOffset); if (packageNameLen <= 0 || packageNameLen > (footerSize - kPackageNameOffset)) { - LOGW("bad ObbFile package name length (0x%08x)\n", packageNameLen); + LOGW("bad ObbFile package name length (0x%04x; 0x%04x possible)\n", + packageNameLen, footerSize - kPackageNameOffset); free(scanBuf); return false; } @@ -206,6 +211,11 @@ bool ObbFile::parseObbFile(int fd) mPackageName = String8(const_cast(packageName), packageNameLen); free(scanBuf); + +#ifdef DEBUG + LOGI("Obb scan succeeded: packageName=%s, version=%d\n", mPackageName.string(), mVersion); +#endif + return true; } @@ -234,6 +244,8 @@ bool ObbFile::writeTo(int fd) return false; } + my_lseek64(fd, 0, SEEK_END); + if (mPackageName.size() == 0 || mVersion == -1) { LOGW("tried to write uninitialized ObbFile data"); return false; diff --git a/libs/utils/tests/ObbFile_test.cpp b/libs/utils/tests/ObbFile_test.cpp index 05aaf0870..29bb70a66 100644 --- a/libs/utils/tests/ObbFile_test.cpp +++ b/libs/utils/tests/ObbFile_test.cpp @@ -22,6 +22,8 @@ #include +#include + namespace android { #define TEST_FILENAME "/test.obb" @@ -39,6 +41,11 @@ protected: const int totalLen = strlen(mExternalStorage) + strlen(TEST_FILENAME) + 1; mFileName = new char[totalLen]; snprintf(mFileName, totalLen, "%s%s", mExternalStorage, TEST_FILENAME); + + int fd = ::open(mFileName, O_CREAT | O_TRUNC); + if (fd < 0) { + FAIL() << "Couldn't create " << mFileName << " for tests"; + } } virtual void TearDown() { @@ -46,8 +53,8 @@ protected: }; TEST_F(ObbFileTest, ReadFailure) { - EXPECT_FALSE(mObbFile->readFrom(-1)) - << "No failure on invalid file descriptor"; + EXPECT_FALSE(mObbFile->readFrom(-1)) + << "No failure on invalid file descriptor"; } TEST_F(ObbFileTest, WriteThenRead) { @@ -66,10 +73,10 @@ TEST_F(ObbFileTest, WriteThenRead) { << "couldn't read from fake .obb file"; EXPECT_EQ(versionNum, mObbFile->getVersion()) - << "version didn't come out the same as it went in"; + << "version didn't come out the same as it went in"; const char* currentPackageName = mObbFile->getPackageName().string(); EXPECT_STREQ(packageName, currentPackageName) - << "package name didn't come out the same as it went in"; + << "package name didn't come out the same as it went in"; } }