Re: [PATCH] pgcrypto: Fix sha-crypt rounds= values that do not fit in int (BUG #19743)

Hi Chao,

Thanks for reviewing and testing v4. Replies inline.

>I think 32 characters is unnecessarily long for a NOTICE
>message. Since PX_SHACRYPT_ROUNDS_MAX (999999999) is
>only 9 digits long, I think a limit of 10 characters
>should be sufficient.
>
>#define PX_SHACRYPT_ROUNDS_PRINT_MAX \
> ((int) sizeof(CppAsString2(PX_SHACRYPT_ROUNDS_MAX)))

Adopted exactly as you suggested, in the attached v5.
The define evaluates to 10, and I verified the boundary
behavior: a 10-character token still prints in full with
no ellipsis (for example rounds=4294968296, and INT_MAX
2147483647), while an 11-character token is truncated;
the INT_MIN token prints as "rounds=-214748364...". The
overlong-token case now shows:
NOTICE: rounds=0000000000... is below supported
value (1000), using 1000 instead

>This is a pre-existing issue, not a problem introduced
>by your patch. The code only checks whether endp
>points to $, so it doesn't reject an empty rounds
>value.
>
>I'm fine if you prefer not to include this in the
>current patch. In that case, I can propose a separate
>patch.

Agreed, an empty rounds= silently clamping to 1000 does
not look right, and checking num == endp would fix it.
I would still prefer to keep it out of this patch: it is
a behavior change (a new ERROR for input that is accepted
today) beyond the reported bug, and I would rather not
widen the scope of a patch that is meant to be
backpatched to REL_18_STABLE. Please do propose it as
a separate patch, as you offered; it stands on its own
that way.

>This test containing 10,000 digits is rather long. I
>think we could simplify it using repeat(), for example:

Also adopted. The test now builds the token as
'$5$rounds=' || repeat('0', 9999) || '5$saltsalt$'
which produces byte-identical input (9999 zeros followed
by a 5), so the coverage is unchanged. As a side effect
the patch file shrinks from 32K to 12K.

v5 is attached. It is otherwise identical to v4: the
pgcrypto suite passes 25/25 locally, and the comparison
battery output differs from v4 only in the shortened
NOTICE echoes described above.

Testing was done with AI assistance.

Regards,
Prashant Chinnam