Re: [PATCH v1] Reject zero resource kinds in test_resowner_many() - Mailing list pgsql-hackers

Hi hackers,

While running Clang Static Analyzer through CodeChecker over PostgreSQL
18.6, we found a possible division by zero in
RememberManyTestResources():

    kind_idx = (kind_idx + 1) % nkinds;

The SQL entry point rejects negative values of nkinds but accepts zero.
If there are resources to remember, the helper then accesses kinds[0]
even though the array contains no resource kinds, and can eventually
reach the modulo operation.

For example:

    CREATE EXTENSION test_resowner;
    SELECT test_resowner_many(0, 1, 0, 0, 0);

On an assertion-enabled master build on macOS, I get:

    NOTICE:  remembering 1 before-locks resources
    TRAP: failed Assert("kind->release_phase != 0"),
          File: "../src/backend/utils/resowner/resowner.c", Line: 536
    LOG:  client backend was terminated by signal 6: Abort trap: 6

ResourceOwnerRemember() detects the invalid descriptor passed as
&kinds[0].desc, so this build fails before reaching the division.

The after-locks path has the same problem:

    SELECT test_resowner_many(0, 0, 0, 1, 0);

Interestingly, in the same build this failed the neighboring assertion:

    TRAP: failed Assert("kind->release_priority != 0"),
          File: "../src/backend/utils/resowner/resowner.c", Line: 537

The two calls fail at different checks depending on the contents of the
invalid kinds[0] entry.  Without assertions, the code has already
invoked undefined behavior before reaching the modulo, so the exact
failure mode is not predictable.

ForgetManyTestResources() contains another "% nkinds", but that
expression is inside a loop bounded by nkinds.  With nkinds equal to
zero, the loop body is never entered, so only the remember path needs
fixing.

The patch rejects nkinds <= 0 at the SQL entry point, matching
test_resowner_priorities() in the same module.  It also adds an
assertion documenting the helper's precondition and regression coverage
for both remember paths.

This deliberately changes the behavior of the all-zero invocation:

    SELECT test_resowner_many(0, 0, 0, 0, 0);

It currently succeeds as a no-op, but with the patch it returns:

    ERROR:  nkinds must be greater than zero

I think rejecting it is preferable because zero resource kinds do not
have a useful meaning for this test.  Would it be better to preserve
the all-zero no-op case and reject nkinds = 0 only when resources are
requested?

The issue was introduced with the test_resowner module in commit
b8bff07daa85, so PostgreSQL 17, PostgreSQL 18, and master are affected.
It is confined to the test module; production ResourceOwner code is not
affected.

The patch applies cleanly to current master.  I reproduced both failures
without the patch and verified that the test_resowner regression test
passes with it applied.

I would also appreciate opinions on whether this test-only fix is worth
back-patching to PostgreSQL 17 and 18.  I can prepare separate
back-branch patches if needed.

I will add the patch to the CommitFest.

Best regards,
  Yuriy Grigoryev

Hi Yuriy,

I reviewed v1 on master with assertions enabled. The test_resowner
regression test passes. Both zero-kind remember cases abort without the
patch and return the expected SQL error with it. I found no blocking issues with this test-code fix.

Kind regards,