From 7bad7c4646ee8fd8d6e6ed0ffd3ddbb0c1b41a2f Mon Sep 17 00:00:00 2001 From: Elliott Hughes Date: Tue, 28 Apr 2015 17:24:24 -0700 Subject: Check all lseek calls succeed. Also add missing TEMP_FAILURE_RETRYs on read, write, and lseek. Bug: http://b/20625546 Change-Id: I03b198e11c1921b35518ee2dd005a7cfcf4fd94b --- minzip/SysUtil.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) (limited to 'minzip/SysUtil.c') diff --git a/minzip/SysUtil.c b/minzip/SysUtil.c index ac6f5c33f..b160c9e3d 100644 --- a/minzip/SysUtil.c +++ b/minzip/SysUtil.c @@ -27,11 +27,13 @@ static int getFileStartAndLength(int fd, off_t *start_, size_t *length_) assert(start_ != NULL); assert(length_ != NULL); - start = lseek(fd, 0L, SEEK_CUR); - end = lseek(fd, 0L, SEEK_END); - (void) lseek(fd, start, SEEK_SET); + // TODO: isn't start always 0 for the single call site? just use fstat instead? - if (start == (off_t) -1 || end == (off_t) -1) { + start = TEMP_FAILURE_RETRY(lseek(fd, 0L, SEEK_CUR)); + end = TEMP_FAILURE_RETRY(lseek(fd, 0L, SEEK_END)); + + if (TEMP_FAILURE_RETRY(lseek(fd, start, SEEK_SET)) == -1 || + start == (off_t) -1 || end == (off_t) -1) { LOGE("could not determine length of file\n"); return -1; } -- cgit v1.2.3 From f267dee1cadba106eee373f7b1732bd4be9ebe13 Mon Sep 17 00:00:00 2001 From: Elliott Hughes Date: Tue, 23 Jun 2015 12:31:02 -0700 Subject: Just use fstat in sysMapFile. Also turn on -Werror and remove a dead function. Change-Id: I436f0a91c40e36db985190b3b98b0a4527cf0eeb --- minzip/SysUtil.c | 86 +++++++++++++++----------------------------------------- 1 file changed, 23 insertions(+), 63 deletions(-) (limited to 'minzip/SysUtil.c') diff --git a/minzip/SysUtil.c b/minzip/SysUtil.c index b160c9e3d..3dd3572fe 100644 --- a/minzip/SysUtil.c +++ b/minzip/SysUtil.c @@ -3,86 +3,46 @@ * * System utilities. */ -#include +#include +#include +#include +#include +#include #include -#include +#include #include #include -#include #include -#include -#include -#include -#include +#include +#include #define LOG_TAG "sysutil" #include "Log.h" #include "SysUtil.h" -static int getFileStartAndLength(int fd, off_t *start_, size_t *length_) -{ - off_t start, end; - size_t length; - - assert(start_ != NULL); - assert(length_ != NULL); - - // TODO: isn't start always 0 for the single call site? just use fstat instead? - - start = TEMP_FAILURE_RETRY(lseek(fd, 0L, SEEK_CUR)); - end = TEMP_FAILURE_RETRY(lseek(fd, 0L, SEEK_END)); - - if (TEMP_FAILURE_RETRY(lseek(fd, start, SEEK_SET)) == -1 || - start == (off_t) -1 || end == (off_t) -1) { - LOGE("could not determine length of file\n"); - return -1; - } - - length = end - start; - if (length == 0) { - LOGE("file is empty\n"); - return -1; - } - - *start_ = start; - *length_ = length; - - return 0; -} - -/* - * Map a file (from fd's current offset) into a private, read-only memory - * segment. The file offset must be a multiple of the page size. - * - * On success, returns 0 and fills out "pMap". On failure, returns a nonzero - * value and does not disturb "pMap". - */ -static int sysMapFD(int fd, MemMapping* pMap) -{ - off_t start; - size_t length; - void* memPtr; - +static bool sysMapFD(int fd, MemMapping* pMap) { assert(pMap != NULL); - if (getFileStartAndLength(fd, &start, &length) < 0) - return -1; + struct stat sb; + if (fstat(fd, &sb) == -1) { + LOGW("fstat(%d) failed: %s\n", fd, strerror(errno)); + return false; + } - memPtr = mmap(NULL, length, PROT_READ, MAP_PRIVATE, fd, start); + void* memPtr = mmap(NULL, sb.st_size, PROT_READ, MAP_PRIVATE, fd, 0); if (memPtr == MAP_FAILED) { - LOGW("mmap(%d, R, PRIVATE, %d, %d) failed: %s\n", (int) length, - fd, (int) start, strerror(errno)); - return -1; + LOGW("mmap(%d, R, PRIVATE, %d, 0) failed: %s\n", (int) sb.st_size, fd, strerror(errno)); + return false; } pMap->addr = memPtr; - pMap->length = length; + pMap->length = sb.st_size; pMap->range_count = 1; pMap->ranges = malloc(sizeof(MappedRange)); pMap->ranges[0].addr = memPtr; - pMap->ranges[0].length = length; + pMap->ranges[0].length = sb.st_size; - return 0; + return true; } static int sysMapBlockFile(FILE* mapf, MemMapping* pMap) @@ -180,13 +140,13 @@ int sysMapFile(const char* fn, MemMapping* pMap) fclose(mapf); } else { // This is a regular file. - int fd = open(fn, O_RDONLY, 0); - if (fd < 0) { + int fd = open(fn, O_RDONLY); + if (fd == -1) { LOGE("Unable to open '%s': %s\n", fn, strerror(errno)); return -1; } - if (sysMapFD(fd, pMap) != 0) { + if (!sysMapFD(fd, pMap)) { LOGE("Map of '%s' failed\n", fn); close(fd); return -1; -- cgit v1.2.3 From d7d0f7503456c3d275a49f90be35e03f02c51bbd Mon Sep 17 00:00:00 2001 From: Tao Bao Date: Wed, 15 Jul 2015 14:13:06 -0700 Subject: Clean up LOG functions. For fatal errors, use LOGE to show messages. Bug: 22236461 Change-Id: Ie2ce7ec769f4502d732fbb53fb7b303c0cf9ed68 --- minzip/SysUtil.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) (limited to 'minzip/SysUtil.c') diff --git a/minzip/SysUtil.c b/minzip/SysUtil.c index 3dd3572fe..09ec8768f 100644 --- a/minzip/SysUtil.c +++ b/minzip/SysUtil.c @@ -25,13 +25,13 @@ static bool sysMapFD(int fd, MemMapping* pMap) { struct stat sb; if (fstat(fd, &sb) == -1) { - LOGW("fstat(%d) failed: %s\n", fd, strerror(errno)); + LOGE("fstat(%d) failed: %s\n", fd, strerror(errno)); return false; } void* memPtr = mmap(NULL, sb.st_size, PROT_READ, MAP_PRIVATE, fd, 0); if (memPtr == MAP_FAILED) { - LOGW("mmap(%d, R, PRIVATE, %d, 0) failed: %s\n", (int) sb.st_size, fd, strerror(errno)); + LOGE("mmap(%d, R, PRIVATE, %d, 0) failed: %s\n", (int) sb.st_size, fd, strerror(errno)); return false; } @@ -55,7 +55,7 @@ static int sysMapBlockFile(FILE* mapf, MemMapping* pMap) unsigned int i; if (fgets(block_dev, sizeof(block_dev), mapf) == NULL) { - LOGW("failed to read block device from header\n"); + LOGE("failed to read block device from header\n"); return -1; } for (i = 0; i < sizeof(block_dev); ++i) { @@ -66,7 +66,7 @@ static int sysMapBlockFile(FILE* mapf, MemMapping* pMap) } if (fscanf(mapf, "%zu %u\n%u\n", &size, &blksize, &range_count) != 3) { - LOGW("failed to parse block map header\n"); + LOGE("failed to parse block map header\n"); return -1; } @@ -80,7 +80,7 @@ static int sysMapBlockFile(FILE* mapf, MemMapping* pMap) unsigned char* reserve; reserve = mmap64(NULL, blocks * blksize, PROT_NONE, MAP_PRIVATE | MAP_ANON, -1, 0); if (reserve == MAP_FAILED) { - LOGW("failed to reserve address space: %s\n", strerror(errno)); + LOGE("failed to reserve address space: %s\n", strerror(errno)); return -1; } @@ -89,7 +89,7 @@ static int sysMapBlockFile(FILE* mapf, MemMapping* pMap) int fd = open(block_dev, O_RDONLY); if (fd < 0) { - LOGW("failed to open block device %s: %s\n", block_dev, strerror(errno)); + LOGE("failed to open block device %s: %s\n", block_dev, strerror(errno)); return -1; } @@ -97,13 +97,13 @@ static int sysMapBlockFile(FILE* mapf, MemMapping* pMap) for (i = 0; i < range_count; ++i) { int start, end; if (fscanf(mapf, "%d %d\n", &start, &end) != 2) { - LOGW("failed to parse range %d in block map\n", i); + LOGE("failed to parse range %d in block map\n", i); return -1; } void* addr = mmap64(next, (end-start)*blksize, PROT_READ, MAP_PRIVATE | MAP_FIXED, fd, ((off64_t)start)*blksize); if (addr == MAP_FAILED) { - LOGW("failed to map block %d: %s\n", i, strerror(errno)); + LOGE("failed to map block %d: %s\n", i, strerror(errno)); return -1; } pMap->ranges[i].addr = addr; @@ -128,12 +128,12 @@ int sysMapFile(const char* fn, MemMapping* pMap) // A map of blocks FILE* mapf = fopen(fn+1, "r"); if (mapf == NULL) { - LOGV("Unable to open '%s': %s\n", fn+1, strerror(errno)); + LOGE("Unable to open '%s': %s\n", fn+1, strerror(errno)); return -1; } if (sysMapBlockFile(mapf, pMap) != 0) { - LOGW("Map of '%s' failed\n", fn); + LOGE("Map of '%s' failed\n", fn); return -1; } @@ -165,7 +165,7 @@ void sysReleaseMap(MemMapping* pMap) int i; for (i = 0; i < pMap->range_count; ++i) { if (munmap(pMap->ranges[i].addr, pMap->ranges[i].length) < 0) { - LOGW("munmap(%p, %d) failed: %s\n", + LOGE("munmap(%p, %d) failed: %s\n", pMap->ranges[i].addr, (int)pMap->ranges[i].length, strerror(errno)); } } -- cgit v1.2.3 From 4f2df162c6ab4a71ca86e4b38735b681729c353b Mon Sep 17 00:00:00 2001 From: Yabin Cui Date: Thu, 18 Feb 2016 11:32:10 -0800 Subject: Fix integer overflows in recovery procedure. Bug: 26960931 Change-Id: Ieae45caccfb4728fcf514f0d920976585d8e6caf (cherry picked from commit a029c9a45888141a2fa382e0b1868e55db1f36d2) --- minzip/SysUtil.c | 69 ++++++++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 54 insertions(+), 15 deletions(-) (limited to 'minzip/SysUtil.c') diff --git a/minzip/SysUtil.c b/minzip/SysUtil.c index b160c9e3d..b1fb4556d 100644 --- a/minzip/SysUtil.c +++ b/minzip/SysUtil.c @@ -3,6 +3,8 @@ * * System utilities. */ +#include +#include #include #include #include @@ -79,6 +81,11 @@ static int sysMapFD(int fd, MemMapping* pMap) pMap->length = length; pMap->range_count = 1; pMap->ranges = malloc(sizeof(MappedRange)); + if (pMap->ranges == NULL) { + LOGE("malloc failed: %s\n", strerror(errno)); + munmap(memPtr, length); + return -1; + } pMap->ranges[0].addr = memPtr; pMap->ranges[0].length = length; @@ -90,7 +97,7 @@ static int sysMapBlockFile(FILE* mapf, MemMapping* pMap) char block_dev[PATH_MAX+1]; size_t size; unsigned int blksize; - unsigned int blocks; + size_t blocks; unsigned int range_count; unsigned int i; @@ -109,49 +116,80 @@ static int sysMapBlockFile(FILE* mapf, MemMapping* pMap) LOGW("failed to parse block map header\n"); return -1; } - - blocks = ((size-1) / blksize) + 1; + if (blksize != 0) { + blocks = ((size-1) / blksize) + 1; + } + if (size == 0 || blksize == 0 || blocks > SIZE_MAX / blksize || range_count == 0) { + LOGE("invalid data in block map file: size %zu, blksize %u, range_count %u\n", + size, blksize, range_count); + return -1; + } pMap->range_count = range_count; - pMap->ranges = malloc(range_count * sizeof(MappedRange)); - memset(pMap->ranges, 0, range_count * sizeof(MappedRange)); + pMap->ranges = calloc(range_count, sizeof(MappedRange)); + if (pMap->ranges == NULL) { + LOGE("calloc(%u, %zu) failed: %s\n", range_count, sizeof(MappedRange), strerror(errno)); + return -1; + } // Reserve enough contiguous address space for the whole file. unsigned char* reserve; reserve = mmap64(NULL, blocks * blksize, PROT_NONE, MAP_PRIVATE | MAP_ANON, -1, 0); if (reserve == MAP_FAILED) { LOGW("failed to reserve address space: %s\n", strerror(errno)); + free(pMap->ranges); return -1; } - pMap->ranges[range_count-1].addr = reserve; - pMap->ranges[range_count-1].length = blocks * blksize; - int fd = open(block_dev, O_RDONLY); if (fd < 0) { LOGW("failed to open block device %s: %s\n", block_dev, strerror(errno)); + munmap(reserve, blocks * blksize); + free(pMap->ranges); return -1; } unsigned char* next = reserve; + size_t remaining_size = blocks * blksize; + bool success = true; for (i = 0; i < range_count; ++i) { - int start, end; - if (fscanf(mapf, "%d %d\n", &start, &end) != 2) { + size_t start, end; + if (fscanf(mapf, "%zu %zu\n", &start, &end) != 2) { LOGW("failed to parse range %d in block map\n", i); - return -1; + success = false; + break; + } + size_t length = (end - start) * blksize; + if (end <= start || (end - start) > SIZE_MAX / blksize || length > remaining_size) { + LOGE("unexpected range in block map: %zu %zu\n", start, end); + success = false; + break; } - void* addr = mmap64(next, (end-start)*blksize, PROT_READ, MAP_PRIVATE | MAP_FIXED, fd, ((off64_t)start)*blksize); + void* addr = mmap64(next, length, PROT_READ, MAP_PRIVATE | MAP_FIXED, fd, ((off64_t)start)*blksize); if (addr == MAP_FAILED) { LOGW("failed to map block %d: %s\n", i, strerror(errno)); - return -1; + success = false; + break; } pMap->ranges[i].addr = addr; - pMap->ranges[i].length = (end-start)*blksize; + pMap->ranges[i].length = length; - next += pMap->ranges[i].length; + next += length; + remaining_size -= length; + } + if (success && remaining_size != 0) { + LOGE("ranges in block map are invalid: remaining_size = %zu\n", remaining_size); + success = false; + } + if (!success) { + close(fd); + munmap(reserve, blocks * blksize); + free(pMap->ranges); + return -1; } + close(fd); pMap->addr = reserve; pMap->length = size; @@ -174,6 +212,7 @@ int sysMapFile(const char* fn, MemMapping* pMap) if (sysMapBlockFile(mapf, pMap) != 0) { LOGW("Map of '%s' failed\n", fn); + fclose(mapf); return -1; } -- cgit v1.2.3