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
