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

Hi Takeshi,

Thank you for the review and for the alternative patch.

> How about using strtol or strtoi64 instead of strtoint to handle
> the rounds value that is out of the integer bounds? I attached
> my patch using strtoi64.

I built your patch on master (f52d4051) and ran the same battery
I used for the original report against unpatched master, my v1,
and your version. The results are identical to v1 on every case:
values between the int and int64 ranges parse exactly and hit
the existing clamps, and values beyond int64 saturate to
INT64_MAX / INT64_MIN and hit them too. Because your NOTICEs
print the supplied token with %.*s, the saturated value never
leaks into the message either: rounds=99999999999999999999
reports the value as supplied and clamps to the maximum, just
as v1 does. The regression tests from v1 also pass unchanged
against your code (pgcrypto suite 25/25).

I agree yours is simpler: no errno handling, no sign check, and
no duplicated ereport blocks. I will adopt it for v2.

One cosmetic difference I noticed: since the clamp NOTICEs now
print the token as supplied for every out-of-range value, a
non-canonical token changes text -- rounds=+10 and rounds=010
now report "rounds=+10" / "rounds=010" instead of "rounds=10".
That seems harmless, and arguably more accurate, but I wanted
to flag it, since v1 deliberately left the in-range messages
untouched.

> Most of the similar postgresql code is just like
> "if (errno == ERANGE)" instead of using another variable.

With the strtoi64 approach there is no errno check left at
all, so the rounds_overflow variable goes away with it.

> We might be OK to turn negative rounds value to error since
> negative rounds value does not make any sense.

I would prefer to keep clamping negative values in this patch.
Clamping is the long-standing behaviour for values that fit in
an int today -- rounds=-5 gives the below-minimum NOTICE and
1000 rounds on current master -- and this bug is specifically
about the silent modulo 2^32 wrap of values that do not fit.
Turning negative values into an ERROR would change behaviour
for inputs that currently work, and it would be hard to
backpatch, while the wrap fix itself should be backpatchable
to REL_18_STABLE. If negative rounds should error, I think
that deserves its own discussion and a master-only change.
What do you think?

I will post a v2 with the strtoi64 change and the regression
tests from v1 (unchanged) shortly.

Testing was done with AI assistance.
Regards,
Prashant Chinnam