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

From: Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com>
To: Prashant Chinnam <crprashant(at)hotmail(dot)com>
Cc: Takeshi Ideriha <iderihatakeshi(at)gmail(dot)com>, "pgsql-hackers(at)lists(dot)postgresql(dot)org" <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Subject: Re: [PATCH] pgcrypto: Fix sha-crypt rounds= values that do not fit in int (BUG #19743)
Date: 2026-11-02 02:05:21
Message-ID: A480A4D0-4F99-428E-89B6-08631992E652@gmail.com
Views: Whole Thread | Raw Message | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

> On Oct 31, 2026, at 02:41, Prashant Chinnam <crprashant(at)hotmail(dot)com> wrote:
>
> Hi,
>
> Attached is v4 of this patch.
>
> As promised in the strtoint() thread, now that Tom's
> commit 426551c8cf49 has landed and been backpatched
> through PG 14 (REL_18_STABLE: ec65cbc00b2c), this
> version switches the rounds= parse back to strtoint(),
> relying on its new saturating behavior on overflow.
>
> The delta against v3 is exactly one line in
> crypt-sha.c (strtoi64() -> strtoint()); the clamping,
> the NOTICE print cap, and all regression tests are
> unchanged from v3. I verified it locally against
> current master: the pgcrypto suite passes 25/25, and
> the overflow battery and NOTICE probe output are
> identical to v3.
>
> Since the strtoint() change is backpatched, this
> version should backpatch to REL_18_STABLE cleanly as
> well.
>
> Testing was done with AI assistance.
>
> Regards,
> Prashant Chinnam<v4-0001-pgcrypto-Fix-sha-crypt-rounds-values-that-do-not-fit.patch>

Thanks for updating the patch. I reviewed and tested v4 today. I have a few comments:

1
```
#define PX_SHACRYPT_ROUNDS_PRINT_MAX 32
```

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.

We could also define PX_SHACRYPT_ROUNDS_PRINT_MAX based on PX_SHACRYPT_ROUNDS_MAX, for example:
```
#define PX_SHACRYPT_ROUNDS_PRINT_MAX ((int) sizeof(CppAsString2(PX_SHACRYPT_ROUNDS_MAX)))
```

In this way, PX_SHACRYPT_ROUNDS_PRINT_MAX is 10, so the hint message is like:
```
evantest=# SELECT crypt('password', E'$5$rounds=1000000000000000000000000000000000000000000000000000000$saltsalt$');
NOTICE: rounds=1000000000... exceeds maximum supported value (999999999), using 999999999 instead
```

2
```
if (*endp != '$')
ereport(ERROR,
errcode(ERRCODE_SYNTAX_ERROR),
errmsg("could not parse salt options"));
```

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:
```
evantest=# SELECT crypt('password', E'$5$rounds=$saltsalt$');
NOTICE: rounds= is below supported value (1000), using 1000 instead
crypt
---------------------------------------------------------------------
$5$rounds=1000$saltsalt$azOwbpkvuuBKkE82dQPwTsQE8JyT9Fflpr9aKid3aT9
(1 row)
```

When no digits are converted, strtoint() returns 0 and sets endp to num. Since num points to $, the existing check passes, and the value is clamped to 1000.

I think an empty rounds value should be rejected. We could handle this by checking whether any characters were consumed:
```
if (num == endp || *endp != '$')
ereport(ERROR,
errcode(ERRCODE_SYNTAX_ERROR),
errmsg("could not parse salt options"));
```

I'm fine if you prefer not to include this in the current patch. In that case, I can propose a separate patch.

3
```
SELECT crypt('password', '$5$rounds=0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000005$saltsalt$')
= '$5$rounds=1000$saltsalt$azOwbpkvuuBKkE82dQPwTsQE8JyT9Fflpr9aKid3aT9' AS result;

```

This test containing 10,000 digits is rather long. I think we could simplify it using repeat(), for example:
```
SELECT crypt('password’, '$5$rounds=' || repeat('0', 9999) || '5$saltsalt$’)
= '$5$rounds=1000$saltsalt$azOwbpkvuuBKkE82dQPwTsQE8JyT9Fflpr9aKid3aT9’ AS result;
```

This would preserve the test coverage while making the SQL much shorter and easier to maintain.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Fujii Masao 2026-11-02 02:32:58 Re: bug: query returns different result with and without memoization.
Previous Message Peter Smith 2026-11-02 02:04:07 Re: Avoid resetting subscription two_phase state to pending when already enabled