Re: [PATCH] Fix use-after-free after failed pg_checksum_init

Hi Daniel,

Thanks for taking a look!

I reworked pg_checksum_final() along the lines you suggested. The NONE and
CRC32C cases now return directly from the switch, while the SHA cases only set
the digest length. The common final/free/clear logic is after the switch, and I
removed the now-redundant Assert. The patch also keeps the error checks at all
fallible pg_checksum_init() and pg_checksum_final() calls in pg_combinebackup.

For the test, I first looked at using the existing injection-point facility.
Sadly it is implemented for backend processes, and pg_combinebackup is
a frontend program. checksum_helper.c is compiled separately with FRONTEND, so
I discovered that this facility cannot make pg_cryptohash_init() or pg_cryptohash_final()
return -1 in the code path we need to test. The existing "error" callback would not
model this either, because it raises ERROR instead of returning -1.

Instead, my v2 follows the test harness pattern used by src/test/modules/test_escape:
it is a small test-only frontend C executable, not installed, and is invoked by a TAP test.
The difference is that our executable links the real checksum_helper.c against
test implementations of the pg_cryptohash_* API instead of the production
cryptohash provider. The linker therefore substitutes a deterministic mock at
the API boundary while the code under test remains the real checksum_helper.c.

The mock can make pg_cryptohash_init() or pg_cryptohash_final() return -1 and
counts calls to pg_cryptohash_free(). For SHA224, SHA256, SHA384, and SHA512,
the test verifies the returned error, that the context is freed exactly once,
and that c_sha2 is cleared. It also checks the corresponding invariants after
successful finalization.

As a negative control, I built the same test executable twice, changing only
checksum_helper.c. With the original file it reports, among other failures:
```
checksum type 2: init failure did not clear context
checksum type 2: final failure did not free context once
checksum type 3: final failure did not clear context
checksum type 4: successful finalization did not clear context
checksum type 5: final failure did not free context once
```
Here types 2 through 5 are SHA224, SHA256, SHA384, and SHA512.
The TAP result is:
```
not ok 1 - checksum contexts are cleaned up after cryptohash failures
Result: FAIL
```

With the patched checksum_helper.c, the same TAP test produces:
```
ok 1 - checksum contexts are cleaned up after cryptohash failures
All tests successful.
Result: PASS
```

On back-patching, I agree that the risk/reward is less obvious. I left that
decision separate for now. My v2 is against master.

Best regards,
Yuriy Grigoryev
________________________________________
От: Daniel Gustafsson <daniel(at)yesql(dot)se>
Отправлено: 24 августа 2026 г. 20:12:11
Кому: Григорьев Юрий
Копия: pgsql-hackers(at)lists(dot)postgresql(dot)org; Каныбеков Канатбек
Тема: Re: [PATCH] Fix use-after-free after failed pg_checksum_init

> On 17 Aug 2026, at 08:40, Grigorev Jurij <ju(dot)grigorev(at)ftdata(dot)ru> wrote:
>
> Hi there, hackers!
>
> pg_checksum_init() is documented to return -1 on failure. For the SHA types it first stores the requested type in the checksum context, then calls pg_cryptohash_create() / pg_cryptohash_init(). If create succeeds and init fails, it frees the SHA context but does not clear context->raw_context.c_sha2.

Thanks for the report!

> basebackup and pg_verifybackup check that return value. pg_combinebackup does not: write_backup_label(), create_manifest_writer(), reconstruct, and the copy path in pg_combinebackup.c all ignore a failed pg_checksum_init() and later call pg_checksum_update() or pg_checksum_final() with the dangling pointer.

Agreed, it seems quite reasonable to fail here like basebackup does.

> create_manifest_writer() always uses CHECKSUM_TYPE_SHA256 for the manifest itself, so this is not limited to --manifest-checksums=sha256.
> The use-after-free is reachable in a frontend OpenSSL build when EVP_DigestInit_ex() fails after a successful create.

Reading the OpenSSL code, it's not a particularly likely - albeit possible -
codepath.

> The in-core cryptohash implementation does not fail after create, so a non-OpenSSL build does not hit this path.

Such builds are also an extreme minority in production.

> A related issue in pg_checksum_final(): if pg_cryptohash_final() fails, the function returns -1 without freeing the SHA context, and there is no separate cleanup API.

case CHECKSUM_TYPE_SHA512:
retval = PG_SHA512_DIGEST_LENGTH;
if (pg_cryptohash_final(context->raw_context.c_sha2,
output, retval) < 0)
- return -1;
+ retval = -1;
pg_cryptohash_free(context->raw_context.c_sha2);
+ context->raw_context.c_sha2 = NULL;
+ if (retval < 0)
+ return -1;

I'm not sure there is much value in this much repetition in the switch. How
about returning retval from CHECKSUM_TYPE_CRC32 and _TYPE_NONE, and for the
_SHAXXX cases breaking out the logic to a single place at the end in a
fallthrough? Something like the below untested sketch (which only looks at the
last case as an example)?

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;
return retval;

Repeating code is a good way of increasing the risk of missing to update all
occurrences when a bug is found. The assertion redundant with the
StaticAssertDecl calls added to the top of the function and can be removed
anyways IMO.

>
> The attached patch is against master. It NULLs the pointer after free on init failure, always frees the SHA context in pg_checksum_final(), and checks pg_checksum_init() / pg_checksum_final() in pg_combinebackup.

> I also think it should be back-patched to 17, where pg_combinebackup was added.

Possibly. The risk/reward ratio for an error which is very unlikely to happen
isn't favorable as backpatching always introduce some level of destabilization
risk. You might be right that the value in catching this rare error, should it
happen, is worth it.

> We found this with Clang Static Analyzer. No new regression test is included, injecting a post-create cryptohash init failure would need a
> mock that we do not have today.

Have a look at injection point tests, they are very handy for testing this
class of almost impossible to hit error paths.

--
Daniel Gustafsson