From: Tim Kientzle Date: Fri, 10 Apr 2009 05:36:25 +0000 (-0400) Subject: Fix a number of memory leaks: X-Git-Tag: v2.8.0~725 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=71b1c6a2cd6b19739fade9dedcd29480b01b794a;p=thirdparty%2Flibarchive.git Fix a number of memory leaks: * write_set_compression_{gzip,bzip2,lzma,xz} need to call finish even if nothing is ever written so that the config data can be cleaned up. Set the finish entry in the initial setup function (instead of the init function), have the finish function only close out the compression if there is a state, update the tests to exercise set up and tear down without any intermediate open and/or writing. * read_open_{fd,filename} would leak data if the fd or file didn't exist. Do all of the fd/file checks before trying to allocate data. * archive_entry leaked the sourcepath field * support_format_mtree leaked the list of active default fields * several tests leaked archive_entry objects * test/read_open_memory has a special mode to exercise providing a minimal set of callbacks; unfortunately it omitted the close callback, which caused a leak. * Add a chokepoint to test/main.c where we can do something for every assertion. I temporarily added a line # printf here to help identify the location of certain crashes. All of this needs to be merged to libarchive/2.7 branch. SVN-Revision: 944 --- diff --git a/libarchive/archive_entry.c b/libarchive/archive_entry.c index 77c81a137..ffbd92f6e 100644 --- a/libarchive/archive_entry.c +++ b/libarchive/archive_entry.c @@ -370,6 +370,7 @@ archive_entry_clear(struct archive_entry *entry) aes_clean(&entry->ae_gname); aes_clean(&entry->ae_hardlink); aes_clean(&entry->ae_pathname); + aes_clean(&entry->ae_sourcepath); aes_clean(&entry->ae_symlink); aes_clean(&entry->ae_uname); archive_entry_acl_clear(entry); diff --git a/libarchive/archive_read_open_fd.c b/libarchive/archive_read_open_fd.c index 3e348f7b1..c7f9d693b 100644 --- a/libarchive/archive_read_open_fd.c +++ b/libarchive/archive_read_open_fd.c @@ -66,6 +66,11 @@ archive_read_open_fd(struct archive *a, int fd, size_t block_size) struct read_fd_data *mine; void *b; + if (fstat(fd, &st) != 0) { + archive_set_error(a, errno, "Can't stat fd %d", fd); + return (ARCHIVE_FATAL); + } + mine = (struct read_fd_data *)malloc(sizeof(*mine)); b = malloc(block_size); if (mine == NULL || b == NULL) { @@ -77,11 +82,6 @@ archive_read_open_fd(struct archive *a, int fd, size_t block_size) mine->block_size = block_size; mine->buffer = b; mine->fd = fd; - /* lseek() hardly ever works, so disable it by default. See below. */ - if (fstat(mine->fd, &st) != 0) { - archive_set_error(a, errno, "Can't stat fd %d", mine->fd); - return (ARCHIVE_FATAL); - } /* * Skip support is a performance optimization for anything * that supports lseek(). On FreeBSD, only regular files and diff --git a/libarchive/archive_read_open_filename.c b/libarchive/archive_read_open_filename.c index e813a710d..04efb0f5a 100644 --- a/libarchive/archive_read_open_filename.c +++ b/libarchive/archive_read_open_filename.c @@ -82,10 +82,21 @@ archive_read_open_filename(struct archive *a, const char *filename, struct stat st; struct read_file_data *mine; void *b; + int fd; if (filename == NULL || filename[0] == '\0') return (archive_read_open_fd(a, 0, block_size)); + fd = open(filename, O_RDONLY | O_BINARY); + if (fd < 0) { + archive_set_error(a, errno, "Failed to open '%s'", filename); + return (ARCHIVE_FATAL); + } + if (fstat(fd, &st) != 0) { + archive_set_error(a, errno, "Can't stat '%s'", filename); + return (ARCHIVE_FATAL); + } + mine = (struct read_file_data *)malloc(sizeof(*mine) + strlen(filename)); b = malloc(block_size); if (mine == NULL || b == NULL) { @@ -97,17 +108,7 @@ archive_read_open_filename(struct archive *a, const char *filename, strcpy(mine->filename, filename); mine->block_size = block_size; mine->buffer = b; - mine->fd = open(mine->filename, O_RDONLY | O_BINARY); - if (mine->fd < 0) { - archive_set_error(a, errno, "Failed to open '%s'", - mine->filename); - return (ARCHIVE_FATAL); - } - if (fstat(mine->fd, &st) != 0) { - archive_set_error(a, errno, "Can't stat '%s'", - mine->filename); - return (ARCHIVE_FATAL); - } + mine->fd = fd; /* Remember mode so close can decide whether to flush. */ mine->st_mode = st.st_mode; /* If we're reading a file from disk, ensure that we don't diff --git a/libarchive/archive_read_support_format_mtree.c b/libarchive/archive_read_support_format_mtree.c index 447f92268..0f0e116e0 100644 --- a/libarchive/archive_read_support_format_mtree.c +++ b/libarchive/archive_read_support_format_mtree.c @@ -404,10 +404,13 @@ read_mtree(struct archive_read *a, struct mtree *mtree) len = readline(a, mtree, &p, 256); if (len == 0) { mtree->this_entry = mtree->entries; + free_options(global); return (ARCHIVE_OK); } - if (len < 0) + if (len < 0) { + free_options(global); return (len); + } /* Leading whitespace is never significant, ignore it. */ while (*p == ' ' || *p == '\t') { ++p; @@ -432,13 +435,16 @@ read_mtree(struct archive_read *a, struct mtree *mtree) } else break; - if (r != ARCHIVE_OK) + if (r != ARCHIVE_OK) { + free_options(global); return r; + } } archive_set_error(&a->archive, ARCHIVE_ERRNO_FILE_FORMAT, "Can't parse line %ju", counter); - return ARCHIVE_FATAL; + free_options(global); + return (ARCHIVE_FATAL); } /* diff --git a/libarchive/archive_write_disk.c b/libarchive/archive_write_disk.c index 0d65a3b97..54fab828a 100644 --- a/libarchive/archive_write_disk.c +++ b/libarchive/archive_write_disk.c @@ -1278,6 +1278,8 @@ _archive_write_finish(struct archive *_a) (a->cleanup_gid)(a->lookup_gid_data); if (a->cleanup_uid != NULL && a->lookup_uid_data != NULL) (a->cleanup_uid)(a->lookup_uid_data); + if (a->entry) + archive_entry_free(a->entry); archive_string_free(&a->_name_data); archive_string_free(&a->archive.error_string); archive_string_free(&a->path_safe); diff --git a/libarchive/archive_write_set_compression_bzip2.c b/libarchive/archive_write_set_compression_bzip2.c index d6d82352f..17f6a0a4d 100644 --- a/libarchive/archive_write_set_compression_bzip2.c +++ b/libarchive/archive_write_set_compression_bzip2.c @@ -99,6 +99,7 @@ archive_write_set_compression_bzip2(struct archive *_a) return (ARCHIVE_FATAL); } a->compressor.config = config; + a->compressor.finish = archive_compressor_bzip2_finish; config->compression_level = 9; /* default */ a->compressor.init = &archive_compressor_bzip2_init; a->compressor.options = &archive_compressor_bzip2_options; @@ -145,7 +146,6 @@ archive_compressor_bzip2_init(struct archive_write *a) state->stream.next_out = state->compressed; state->stream.avail_out = state->compressed_buffer_size; a->compressor.write = archive_compressor_bzip2_write; - a->compressor.finish = archive_compressor_bzip2_finish; /* Initialize compression library */ ret = BZ2_bzCompressInit(&(state->stream), @@ -255,83 +255,88 @@ archive_compressor_bzip2_finish(struct archive_write *a) ssize_t bytes_written; unsigned tocopy; - state = (struct private_data *)a->compressor.data; ret = ARCHIVE_OK; - if (a->client_writer == NULL) { - archive_set_error(&a->archive, ARCHIVE_ERRNO_PROGRAMMER, - "No write callback is registered?\n" - "This is probably an internal programming error."); - ret = ARCHIVE_FATAL; - goto cleanup; - } + state = (struct private_data *)a->compressor.data; + if (state != NULL) { + if (a->client_writer == NULL) { + archive_set_error(&a->archive, + ARCHIVE_ERRNO_PROGRAMMER, + "No write callback is registered?\n" + "This is probably an internal programming error."); + ret = ARCHIVE_FATAL; + goto cleanup; + } - /* By default, always pad the uncompressed data. */ - if (a->pad_uncompressed) { - tocopy = a->bytes_per_block - - (state->total_in % a->bytes_per_block); - while (tocopy > 0 && tocopy < (unsigned)a->bytes_per_block) { - SET_NEXT_IN(state, a->nulls); - state->stream.avail_in = tocopy < a->null_length ? - tocopy : a->null_length; - state->total_in += state->stream.avail_in; - tocopy -= state->stream.avail_in; - ret = drive_compressor(a, state, 0); - if (ret != ARCHIVE_OK) - goto cleanup; + /* By default, always pad the uncompressed data. */ + if (a->pad_uncompressed) { + tocopy = a->bytes_per_block - + (state->total_in % a->bytes_per_block); + while (tocopy > 0 && tocopy < (unsigned)a->bytes_per_block) { + SET_NEXT_IN(state, a->nulls); + state->stream.avail_in = tocopy < a->null_length ? + tocopy : a->null_length; + state->total_in += state->stream.avail_in; + tocopy -= state->stream.avail_in; + ret = drive_compressor(a, state, 0); + if (ret != ARCHIVE_OK) + goto cleanup; + } } - } - /* Finish compression cycle. */ - if ((ret = drive_compressor(a, state, 1))) - goto cleanup; - - /* Optionally, pad the final compressed block. */ - block_length = state->stream.next_out - state->compressed; - - - /* Tricky calculation to determine size of last block. */ - target_block_length = block_length; - if (a->bytes_in_last_block <= 0) - /* Default or Zero: pad to full block */ - target_block_length = a->bytes_per_block; - else - /* Round length to next multiple of bytes_in_last_block. */ - target_block_length = a->bytes_in_last_block * - ( (block_length + a->bytes_in_last_block - 1) / - a->bytes_in_last_block); - if (target_block_length > a->bytes_per_block) - target_block_length = a->bytes_per_block; - if (block_length < target_block_length) { - memset(state->stream.next_out, 0, - target_block_length - block_length); - block_length = target_block_length; - } + /* Finish compression cycle. */ + if ((ret = drive_compressor(a, state, 1))) + goto cleanup; + + /* Optionally, pad the final compressed block. */ + block_length = state->stream.next_out - state->compressed; + + /* Tricky calculation to determine size of last block. */ + target_block_length = block_length; + if (a->bytes_in_last_block <= 0) + /* Default or Zero: pad to full block */ + target_block_length = a->bytes_per_block; + else + /* Round length to next multiple of bytes_in_last_block. */ + target_block_length = a->bytes_in_last_block * + ( (block_length + a->bytes_in_last_block - 1) / + a->bytes_in_last_block); + if (target_block_length > a->bytes_per_block) + target_block_length = a->bytes_per_block; + if (block_length < target_block_length) { + memset(state->stream.next_out, 0, + target_block_length - block_length); + block_length = target_block_length; + } - /* Write the last block */ - bytes_written = (a->client_writer)(&a->archive, a->client_data, - state->compressed, block_length); + /* Write the last block */ + bytes_written = (a->client_writer)(&a->archive, a->client_data, + state->compressed, block_length); - /* TODO: Handle short write of final block. */ - if (bytes_written <= 0) - ret = ARCHIVE_FATAL; - else { - a->archive.raw_position += ret; - ret = ARCHIVE_OK; - } + /* TODO: Handle short write of final block. */ + if (bytes_written <= 0) + ret = ARCHIVE_FATAL; + else { + a->archive.raw_position += ret; + ret = ARCHIVE_OK; + } - /* Cleanup: shut down compressor, release memory, etc. */ + /* Cleanup: shut down compressor, release memory, etc. */ cleanup: - switch (BZ2_bzCompressEnd(&(state->stream))) { - case BZ_OK: - break; - default: - archive_set_error(&a->archive, ARCHIVE_ERRNO_PROGRAMMER, - "Failed to clean up compressor"); - ret = ARCHIVE_FATAL; - } + switch (BZ2_bzCompressEnd(&(state->stream))) { + case BZ_OK: + break; + default: + archive_set_error(&a->archive, ARCHIVE_ERRNO_PROGRAMMER, + "Failed to clean up compressor"); + ret = ARCHIVE_FATAL; + } - free(state->compressed); - free(state); + free(state->compressed); + free(state); + } + /* Free configuration data even if we were never fully initialized. */ + free(a->compressor.config); + a->compressor.config = NULL; return (ret); } diff --git a/libarchive/archive_write_set_compression_gzip.c b/libarchive/archive_write_set_compression_gzip.c index 35303fc02..a8b1c3336 100644 --- a/libarchive/archive_write_set_compression_gzip.c +++ b/libarchive/archive_write_set_compression_gzip.c @@ -102,6 +102,7 @@ archive_write_set_compression_gzip(struct archive *_a) return (ARCHIVE_FATAL); } a->compressor.config = config; + a->compressor.finish = &archive_compressor_gzip_finish; config->compression_level = Z_DEFAULT_COMPRESSION; a->compressor.init = &archive_compressor_gzip_init; a->compressor.options = &archive_compressor_gzip_options; @@ -186,7 +187,6 @@ archive_compressor_gzip_init(struct archive_write *a) state->stream.avail_out -= 10; a->compressor.write = archive_compressor_gzip_write; - a->compressor.finish = archive_compressor_gzip_finish; /* Initialize compression library. */ ret = deflateInit2(&(state->stream), @@ -281,7 +281,6 @@ archive_compressor_gzip_write(struct archive_write *a, const void *buff, return (ARCHIVE_OK); } - /* * Finish the compression... */ @@ -296,113 +295,118 @@ archive_compressor_gzip_finish(struct archive_write *a) state = (struct private_data *)a->compressor.data; ret = 0; - if (a->client_writer == NULL) { - archive_set_error(&a->archive, ARCHIVE_ERRNO_PROGRAMMER, - "No write callback is registered? " - "This is probably an internal programming error."); - ret = ARCHIVE_FATAL; - goto cleanup; - } + if (state != NULL) { + if (a->client_writer == NULL) { + archive_set_error(&a->archive, + ARCHIVE_ERRNO_PROGRAMMER, + "No write callback is registered? " + "This is probably an internal programming error."); + ret = ARCHIVE_FATAL; + goto cleanup; + } - /* By default, always pad the uncompressed data. */ - if (a->pad_uncompressed) { - tocopy = a->bytes_per_block - - (state->total_in % a->bytes_per_block); - while (tocopy > 0 && tocopy < (unsigned)a->bytes_per_block) { - SET_NEXT_IN(state, a->nulls); - state->stream.avail_in = tocopy < a->null_length ? - tocopy : a->null_length; - state->crc = crc32(state->crc, a->nulls, - state->stream.avail_in); - state->total_in += state->stream.avail_in; - tocopy -= state->stream.avail_in; - ret = drive_compressor(a, state, 0); - if (ret != ARCHIVE_OK) + /* By default, always pad the uncompressed data. */ + if (a->pad_uncompressed) { + tocopy = a->bytes_per_block - + (state->total_in % a->bytes_per_block); + while (tocopy > 0 && tocopy < (unsigned)a->bytes_per_block) { + SET_NEXT_IN(state, a->nulls); + state->stream.avail_in = tocopy < a->null_length ? + tocopy : a->null_length; + state->crc = crc32(state->crc, a->nulls, + state->stream.avail_in); + state->total_in += state->stream.avail_in; + tocopy -= state->stream.avail_in; + ret = drive_compressor(a, state, 0); + if (ret != ARCHIVE_OK) + goto cleanup; + } + } + + /* Finish compression cycle */ + if (((ret = drive_compressor(a, state, 1))) != ARCHIVE_OK) + goto cleanup; + + /* Build trailer: 4-byte CRC and 4-byte length. */ + trailer[0] = (state->crc)&0xff; + trailer[1] = (state->crc >> 8)&0xff; + trailer[2] = (state->crc >> 16)&0xff; + trailer[3] = (state->crc >> 24)&0xff; + trailer[4] = (state->total_in)&0xff; + trailer[5] = (state->total_in >> 8)&0xff; + trailer[6] = (state->total_in >> 16)&0xff; + trailer[7] = (state->total_in >> 24)&0xff; + + /* Add trailer to current block. */ + tocopy = 8; + if (tocopy > state->stream.avail_out) + tocopy = state->stream.avail_out; + memcpy(state->stream.next_out, trailer, tocopy); + state->stream.next_out += tocopy; + state->stream.avail_out -= tocopy; + + /* If it overflowed, flush and start a new block. */ + if (tocopy < 8) { + bytes_written = (a->client_writer)(&a->archive, a->client_data, + state->compressed, state->compressed_buffer_size); + if (bytes_written <= 0) { + ret = ARCHIVE_FATAL; goto cleanup; + } + a->archive.raw_position += bytes_written; + state->stream.next_out = state->compressed; + state->stream.avail_out = state->compressed_buffer_size; + memcpy(state->stream.next_out, trailer + tocopy, 8-tocopy); + state->stream.next_out += 8-tocopy; + state->stream.avail_out -= 8-tocopy; } - } - /* Finish compression cycle */ - if (((ret = drive_compressor(a, state, 1))) != ARCHIVE_OK) - goto cleanup; - - /* Build trailer: 4-byte CRC and 4-byte length. */ - trailer[0] = (state->crc)&0xff; - trailer[1] = (state->crc >> 8)&0xff; - trailer[2] = (state->crc >> 16)&0xff; - trailer[3] = (state->crc >> 24)&0xff; - trailer[4] = (state->total_in)&0xff; - trailer[5] = (state->total_in >> 8)&0xff; - trailer[6] = (state->total_in >> 16)&0xff; - trailer[7] = (state->total_in >> 24)&0xff; - - /* Add trailer to current block. */ - tocopy = 8; - if (tocopy > state->stream.avail_out) - tocopy = state->stream.avail_out; - memcpy(state->stream.next_out, trailer, tocopy); - state->stream.next_out += tocopy; - state->stream.avail_out -= tocopy; - - /* If it overflowed, flush and start a new block. */ - if (tocopy < 8) { + /* Optionally, pad the final compressed block. */ + block_length = state->stream.next_out - state->compressed; + + /* Tricky calculation to determine size of last block. */ + target_block_length = block_length; + if (a->bytes_in_last_block <= 0) + /* Default or Zero: pad to full block */ + target_block_length = a->bytes_per_block; + else + /* Round length to next multiple of bytes_in_last_block. */ + target_block_length = a->bytes_in_last_block * + ( (block_length + a->bytes_in_last_block - 1) / + a->bytes_in_last_block); + if (target_block_length > a->bytes_per_block) + target_block_length = a->bytes_per_block; + if (block_length < target_block_length) { + memset(state->stream.next_out, 0, + target_block_length - block_length); + block_length = target_block_length; + } + + /* Write the last block */ bytes_written = (a->client_writer)(&a->archive, a->client_data, - state->compressed, state->compressed_buffer_size); + state->compressed, block_length); if (bytes_written <= 0) { ret = ARCHIVE_FATAL; goto cleanup; } a->archive.raw_position += bytes_written; - state->stream.next_out = state->compressed; - state->stream.avail_out = state->compressed_buffer_size; - memcpy(state->stream.next_out, trailer + tocopy, 8-tocopy); - state->stream.next_out += 8-tocopy; - state->stream.avail_out -= 8-tocopy; - } - - /* Optionally, pad the final compressed block. */ - block_length = state->stream.next_out - state->compressed; - - - /* Tricky calculation to determine size of last block. */ - target_block_length = block_length; - if (a->bytes_in_last_block <= 0) - /* Default or Zero: pad to full block */ - target_block_length = a->bytes_per_block; - else - /* Round length to next multiple of bytes_in_last_block. */ - target_block_length = a->bytes_in_last_block * - ( (block_length + a->bytes_in_last_block - 1) / - a->bytes_in_last_block); - if (target_block_length > a->bytes_per_block) - target_block_length = a->bytes_per_block; - if (block_length < target_block_length) { - memset(state->stream.next_out, 0, - target_block_length - block_length); - block_length = target_block_length; - } - /* Write the last block */ - bytes_written = (a->client_writer)(&a->archive, a->client_data, - state->compressed, block_length); - if (bytes_written <= 0) { - ret = ARCHIVE_FATAL; - goto cleanup; - } - a->archive.raw_position += bytes_written; - - /* Cleanup: shut down compressor, release memory, etc. */ -cleanup: - switch (deflateEnd(&(state->stream))) { - case Z_OK: - break; - default: - archive_set_error(&a->archive, ARCHIVE_ERRNO_MISC, - "Failed to clean up compressor"); - ret = ARCHIVE_FATAL; + /* Cleanup: shut down compressor, release memory, etc. */ + cleanup: + switch (deflateEnd(&(state->stream))) { + case Z_OK: + break; + default: + archive_set_error(&a->archive, ARCHIVE_ERRNO_MISC, + "Failed to clean up compressor"); + ret = ARCHIVE_FATAL; + } + free(state->compressed); + free(state); } - free(state->compressed); - free(state); + /* Clean up config area even if we never initialized. */ + free(a->compressor.config); + a->compressor.config = NULL; return (ret); } diff --git a/libarchive/archive_write_set_compression_xz.c b/libarchive/archive_write_set_compression_xz.c index 96c0577d5..839625252 100644 --- a/libarchive/archive_write_set_compression_xz.c +++ b/libarchive/archive_write_set_compression_xz.c @@ -104,6 +104,7 @@ archive_write_set_compression_xz(struct archive *_a) return (ARCHIVE_FATAL); } a->compressor.config = config; + a->compressor.finish = archive_compressor_xz_finish; config->compression_level = LZMA_PRESET_DEFAULT; a->compressor.init = &archive_compressor_xz_init; a->compressor.options = &archive_compressor_xz_options; @@ -197,7 +198,6 @@ archive_compressor_xz_init(struct archive_write *a) return (ARCHIVE_FATAL); } a->compressor.write = archive_compressor_xz_write; - a->compressor.finish = archive_compressor_xz_finish; /* Initialize compression library. */ if (lzma_lzma_preset(&state->lzma_opt, config->compression_level)) { @@ -287,71 +287,76 @@ archive_compressor_xz_finish(struct archive_write *a) struct private_data *state; unsigned tocopy; + ret = ARCHIVE_OK; state = (struct private_data *)a->compressor.data; - ret = 0; - if (a->client_writer == NULL) { - archive_set_error(&a->archive, ARCHIVE_ERRNO_PROGRAMMER, - "No write callback is registered? " - "This is probably an internal programming error."); - ret = ARCHIVE_FATAL; - goto cleanup; - } - - /* By default, always pad the uncompressed data. */ - if (a->pad_uncompressed) { - tocopy = a->bytes_per_block - - (state->total_in % a->bytes_per_block); - while (tocopy > 0 && tocopy < (unsigned)a->bytes_per_block) { - state->stream.next_in = a->nulls; - state->stream.avail_in = tocopy < a->null_length ? - tocopy : a->null_length; - state->total_in += state->stream.avail_in; - tocopy -= state->stream.avail_in; - ret = drive_compressor(a, state, 0); - if (ret != ARCHIVE_OK) - goto cleanup; + if (state != NULL) { + if (a->client_writer == NULL) { + archive_set_error(&a->archive, + ARCHIVE_ERRNO_PROGRAMMER, + "No write callback is registered? " + "This is probably an internal programming error."); + ret = ARCHIVE_FATAL; + goto cleanup; } - } - /* Finish compression cycle */ - if (((ret = drive_compressor(a, state, 1))) != ARCHIVE_OK) - goto cleanup; + /* By default, always pad the uncompressed data. */ + if (a->pad_uncompressed) { + tocopy = a->bytes_per_block - + (state->total_in % a->bytes_per_block); + while (tocopy > 0 && tocopy < (unsigned)a->bytes_per_block) { + state->stream.next_in = a->nulls; + state->stream.avail_in = tocopy < a->null_length ? + tocopy : a->null_length; + state->total_in += state->stream.avail_in; + tocopy -= state->stream.avail_in; + ret = drive_compressor(a, state, 0); + if (ret != ARCHIVE_OK) + goto cleanup; + } + } - /* Optionally, pad the final compressed block. */ - block_length = state->stream.next_out - state->compressed; + /* Finish compression cycle */ + if (((ret = drive_compressor(a, state, 1))) != ARCHIVE_OK) + goto cleanup; + + /* Optionally, pad the final compressed block. */ + block_length = state->stream.next_out - state->compressed; + + /* Tricky calculation to determine size of last block. */ + target_block_length = block_length; + if (a->bytes_in_last_block <= 0) + /* Default or Zero: pad to full block */ + target_block_length = a->bytes_per_block; + else + /* Round length to next multiple of bytes_in_last_block. */ + target_block_length = a->bytes_in_last_block * + ( (block_length + a->bytes_in_last_block - 1) / + a->bytes_in_last_block); + if (target_block_length > a->bytes_per_block) + target_block_length = a->bytes_per_block; + if (block_length < target_block_length) { + memset(state->stream.next_out, 0, + target_block_length - block_length); + block_length = target_block_length; + } - /* Tricky calculation to determine size of last block. */ - target_block_length = block_length; - if (a->bytes_in_last_block <= 0) - /* Default or Zero: pad to full block */ - target_block_length = a->bytes_per_block; - else - /* Round length to next multiple of bytes_in_last_block. */ - target_block_length = a->bytes_in_last_block * - ( (block_length + a->bytes_in_last_block - 1) / - a->bytes_in_last_block); - if (target_block_length > a->bytes_per_block) - target_block_length = a->bytes_per_block; - if (block_length < target_block_length) { - memset(state->stream.next_out, 0, - target_block_length - block_length); - block_length = target_block_length; - } + /* Write the last block */ + bytes_written = (a->client_writer)(&a->archive, a->client_data, + state->compressed, block_length); + if (bytes_written <= 0) { + ret = ARCHIVE_FATAL; + goto cleanup; + } + a->archive.raw_position += bytes_written; - /* Write the last block */ - bytes_written = (a->client_writer)(&a->archive, a->client_data, - state->compressed, block_length); - if (bytes_written <= 0) { - ret = ARCHIVE_FATAL; - goto cleanup; + /* Cleanup: shut down compressor, release memory, etc. */ + cleanup: + lzma_end(&(state->stream)); + free(state->compressed); + free(state); } - a->archive.raw_position += bytes_written; - - /* Cleanup: shut down compressor, release memory, etc. */ -cleanup: - lzma_end(&(state->stream)); - free(state->compressed); - free(state); + free(a->compressor.config); + a->compressor.config = NULL; return (ret); } diff --git a/libarchive/test/main.c b/libarchive/test/main.c index 809af5005..830b6a9a1 100644 --- a/libarchive/test/main.c +++ b/libarchive/test/main.c @@ -127,6 +127,20 @@ static struct line { int critical; } failed_lines[1000]; +/* + * Called at the beginning of each assert() function. + */ +static void +count_assertion(const char *file, int line) +{ + (void)file; /* UNUSED */ + (void)line; /* UNUSED */ + ++assertions; + /* Uncomment to print file:line after every assertion. + * Verbose, but occasionally useful in tracking down crashes. */ + /* printf("Checked %s:%d\n", file, line); */ +} + /* * Count this failure; return the number of previous failures. */ @@ -262,7 +276,7 @@ failure(const char *fmt, ...) int test_assert(const char *file, int line, int value, const char *condition, void *extra) { - ++assertions; + count_assertion(file, line); if (value) { msg[0] = '\0'; return (value); @@ -281,7 +295,7 @@ int test_assert_equal_int(const char *file, int line, int v1, const char *e1, int v2, const char *e2, void *extra) { - ++assertions; + count_assertion(file, line); if (v1 == v2) { msg[0] = '\0'; return (1); @@ -328,7 +342,7 @@ test_assert_equal_string(const char *file, int line, const char *v2, const char *e2, void *extra) { - ++assertions; + count_assertion(file, line); if (v1 == NULL || v2 == NULL) { if (v1 == v2) { msg[0] = '\0'; @@ -381,7 +395,7 @@ test_assert_equal_wstring(const char *file, int line, const wchar_t *v2, const char *e2, void *extra) { - ++assertions; + count_assertion(file, line); if (v1 == NULL) { if (v2 == NULL) { msg[0] = '\0'; @@ -456,7 +470,7 @@ test_assert_equal_mem(const char *file, int line, const char *v2, const char *e2, size_t l, const char *ld, void *extra) { - ++assertions; + count_assertion(file, line); if (v1 == NULL || v2 == NULL) { if (v1 == v2) { msg[0] = '\0'; @@ -723,6 +737,16 @@ struct { void (*func)(void); const char *name; } tests[] = { #include "list.h" }; +/* + * This is well-intentioned, but sometimes the standard libraries + * leave open file descriptors and expect to be able to come back to + * them (e.g., for username lookups or logging). Closing these + * descriptors out from under those libraries creates havoc. + * + * Maybe there's some reasonably portable way to tell if a descriptor + * is open without using close()? + */ +#if 0 static void close_descriptors(int warn) { @@ -739,6 +763,7 @@ close_descriptors(int warn) report_failure(NULL); } } +#endif /* * Each test is run in a private work dir. Those work dirs @@ -782,11 +807,12 @@ static int test_run(int i, const char *tmpdir) /* Explicitly reset the locale before each test. */ setlocale(LC_ALL, "C"); /* Make sure there are no stray descriptors going into the test. */ - close_descriptors(0); + /* TODO: Find a better way to identify file descriptor leaks. */ + //close_descriptors(0); /* Run the actual test. */ (*tests[i].func)(); /* Close stray descriptors, record as errors against this test. */ - close_descriptors(1); + //close_descriptors(1); /* Summarize the results of this test. */ summarize(); /* If there were no failures, we can remove the work dir. */ diff --git a/libarchive/test/read_open_memory.c b/libarchive/test/read_open_memory.c index 3f07d08ec..bc633eca1 100644 --- a/libarchive/test/read_open_memory.c +++ b/libarchive/test/read_open_memory.c @@ -96,7 +96,7 @@ read_open_memory_internal(struct archive *a, void *buff, memory_read, memory_read_skip, memory_read_close)); else return (archive_read_open2(a, mine, NULL, - memory_read, NULL, NULL)); + memory_read, NULL, memory_read_close)); } /* diff --git a/libarchive/test/test_extattr_freebsd.c b/libarchive/test/test_extattr_freebsd.c index 7d2abb9aa..19881c2ec 100644 --- a/libarchive/test/test_extattr_freebsd.c +++ b/libarchive/test/test_extattr_freebsd.c @@ -169,5 +169,6 @@ DEFINE_TEST(test_extattr_freebsd) assertEqualMem(xval, "12345", xsize); assertEqualIntA(a, ARCHIVE_OK, archive_read_close(a)); assertEqualInt(ARCHIVE_OK, archive_read_finish(a)); + archive_entry_free(ae); #endif } diff --git a/libarchive/test/test_write_compress_bzip2.c b/libarchive/test/test_write_compress_bzip2.c index 256288f81..54c8a5096 100644 --- a/libarchive/test/test_write_compress_bzip2.c +++ b/libarchive/test/test_write_compress_bzip2.c @@ -194,6 +194,32 @@ DEFINE_TEST(test_write_compress_bzip2) assert(0 == archive_read_close(a)); assert(0 == archive_read_finish(a)); + /* + * Test various premature shutdown scenarios to make sure we + * don't crash or leak memory. + */ + assert((a = archive_write_new()) != NULL); + assertEqualIntA(a, ARCHIVE_OK, archive_write_set_compression_bzip2(a)); + assertEqualInt(ARCHIVE_OK, archive_write_finish(a)); + + assert((a = archive_write_new()) != NULL); + assertEqualIntA(a, ARCHIVE_OK, archive_write_set_compression_bzip2(a)); + assertEqualInt(ARCHIVE_OK, archive_write_close(a)); + assertEqualInt(ARCHIVE_OK, archive_write_finish(a)); + + assert((a = archive_write_new()) != NULL); + assertEqualIntA(a, ARCHIVE_OK, archive_write_set_format_ustar(a)); + assertEqualIntA(a, ARCHIVE_OK, archive_write_set_compression_bzip2(a)); + assertEqualInt(ARCHIVE_OK, archive_write_close(a)); + assertEqualInt(ARCHIVE_OK, archive_write_finish(a)); + + assert((a = archive_write_new()) != NULL); + assertEqualIntA(a, ARCHIVE_OK, archive_write_set_format_ustar(a)); + assertEqualIntA(a, ARCHIVE_OK, archive_write_set_compression_bzip2(a)); + assertA(0 == archive_write_open_memory(a, buff, buffsize, &used2)); + assertEqualInt(ARCHIVE_OK, archive_write_close(a)); + assertEqualInt(ARCHIVE_OK, archive_write_finish(a)); + /* * Clean up. */ diff --git a/libarchive/test/test_write_compress_gzip.c b/libarchive/test/test_write_compress_gzip.c index 4b9c84ca6..9e42b137a 100644 --- a/libarchive/test/test_write_compress_gzip.c +++ b/libarchive/test/test_write_compress_gzip.c @@ -218,6 +218,32 @@ DEFINE_TEST(test_write_compress_gzip) } assertEqualInt(ARCHIVE_OK, archive_read_finish(a)); + /* + * Test various premature shutdown scenarios to make sure we + * don't crash or leak memory. + */ + assert((a = archive_write_new()) != NULL); + assertEqualIntA(a, ARCHIVE_OK, archive_write_set_compression_gzip(a)); + assertEqualInt(ARCHIVE_OK, archive_write_finish(a)); + + assert((a = archive_write_new()) != NULL); + assertEqualIntA(a, ARCHIVE_OK, archive_write_set_compression_gzip(a)); + assertEqualInt(ARCHIVE_OK, archive_write_close(a)); + assertEqualInt(ARCHIVE_OK, archive_write_finish(a)); + + assert((a = archive_write_new()) != NULL); + assertEqualIntA(a, ARCHIVE_OK, archive_write_set_format_ustar(a)); + assertEqualIntA(a, ARCHIVE_OK, archive_write_set_compression_gzip(a)); + assertEqualInt(ARCHIVE_OK, archive_write_close(a)); + assertEqualInt(ARCHIVE_OK, archive_write_finish(a)); + + assert((a = archive_write_new()) != NULL); + assertEqualIntA(a, ARCHIVE_OK, archive_write_set_format_ustar(a)); + assertEqualIntA(a, ARCHIVE_OK, archive_write_set_compression_gzip(a)); + assertA(0 == archive_write_open_memory(a, buff, buffsize, &used2)); + assertEqualInt(ARCHIVE_OK, archive_write_close(a)); + assertEqualInt(ARCHIVE_OK, archive_write_finish(a)); + /* * Clean up. */ diff --git a/libarchive/test/test_write_compress_lzma.c b/libarchive/test/test_write_compress_lzma.c index 4c3169ab2..639e58f6c 100644 --- a/libarchive/test/test_write_compress_lzma.c +++ b/libarchive/test/test_write_compress_lzma.c @@ -211,6 +211,32 @@ DEFINE_TEST(test_write_compress_lzma) } assertEqualInt(ARCHIVE_OK, archive_read_finish(a)); + /* + * Test various premature shutdown scenarios to make sure we + * don't crash or leak memory. + */ + assert((a = archive_write_new()) != NULL); + assertEqualIntA(a, ARCHIVE_OK, archive_write_set_compression_lzma(a)); + assertEqualInt(ARCHIVE_OK, archive_write_finish(a)); + + assert((a = archive_write_new()) != NULL); + assertEqualIntA(a, ARCHIVE_OK, archive_write_set_compression_lzma(a)); + assertEqualInt(ARCHIVE_OK, archive_write_close(a)); + assertEqualInt(ARCHIVE_OK, archive_write_finish(a)); + + assert((a = archive_write_new()) != NULL); + assertEqualIntA(a, ARCHIVE_OK, archive_write_set_format_ustar(a)); + assertEqualIntA(a, ARCHIVE_OK, archive_write_set_compression_lzma(a)); + assertEqualInt(ARCHIVE_OK, archive_write_close(a)); + assertEqualInt(ARCHIVE_OK, archive_write_finish(a)); + + assert((a = archive_write_new()) != NULL); + assertEqualIntA(a, ARCHIVE_OK, archive_write_set_format_ustar(a)); + assertEqualIntA(a, ARCHIVE_OK, archive_write_set_compression_lzma(a)); + assertA(0 == archive_write_open_memory(a, buff, buffsize, &used2)); + assertEqualInt(ARCHIVE_OK, archive_write_close(a)); + assertEqualInt(ARCHIVE_OK, archive_write_finish(a)); + /* * Clean up. */ diff --git a/libarchive/test/test_write_compress_xz.c b/libarchive/test/test_write_compress_xz.c index 60effec28..988fb6e87 100644 --- a/libarchive/test/test_write_compress_xz.c +++ b/libarchive/test/test_write_compress_xz.c @@ -219,6 +219,32 @@ DEFINE_TEST(test_write_compress_xz) } assertEqualInt(ARCHIVE_OK, archive_read_finish(a)); + /* + * Test various premature shutdown scenarios to make sure we + * don't crash or leak memory. + */ + assert((a = archive_write_new()) != NULL); + assertEqualIntA(a, ARCHIVE_OK, archive_write_set_compression_xz(a)); + assertEqualInt(ARCHIVE_OK, archive_write_finish(a)); + + assert((a = archive_write_new()) != NULL); + assertEqualIntA(a, ARCHIVE_OK, archive_write_set_compression_xz(a)); + assertEqualInt(ARCHIVE_OK, archive_write_close(a)); + assertEqualInt(ARCHIVE_OK, archive_write_finish(a)); + + assert((a = archive_write_new()) != NULL); + assertEqualIntA(a, ARCHIVE_OK, archive_write_set_format_ustar(a)); + assertEqualIntA(a, ARCHIVE_OK, archive_write_set_compression_xz(a)); + assertEqualInt(ARCHIVE_OK, archive_write_close(a)); + assertEqualInt(ARCHIVE_OK, archive_write_finish(a)); + + assert((a = archive_write_new()) != NULL); + assertEqualIntA(a, ARCHIVE_OK, archive_write_set_format_ustar(a)); + assertEqualIntA(a, ARCHIVE_OK, archive_write_set_compression_xz(a)); + assertA(0 == archive_write_open_memory(a, buff, buffsize, &used2)); + assertEqualInt(ARCHIVE_OK, archive_write_close(a)); + assertEqualInt(ARCHIVE_OK, archive_write_finish(a)); + /* * Clean up. */ diff --git a/libarchive/test/test_write_disk_sparse.c b/libarchive/test/test_write_disk_sparse.c index 617961085..c9c00d3d1 100644 --- a/libarchive/test/test_write_disk_sparse.c +++ b/libarchive/test/test_write_disk_sparse.c @@ -116,6 +116,7 @@ verify_write_data(struct archive *a, int sparse) /* XXX more XXX */ assertEqualInt(0, close(fd)); + archive_entry_free(ae); free(buff); } @@ -249,6 +250,7 @@ verify_write_data_block(struct archive *a, int sparse) assertEqualInt(0, close(fd)); free(buff); + archive_entry_free(ae); } DEFINE_TEST(test_write_disk_sparse)