Skip to content

Fix error reporting and resource cleanup across sources and constructors - #591

Open
filtede98 wants to merge 1 commit into
nih-at:mainfrom
filtede98:fix-source-error-handling-and-cleanup
Open

filtede98 wants to merge 1 commit into
nih-at:mainfrom
filtede98:fix-source-error-handling-and-cleanup

Conversation

@filtede98

Copy link
Copy Markdown
Contributor

This patch fixes several error reporting and resource cleanup defects across source constructors and crypto wrappers:

  1. zip_source_file_common.c (zip_source_file_common_new):

    • When a file does not exist (!sb.exists) and cannot be opened for writing (e.g. read-only, non-zero offset, or length specified), zip_error_set(&ctx->stat_error, ZIP_ER_READ, ENOENT) was called on the internal ctx->stat_error, and ctx was immediately freed. The caller-provided error pointer remained unpopulated, leaving callers of zip_source_file_create with NULL and an uninitialized error reason. This now correctly sets zip_error_set(error, ZIP_ER_READ, ENOENT).
    • On all error unwinding paths (!ops->stat, !sb.exists, length overflow, and zip_source_function_create failure), zip_error_fini is now invoked on &ctx->stat_error and &ctx->error before freeing ctx.
    • In read_file under ZIP_SOURCE_FREE, zip_error_fini is now called on &ctx->stat_error and &ctx->error before freeing ctx.
  2. zip_source_free.c (zip_source_free):

    • When destroying a source, zip_error_fini(&src->error) is now called before freeing src. src->error is initialized in _zip_source_new(), and any dynamically allocated error string from operations on the source was previously leaked upon freeing.
  3. zip_source_buffer.c (buffer_new, _zip_source_buffer_new, read_data):

    • In buffer_new, if allocating buffer fails, zip_error_set(error, ZIP_ER_MEMORY, 0) is now called, consistent with the other allocation checks in the function.
    • On zip_source_function_create failure in _zip_source_buffer_new and under ZIP_SOURCE_FREE in read_data, zip_error_fini(&ctx->error) is now called before freeing ctx.
  4. zip_string.c (_zip_string_new):

    • If allocating s->raw fails, zip_error_set(error, ZIP_ER_MEMORY, 0) is now called before freeing s and returning NULL (matching the s = malloc(...) check).
  5. zip_winzip_aes.c (_zip_winzip_aes_new):

    • If _zip_crypto_pbkdf2 fails, sensitive key material in buffer and ctx is now scrubbed with _zip_crypto_clear(), ctx is freed, and zip_error_set(error, ZIP_ER_INTERNAL, 0) is reported, preventing sensitive data leakage and unpopulated caller errors.
  6. zip_source_pkware_decode.c (trad_pkware_free):

    • zip_error_fini(&ctx->error) is now called before freeing ctx, matching zip_source_pkware_encode.c.
  7. zip_source_window.c (_zip_source_window_new, window_read):

    • zip_error_fini(&ctx->error) is now invoked on error unwinding and under ZIP_SOURCE_FREE.
  8. zip_source_crc.c (zip_source_crc_create):

    • If zip_source_layered_create fails, crc_context_free(ctx) is now called instead of raw free(ctx), ensuring ctx->error is finalized.

Several source creation and cleanup routines have missing error reporting to callers or fail to finalize internal zip_error_t structures before freeing their enclosing contexts:

- In zip_source_file_common_new, when a file does not exist and write mode is not applicable, report ZIP_ER_READ (ENOENT) into the caller-provided error pointer instead of setting the internal ctx->stat_error which was immediately freed without informing the caller.
- Call zip_error_fini on ctx->stat_error and ctx->error in all error unwinding branches of zip_source_file_common_new as well as during read_file ZIP_SOURCE_FREE.
- Call zip_error_fini(&src->error) in zip_source_free before free(src).
- In buffer_new, set ZIP_ER_MEMORY when malloc for buffer fails, matching other allocation checks in the function.
- In _zip_source_buffer_new and read_data ZIP_SOURCE_FREE, finalize ctx->error before freeing ctx.
- In _zip_string_new, set ZIP_ER_MEMORY if allocating s->raw fails.
- In _zip_winzip_aes_new, clear sensitive key material in buffer and ctx with _zip_crypto_clear and report ZIP_ER_INTERNAL if _zip_crypto_pbkdf2 fails.
- In trad_pkware_free (zip_source_pkware_decode.c), finalize ctx->error before free(ctx), consistent with zip_source_pkware_encode.c.
- In _zip_source_window_new and window_read ZIP_SOURCE_FREE, finalize ctx->error before freeing ctx.
- In zip_source_crc_create, use crc_context_free instead of raw free(ctx) upon layered source creation failure.
@DeNdlerX

Copy link
Copy Markdown

I ran into the zip_source_free() leak independently while running the test suite under ASan on Linux, so I can confirm that part of this PR.

On current main, read_incons and decrypt-wrong-password-pkware-2 fail with a LeakSanitizer report. ziptool cat prints zip_error_strerror(zip_source_error(src)) and then calls zip_source_free(src), and since the source's error is never finalized, the string is never freed.

cmake -B build -DCMAKE_C_FLAGS="-fsanitize=address -g"
cmake --build build
ctest --test-dir build -R 'read_incons|decrypt-wrong-password-pkware-2'

Both pass with this PR. Valgrind shows the same leak on a normal build.

I also ran the full test suite with this PR under ASan. The only remaining failures are four tests using tryopen, which has a separate leak in the test program itself; I'll open a PR for that.

(clang 21, Ubuntu)

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug For issues, use type instead.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants