From 917db7558d81493381f111a949885735036bfe29 Mon Sep 17 00:00:00 2001 From: Yuriy Grigoryev Date: Tue, 25 Aug 2026 09:40:40 +0700 Subject: [PATCH v2] Fix checksum error handling in pg_combinebackup pg_checksum_init() frees the SHA context if initialization fails, but leaves a dangling pointer in the checksum context. Clear the pointer after freeing it. Likewise, make pg_checksum_final() release and clear the SHA context whether finalization succeeds or fails. Keep the shared SHA cleanup outside the switch to avoid duplicating it for every SHA variant. Check initialization and finalization failures in pg_combinebackup, matching the handling in basebackup and pg_verifybackup. Add a frontend test program which injects failures through a mock cryptohash provider. Test every SHA variant and verify that both error paths and successful finalization release and clear the context. Co-authored-by: Kanatbek Kanybekov --- src/bin/pg_combinebackup/Makefile | 16 +- src/bin/pg_combinebackup/backup_label.c | 9 +- src/bin/pg_combinebackup/meson.build | 15 ++ src/bin/pg_combinebackup/pg_combinebackup.c | 11 +- src/bin/pg_combinebackup/reconstruct.c | 7 +- .../pg_combinebackup/t/013_checksum_helper.pl | 13 ++ .../pg_combinebackup/test_checksum_helper.c | 144 ++++++++++++++++++ src/bin/pg_combinebackup/write_manifest.c | 7 +- src/common/checksum_helper.c | 31 ++-- 9 files changed, 224 insertions(+), 29 deletions(-) create mode 100644 src/bin/pg_combinebackup/t/013_checksum_helper.pl create mode 100644 src/bin/pg_combinebackup/test_checksum_helper.c diff --git a/src/bin/pg_combinebackup/Makefile b/src/bin/pg_combinebackup/Makefile index 7a6c44d..c33f854 100644 --- a/src/bin/pg_combinebackup/Makefile +++ b/src/bin/pg_combinebackup/Makefile @@ -30,11 +30,21 @@ OBJS = \ reconstruct.o \ write_manifest.o +TEST_OBJS = \ + checksum_helper_test.o \ + test_checksum_helper.o + all: pg_combinebackup pg_combinebackup: $(OBJS) | submake-libpgport submake-libpgfeutils $(CC) $(CFLAGS) $^ $(LDFLAGS) $(LDFLAGS_EX) $(LIBS) -o $@$(X) +test_checksum_helper$(X): $(TEST_OBJS) | submake-libpgport submake-libpgfeutils + $(CC) $(CFLAGS) $^ $(LDFLAGS) $(LDFLAGS_EX) $(LIBS) -o $@$(X) + +checksum_helper_test.o: $(top_srcdir)/src/common/checksum_helper.c + $(CC) $(CFLAGS) $(CPPFLAGS) -DFRONTEND -c $< -o $@ + install: all installdirs $(INSTALL_PROGRAM) pg_combinebackup$(X) '$(DESTDIR)$(bindir)/pg_combinebackup$(X)' @@ -45,11 +55,11 @@ uninstall: rm -f '$(DESTDIR)$(bindir)/pg_combinebackup$(X)' clean distclean maintainer-clean: - rm -f pg_combinebackup$(X) $(OBJS) + rm -f pg_combinebackup$(X) test_checksum_helper$(X) $(OBJS) $(TEST_OBJS) rm -rf tmp_check -check: +check: test_checksum_helper$(X) $(prove_check) -installcheck: +installcheck: test_checksum_helper$(X) $(prove_installcheck) diff --git a/src/bin/pg_combinebackup/backup_label.c b/src/bin/pg_combinebackup/backup_label.c index dc4210f..6d8d431 100644 --- a/src/bin/pg_combinebackup/backup_label.c +++ b/src/bin/pg_combinebackup/backup_label.c @@ -134,10 +134,12 @@ write_backup_label(char *output_directory, StringInfo buf, uint8 checksum_payload[PG_CHECKSUM_MAX_LENGTH]; int checksum_length; - pg_checksum_init(&checksum_ctx, checksum_type); - snprintf(output_filename, MAXPGPATH, "%s/backup_label", output_directory); + if (pg_checksum_init(&checksum_ctx, checksum_type) < 0) + pg_fatal("could not initialize checksum of file \"%s\"", + output_filename); + if ((output_fd = open(output_filename, O_WRONLY | O_CREAT | O_EXCL | PG_BINARY, pg_file_create_mode)) < 0) @@ -176,6 +178,9 @@ write_backup_label(char *output_directory, StringInfo buf, pg_fatal("could not close file \"%s\": %m", output_filename); checksum_length = pg_checksum_final(&checksum_ctx, checksum_payload); + if (checksum_length < 0) + pg_fatal("could not finalize checksum of file \"%s\"", + output_filename); if (mwriter != NULL) { diff --git a/src/bin/pg_combinebackup/meson.build b/src/bin/pg_combinebackup/meson.build index ba1c8cf..f3069b3 100644 --- a/src/bin/pg_combinebackup/meson.build +++ b/src/bin/pg_combinebackup/meson.build @@ -22,6 +22,19 @@ pg_combinebackup = executable('pg_combinebackup', ) bin_targets += pg_combinebackup +test_checksum_helper = executable('test_checksum_helper', + files( + 'test_checksum_helper.c', + '../../common/checksum_helper.c', + ), + dependencies: [frontend_code], + kwargs: default_bin_args + { + 'c_args': ['-DFRONTEND'], + 'install': false, + }, +) +testprep_targets += test_checksum_helper + tests += { 'name': 'pg_combinebackup', 'sd': meson.current_source_dir(), @@ -40,7 +53,9 @@ tests += { 't/010_hardlink.pl', 't/011_ib_truncation.pl', 't/012_vm_consistency.pl', + 't/013_checksum_helper.pl', ], + 'deps': [test_checksum_helper], } } diff --git a/src/bin/pg_combinebackup/pg_combinebackup.c b/src/bin/pg_combinebackup/pg_combinebackup.c index 86e2ee3..42669af 100644 --- a/src/bin/pg_combinebackup/pg_combinebackup.c +++ b/src/bin/pg_combinebackup/pg_combinebackup.c @@ -1067,6 +1067,8 @@ process_directory_recursively(Oid tsoid, snprintf(manifest_path, MAXPGPATH, "%s%s", manifest_prefix, de->d_name); + snprintf(ofullpath, MAXPGPATH, "%s/%s", ofulldir, de->d_name); + /* * It's not an incremental file, so we need to copy the entire * file to the output directory. @@ -1110,11 +1112,11 @@ process_directory_recursively(Oid tsoid, */ if (checksum_length != 0) pg_checksum_init(&checksum_ctx, CHECKSUM_TYPE_NONE); - else - pg_checksum_init(&checksum_ctx, checksum_type); + else if (pg_checksum_init(&checksum_ctx, checksum_type) < 0) + pg_fatal("could not initialize checksum of file \"%s\"", + ofullpath); /* Actually copy the file. */ - snprintf(ofullpath, MAXPGPATH, "%s/%s", ofulldir, de->d_name); copy_file(ifullpath, ofullpath, &checksum_ctx, opt->copy_method, opt->dry_run); @@ -1128,6 +1130,9 @@ process_directory_recursively(Oid tsoid, checksum_payload = pg_malloc(PG_CHECKSUM_MAX_LENGTH); checksum_length = pg_checksum_final(&checksum_ctx, checksum_payload); + if (checksum_length < 0) + pg_fatal("could not finalize checksum of file \"%s\"", + ofullpath); } } diff --git a/src/bin/pg_combinebackup/reconstruct.c b/src/bin/pg_combinebackup/reconstruct.c index e4b0274..6605853 100644 --- a/src/bin/pg_combinebackup/reconstruct.c +++ b/src/bin/pg_combinebackup/reconstruct.c @@ -317,7 +317,9 @@ reconstruct_from_incremental_file(char *input_filename, } /* Prepare for checksum calculation, if required. */ - pg_checksum_init(&checksum_ctx, checksum_type); + if (pg_checksum_init(&checksum_ctx, checksum_type) < 0) + pg_fatal("could not initialize checksum of file \"%s\"", + output_filename); /* * If the full file can be created by copying a file from an older backup @@ -353,6 +355,9 @@ reconstruct_from_incremental_file(char *input_filename, *checksum_payload = pg_malloc(PG_CHECKSUM_MAX_LENGTH); *checksum_length = pg_checksum_final(&checksum_ctx, *checksum_payload); + if (*checksum_length < 0) + pg_fatal("could not finalize checksum of file \"%s\"", + output_filename); } /* diff --git a/src/bin/pg_combinebackup/t/013_checksum_helper.pl b/src/bin/pg_combinebackup/t/013_checksum_helper.pl new file mode 100644 index 0000000..86f428f --- /dev/null +++ b/src/bin/pg_combinebackup/t/013_checksum_helper.pl @@ -0,0 +1,13 @@ +# Copyright (c) 2026, PostgreSQL Global Development Group + +use strict; +use warnings FATAL => 'all'; +use PostgreSQL::Test::Utils; +use Test::More; + +command_exit_is( + ['test_checksum_helper'], + 0, + 'checksum contexts are cleaned up after cryptohash failures'); + +done_testing(); diff --git a/src/bin/pg_combinebackup/test_checksum_helper.c b/src/bin/pg_combinebackup/test_checksum_helper.c new file mode 100644 index 0000000..8262f71 --- /dev/null +++ b/src/bin/pg_combinebackup/test_checksum_helper.c @@ -0,0 +1,144 @@ +/*------------------------------------------------------------------------- + * + * test_checksum_helper.c + * Test checksum helper error handling using a mock cryptohash provider + * + * Portions Copyright (c) 1996-2026, PostgreSQL Global Development Group + * + * IDENTIFICATION + * src/bin/pg_combinebackup/test_checksum_helper.c + * + *------------------------------------------------------------------------- + */ + +#include "postgres_fe.h" + +#include "common/checksum_helper.h" + +struct pg_cryptohash_ctx +{ + pg_cryptohash_type type; +}; + +static bool fail_init; +static bool fail_final; +static int free_count; + +pg_cryptohash_ctx * +pg_cryptohash_create(pg_cryptohash_type type) +{ + pg_cryptohash_ctx *ctx = malloc(sizeof(*ctx)); + + if (ctx != NULL) + ctx->type = type; + return ctx; +} + +int +pg_cryptohash_init(pg_cryptohash_ctx *ctx) +{ + (void) ctx; + return fail_init ? -1 : 0; +} + +int +pg_cryptohash_update(pg_cryptohash_ctx *ctx, const uint8 *data, size_t len) +{ + (void) ctx; + (void) data; + (void) len; + return 0; +} + +int +pg_cryptohash_final(pg_cryptohash_ctx *ctx, uint8 *dest, size_t len) +{ + (void) ctx; + if (fail_final) + return -1; + memset(dest, 0, len); + return 0; +} + +void +pg_cryptohash_free(pg_cryptohash_ctx *ctx) +{ + free_count++; + free(ctx); +} + +const char * +pg_cryptohash_error(pg_cryptohash_ctx *ctx) +{ + (void) ctx; + return "injected failure"; +} + +static bool +check(bool condition, pg_checksum_type type, const char *message) +{ + if (condition) + return true; + + fprintf(stderr, "checksum type %d: %s\n", type, message); + return false; +} + +int +main(void) +{ + const struct + { + pg_checksum_type type; + int digest_length; + } sha_types[] = { + {CHECKSUM_TYPE_SHA224, PG_SHA224_DIGEST_LENGTH}, + {CHECKSUM_TYPE_SHA256, PG_SHA256_DIGEST_LENGTH}, + {CHECKSUM_TYPE_SHA384, PG_SHA384_DIGEST_LENGTH}, + {CHECKSUM_TYPE_SHA512, PG_SHA512_DIGEST_LENGTH} + }; + uint8 output[PG_CHECKSUM_MAX_LENGTH]; + bool success = true; + + for (size_t i = 0; i < lengthof(sha_types); i++) + { + pg_checksum_context context; + pg_checksum_type type = sha_types[i].type; + + fail_init = true; + fail_final = false; + free_count = 0; + success &= check(pg_checksum_init(&context, type) == -1, + type, "init failure was not returned"); + success &= check(context.raw_context.c_sha2 == NULL, + type, "init failure did not clear context"); + success &= check(free_count == 1, + type, "init failure did not free context once"); + + fail_init = false; + fail_final = true; + free_count = 0; + success &= check(pg_checksum_init(&context, type) == 0, + type, "initialization failed unexpectedly"); + success &= check(pg_checksum_final(&context, output) == -1, + type, "final failure was not returned"); + success &= check(context.raw_context.c_sha2 == NULL, + type, "final failure did not clear context"); + success &= check(free_count == 1, + type, "final failure did not free context once"); + + fail_final = false; + free_count = 0; + success &= check(pg_checksum_init(&context, type) == 0, + type, "initialization failed unexpectedly"); + success &= check(pg_checksum_final(&context, output) == + sha_types[i].digest_length, + type, "finalization returned the wrong digest length"); + success &= check(context.raw_context.c_sha2 == NULL, + type, "successful finalization did not clear context"); + success &= check(free_count == 1, + type, "successful finalization did not free context once"); + } + + return success ? 0 : 1; +} diff --git a/src/bin/pg_combinebackup/write_manifest.c b/src/bin/pg_combinebackup/write_manifest.c index c2ab728..7b34490 100644 --- a/src/bin/pg_combinebackup/write_manifest.c +++ b/src/bin/pg_combinebackup/write_manifest.c @@ -54,7 +54,9 @@ create_manifest_writer(char *directory, uint64 system_identifier) initStringInfo(&mwriter->buf); mwriter->first_file = true; mwriter->still_checksumming = true; - pg_checksum_init(&mwriter->manifest_ctx, CHECKSUM_TYPE_SHA256); + if (pg_checksum_init(&mwriter->manifest_ctx, CHECKSUM_TYPE_SHA256) < 0) + pg_fatal("could not initialize checksum of file \"%s\"", + mwriter->pathname); appendStringInfo(&mwriter->buf, "{ \"PostgreSQL-Backup-Manifest-Version\": 2,\n" @@ -174,6 +176,9 @@ finalize_manifest(manifest_writer *mwriter, appendStringInfoString(&mwriter->buf, "\"Manifest-Checksum\": \""); enlargeStringInfo(&mwriter->buf, 2 * PG_SHA256_DIGEST_STRING_LENGTH); len = pg_checksum_final(&mwriter->manifest_ctx, checksumbuf); + if (len < 0) + pg_fatal("could not finalize checksum of file \"%s\"", + mwriter->pathname); Assert(len == PG_SHA256_DIGEST_LENGTH); mwriter->buf.len += hex_encode(checksumbuf, len, &mwriter->buf.data[mwriter->buf.len]); diff --git a/src/common/checksum_helper.c b/src/common/checksum_helper.c index 2c6e2fc..57d80c5 100644 --- a/src/common/checksum_helper.c +++ b/src/common/checksum_helper.c @@ -99,6 +99,7 @@ pg_checksum_init(pg_checksum_context *context, pg_checksum_type type) if (pg_cryptohash_init(context->raw_context.c_sha2) < 0) { pg_cryptohash_free(context->raw_context.c_sha2); + context->raw_context.c_sha2 = NULL; return -1; } break; @@ -109,6 +110,7 @@ pg_checksum_init(pg_checksum_context *context, pg_checksum_type type) if (pg_cryptohash_init(context->raw_context.c_sha2) < 0) { pg_cryptohash_free(context->raw_context.c_sha2); + context->raw_context.c_sha2 = NULL; return -1; } break; @@ -119,6 +121,7 @@ pg_checksum_init(pg_checksum_context *context, pg_checksum_type type) if (pg_cryptohash_init(context->raw_context.c_sha2) < 0) { pg_cryptohash_free(context->raw_context.c_sha2); + context->raw_context.c_sha2 = NULL; return -1; } break; @@ -129,6 +132,7 @@ pg_checksum_init(pg_checksum_context *context, pg_checksum_type type) if (pg_cryptohash_init(context->raw_context.c_sha2) < 0) { pg_cryptohash_free(context->raw_context.c_sha2); + context->raw_context.c_sha2 = NULL; return -1; } break; @@ -191,42 +195,31 @@ pg_checksum_final(pg_checksum_context *context, uint8 *output) switch (context->type) { case CHECKSUM_TYPE_NONE: - break; + return 0; case CHECKSUM_TYPE_CRC32C: FIN_CRC32C(context->raw_context.c_crc32c); retval = sizeof(pg_crc32c); memcpy(output, &context->raw_context.c_crc32c, retval); - break; + return retval; case CHECKSUM_TYPE_SHA224: retval = PG_SHA224_DIGEST_LENGTH; - if (pg_cryptohash_final(context->raw_context.c_sha2, - output, retval) < 0) - return -1; - pg_cryptohash_free(context->raw_context.c_sha2); break; case CHECKSUM_TYPE_SHA256: retval = PG_SHA256_DIGEST_LENGTH; - if (pg_cryptohash_final(context->raw_context.c_sha2, - output, retval) < 0) - return -1; - pg_cryptohash_free(context->raw_context.c_sha2); break; case CHECKSUM_TYPE_SHA384: retval = PG_SHA384_DIGEST_LENGTH; - if (pg_cryptohash_final(context->raw_context.c_sha2, - output, retval) < 0) - return -1; - pg_cryptohash_free(context->raw_context.c_sha2); break; case CHECKSUM_TYPE_SHA512: retval = PG_SHA512_DIGEST_LENGTH; - if (pg_cryptohash_final(context->raw_context.c_sha2, - output, retval) < 0) - return -1; - pg_cryptohash_free(context->raw_context.c_sha2); break; } - Assert(retval <= PG_CHECKSUM_MAX_LENGTH); + if (pg_cryptohash_final(context->raw_context.c_sha2, + output, retval) < 0) + retval = -1; + pg_cryptohash_free(context->raw_context.c_sha2); + context->raw_context.c_sha2 = NULL; + return retval; } -- 2.50.1 (Apple Git-155)