Re: Reject empty rounds values in pgcrypto's SHA-crypt salts

Hi Chao,

Thanks for picking this up as a separate patch, as you
offered in your review of my sha-crypt rounds fix. I
have read v1, reproduced the problem, and tested the
fix; my reading agrees with yours and I think the patch
is ready.

The mechanism, as I understand it: px_crypt_shacrypt()
parses the rounds= token of a $5$/$6$ salt with
strtoint() and only checks that parsing stopped at '$'.
For an empty token strtoint() returns 0 and leaves endp
pointing at the token itself, and since that character
is '$' the check passes -- so "rounds=" is treated as
rounds=0 and silently clamped to 1000. Your fix adds
the missing no-conversion check, endp == num, to the
same condition, so the empty value now gets the existing
"could not parse salt options" error. That is the right
layer: it extends the malformed-option check that is
already there, in the one parsing site both salt types
share, and it is consistent with how the neighbouring
malformed inputs (rounds=abc, a lone sign, trailing
junk) already error out today.

Reproduction on unpatched master (f1d69bb, which is a
few commits past the patch's base; v1 applies cleanly
there):

SELECT crypt('password', '$5$rounds=$saltsalt$');
NOTICE: rounds=0 is below supported value (1000),
using 1000 instead
$5$rounds=1000$saltsalt$azOwbpkvuuBKkE82dQPwTsQE8Jy
T9Fflpr9aKid3aT9

The result is identical to an explicit rounds=1000, so
the empty value is silently accepted today. (On plain
master the NOTICE says rounds=0; the odd-looking
"rounds=" form in your report comes from my patch
printing the raw token.) With v1 applied, the same
statement, and the $6$ form, fail with:

ERROR: could not parse salt options

I also ran a probe battery comparing master and the
patched build on the neighbouring cases: salts with no
rounds= and with explicit rounds=5000 (both $5$ and
$6$), rounds=0 / 10 / -5 clamping to 1000, an
overflowing rounds value clamping to the maximum,
rounds=abc, a lone sign, trailing junk, and the
whitespace and leading-'+' forms the parser admits.
The two outputs differ only in the empty-rounds cases;
everything else, including all the existing clamping
behaviour, is byte-identical.

Scope check: I grepped the tree for other rounds=
handling. The strtoint() call in crypt-sha.c is the
only place a SHA-crypt salt's rounds option is parsed
from a string. _crypt_gensalt_sha() only formats a
rounds value it is given as an integer, px_gen_salt()
range-checks that integer, and the strstr() for
"rounds=" later in crypt-sha.c rejects the option
embedded in the salt body rather than parsing it. So
the fix is complete as it stands.

Tests: the patch adds an empty-rounds case to both the
$5$ and the $6$ sections of crypt-shacrypt, which
covers both entry points of the shared code path. The
pgcrypto regression suite passes 25/25 with the patch
(it also passes 25/25 on unpatched master, verified
on a full-schedule run), and with
the patched test files run against unpatched code the
suite fails 24/25, with crypt-shacrypt differing in
exactly the two new cases -- so the new tests do pin
the fix. I also ran make installcheck-world against
an installed patched build: it completed with no
failures, 658 tests across 54 suites, including core
regression 239/239 and isolation 134/134. That build
had TAP tests disabled, so the TAP suites did not
run, and the pg_stat_statements and test_decoding
installcheck targets were no-ops; everything else
pg_regress-based did run. CFBot is green on v1
as well.

One interaction worth flagging, since both patches are
in flight: this patch and my #7507 v5 both touch the
same few lines of crypt-sha.c (#7507 inserts lines
between the strtoint() call and the check you modify).
Whichever lands second will not apply as a bare patch
file -- the hunk context no longer matches -- but a
three-way merge resolves it cleanly in both orders, I
verified the combined pgcrypto tree is the same either
way, and there is no semantic overlap: your check
errors out before any of #7507's NOTICE code can see
the empty token. So it is only a context rebase for
whoever goes second, not a logic change. How far to
backpatch I will leave to the committer; the change
does turn input that is silently accepted today into
an error.

For the hands-on part I used AI assistance: the builds,
the probe battery, and the test-suite runs were done
with AI assistance, and I reviewed the results and the
patch itself as described above.

Verdict: I am marking this Ready for Committer.

Regards,
Prashant Chinnam