Thread: Re: Modern SHA2- based password hashes for pgcrypto

Am Freitag, dem 24.01.2025 um 19:06 +0100 schrieb Alvaro Herrera:
> > So we behave exactly the same way as px_crypt_md5(): It stops after
> > the
> > first '$' after the magic byte preamble. For shacrypt, this could
> > be
> > the next '$' after the closing one of the non-mandatory 'rounds'
> > option, but with your example this doesn't happen since it gets
> > never
> > parsed. The salt length will be set to 0.
>
> IMO silently using no salt or 0 iterations because the input is
> somewhat
> broken is bad security and should be rejected.  If we did so in the
> past
> without noticing, that's bad already, but we should not replicate
> that
> behavior any further.

Following Japin's example we parse the broken input after the magic
byte into

$6$rounds=10000$/Zg436s2vmTwsoSz

So this is what gets passed over to the next steps after extracting the
magic byte.

We then try to parse the optional rounds option, which is expected to
be at the very first characters at this stage, which isn't true. In
that case we fall back to default 5000 iterations. So no problem with
iterations here. Note that we don't output the rounds option in that
case. I adapted this from Drepper's code to be compatible with the
tests (it suppresses the rounds option in case the default is used). I
forgot to adjust the docs about this change.

But we don't extract the salt because of the first '$', which confused
the input handling that there isn't anything interesting for it. And
that get unnoticed currently, which might be a problem for the caller.
Well, you see that something is wrong when looking at the result, but
if that happens programmatically it might be hidden.

I've looked what others do:

Drepper's code with the input above produces the following:

$5$=10000$cWSkAaJa1mL912.HQqjIhODJ9b3S7hmgsb/k9Efp7.7

It parses the salt from the input as "=10000".

Python's passlib is very strict when it comes to supported characters
within a salt string. It rejects everything thats not matching '[./0-
9A-Za-z]'. So when you provide the example above you get

  File "/usr/lib/python3.13/site-packages/passlib/utils/handlers.py",
line 1459, in _norm_salt
    raise ValueError("invalid characters in %s salt" % cls.name)
ValueError: invalid characters in sha256_crypt salt

openssl-passwd does this:

echo test | openssl passwd -5 -stdin -salt
'$6$rounds=10000$/Zg436s2vmTwsoSz'

$5$$6$rounds=10000$$BFtTxJrvpb/ra8SnnXkiNCJ1HGZ3OOmwvyQ2TJZx44B

It absorbs everything up to 16 bytes and uses that as the salt string.

Interestingly, python-passlib and Drepper's code accept both empty
salt, openssl-passwd seems to be buggy:

echo test | openssl passwd -5 -stdin -salt ''
<NULL>

So at least, they do *something* with the input and don't silently omit
salting the input passphrase, but the results aren't the same hash.

So i think we have two options: adapt Drepper's code and throw away
everything or be very strict like python-passlib. I myself can find
support for both, so not sure.


    Bernd

On 2025-Jan-28, Bernd Helmle wrote:

> Python's passlib is very strict when it comes to supported characters
> within a salt string. It rejects everything thats not matching '[./0-
> 9A-Za-z]'. So when you provide the example above you get

The reason it uses these chars is that in their scheme the salt bytes
are base64-encoded.

The passlib docs has this page about the "modular crypt format":
https://passlib.readthedocs.io/en/stable/modular_crypt_format.html

and they point this other page as a "modern, non-ambiguous standard":
https://github.com/P-H-C/phc-string-format/blob/master/phc-sf-spec.md
About the salt, this last document says:

  The role of salts is to achieve uniqueness. A random salt is fine for
  that as long as its length is sufficient; a 16-byte salt would work
  well (by definition, UUID are very good salts, and they encode over
  exactly 16 bytes). 16 bytes encode as 22 characters in B64. Functions
  should disallow salt values that are too small for security (4 bytes
  should be viewed as an absolute minimum).

This "Password Hashing Competition" organization hardly seems an
authority though.  It'd be great to have an IETF standard about this ...

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/

On Thu, 06 Feb 2025 at 11:20, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> On 2025-Jan-28, Bernd Helmle wrote:
>
>> Python's passlib is very strict when it comes to supported characters
>> within a salt string. It rejects everything thats not matching '[./0-
>> 9A-Za-z]'. So when you provide the example above you get
>
> The reason it uses these chars is that in their scheme the salt bytes
> are base64-encoded.
>
> The passlib docs has this page about the "modular crypt format":
> https://passlib.readthedocs.io/en/stable/modular_crypt_format.html
>
> and they point this other page as a "modern, non-ambiguous standard":
> https://github.com/P-H-C/phc-string-format/blob/master/phc-sf-spec.md
> About the salt, this last document says:
>
>   The role of salts is to achieve uniqueness. A random salt is fine for
>   that as long as its length is sufficient; a 16-byte salt would work
>   well (by definition, UUID are very good salts, and they encode over
>   exactly 16 bytes). 16 bytes encode as 22 characters in B64. Functions
>   should disallow salt values that are too small for security (4 bytes
>   should be viewed as an absolute minimum).
>
> This "Password Hashing Competition" organization hardly seems an
> authority though.  It'd be great to have an IETF standard about this ...

Yeah.

Since there is no standard, how do we handle this?  I prefer to use the strict
mode like passlib.

-- 
Regrads,
Japin Li

On 2025-Feb-07, Japin Li wrote:

> Since there is no standard, how do we handle this?  I prefer to use
> the strict mode like passlib.

I definitely like that passlib have documented their thought process
thoroughly.

I think using their strict mode is good on principle, but if we're going
to do that, then the salt string should not be used verbatim, but
instead base64-decoded first to get the actual salt bytes, like they do.
Does this break compabitibility with other systems?  Are
passlib-generated password hashes incompatible with, say, "openssl
passwd" which you (Bernd) mentioned at the beginning of the thread?
Maybe if the password hashes are no longer compatible, then we should
ditch the idea of restricting salts to base64 chars and accept the whole
range of bytes, like Drepper.

But in any case ISTM we should reject, as they suggest, the use of less
than 4 bytes of salt (and should perhaps settle for a default of 16, as
passlib suggests).  I suppose this is why passlib returns NULL with
empty salt.  What we should do in that case IMO is ereport(ERROR).  

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"Porque Kim no hacía nada, pero, eso sí,
con extraordinario éxito" ("Kim", Kipling)

Hi,

> I definitely like that passlib have documented their thought process
> thoroughly.
>

Please find attached v4 of this patch. I added the following changes:

- Check for non-supported characters in the salt like passlib does.
- Check for reserved tokens when parsing the salt string (i find this
very strict, but it covers the cases Japin Li pointed out).

I didn't implement being more strict when it comes to the salt length:
the salt might be provided from external sources and the password hash
is just used for validating within the database.

When hashing the password within postgres, users always should use
gen_salt(), which generates a reasonable salt string.

Maybe, in case of empty salts, we should issue a WARNING instead of
erroring out and put additional documentation on how to use it right.

> I think using their strict mode is good on principle, but if we're
> going
> to do that, then the salt string should not be used verbatim, but
> instead base64-decoded first to get the actual salt bytes, like they
> do.
> Does this break compabitibility with other systems?  Are
> passlib-generated password hashes incompatible with, say, "openssl
> passwd" which you (Bernd) mentioned at the beginning of the thread?
> Maybe if the password hashes are no longer compatible, then we should
> ditch the idea of restricting salts to base64 chars and accept the
> whole
> range of bytes, like Drepper.
>
> But in any case ISTM we should reject, as they suggest, the use of
> less
> than 4 bytes of salt (and should perhaps settle for a default of 16,
> as
> passlib suggests).  I suppose this is why passlib returns NULL with
> empty salt.  What we should do in that case IMO is ereport(ERROR).
>


Hmm, i didn't understand that passlib does decode them first, i thought
they use it encoded... at least, in our current form we're pretty much
compatible with Drepper, passlib and OpenSSL, as far as i tested:

Empty salt
----------

# Drepper reference code

shacrypt password "" sha512crypt
$6$$bLTg4cpho8PIUrjfsE7qlU08Qx2UEfw..xOc6I1wpGVtyVYToGrr7BzRdAAnEr5lYFr
1Z9WcCf1xNZ1HG9qFW1

# Patch

SELECT crypt('password', '$6$$');
                                           crypt
───────────────────────────────────────────────────────────────────────
─────────────────────
$6$$bLTg4cpho8PIUrjfsE7qlU08Qx2UEfw..xOc6I1wpGVtyVYToGrr7BzRdAAnEr5lYFr1
Z9WcCf1xNZ1HG9qFW1
(1 row)

# Passlib

from passlib.hash import sha256_crypt,sha512_crypt
hash = sha512_crypt.using(salt='').using(rounds=5000).hash("password")
print(hash)
$6$$bLTg4cpho8PIUrjfsE7qlU08Qx2UEfw..xOc6I1wpGVtyVYToGrr7BzRdAAnEr5lYFr
1Z9WcCf1xNZ1HG9qFW1

# OpenSSL

echo password | openssl passwd -6 -stdin -salt ''
<NULL>

Short salt
----------

# Drepper reference code

./shacrypt password abcdef sha512crypt
$6$abcdef$2yYFwtar3.rlLovG0bXbxXMjZe7Z/1EhQ0gEs0nXalsCW04p2iy6unkvbrBFH
1SXexabbdNXXOO11F7sdyurk/

# Patch

SELECT crypt('password', '$6$abcdef$');
                                              crypt
───────────────────────────────────────────────────────────────────────
───────────────────────────
$6$abcdef$2yYFwtar3.rlLovG0bXbxXMjZe7Z/1EhQ0gEs0nXalsCW04p2iy6unkvbrBFH1
SXexabbdNXXOO11F7sdyurk/
(1 row)

# Passlib

from passlib.hash import sha256_crypt,sha512_crypt
hash =
sha512_crypt.using(salt='abcdef').using(rounds=5000).hash("password")
print(hash)
$6$abcdef$2yYFwtar3.rlLovG0bXbxXMjZe7Z/1EhQ0gEs0nXalsCW04p2iy6unkvbrBFH
1SXexabbdNXXOO11F7sdyurk/

# OpenSSL

echo password | openssl passwd -6 -stdin -salt 'abcdef'
$6$abcdef$2yYFwtar3.rlLovG0bXbxXMjZe7Z/1EhQ0gEs0nXalsCW04p2iy6unkvbrBFH
1SXexabbdNXXOO11F7sdyurk/

Salt > MAXLEN(16 Bytes)
-----------------------

# Drepper reference code
(truncates salt)

shacrypt password abcdefghijklmnopqrstuvwxyz sha512crypt
$6$abcdefghijklmnop$0aenUFHf897F9u0tURIHOeACWajSuVGa7jgJGyq.DKZm/WXl/IZ
FvPbneFydBjomEOgM.Sh1m0L3KsS1.H5b//

# Patch
(truncates salt)

SELECT crypt('password', '$6$abcdefghijklmnopqrstuvwxyz$');
                                                   crypt
───────────────────────────────────────────────────────────────────────
─────────────────────────────────────
$6$abcdefghijklmnop$0aenUFHf897F9u0tURIHOeACWajSuVGa7jgJGyq.DKZm/WXl/IZF
vPbneFydBjomEOgM.Sh1m0L3KsS1.H5b//
(1 row)

# Passlib

Errors out with

ValueError: salt too large (sha512_crypt requires <= 16 chars)

# OpenSSL
(truncates salt)

echo password | openssl passwd -6 -stdin -salt
'abcdefghijklmnopqrstuvwxyz'
$6$abcdefghijklmnop$0aenUFHf897F9u0tURIHOeACWajSuVGa7jgJGyq.DKZm/WXl/IZ
FvPbneFydBjomEOgM.Sh1m0L3KsS1.H5b//

Salt with "invalid" character
-----------------------------

# Drepper reference code

./shacrypt password abcdefghi%jklmno sha512crypt
$6$abcdefghi%jklmno$LMN/V1pW97IoK0rWSDVqCo9EYd6zpqP0TdTX9.cxFAFqsdSMWQM
jehkmMtDzL36VBKeG6dg.kFAQKoFvZpK0G.

# Patch
current version V4 (attached)

Errors out:

SELECT crypt('password', '$6$abcdefghi%jklmno$');
FEHLER:  invalid character in salt string: "%"
Time: 0,217 ms

# Passlib

Errors out

ValueError: invalid characters in sha512_crypt salt

# OpenSSL

echo password | openssl passwd -6 -stdin -salt 'abcdefghi%jklmno'
$6$abcdefghi%jklmno$LMN/V1pW97IoK0rWSDVqCo9EYd6zpqP0TdTX9.cxFAFqsdSMWQM
jehkmMtDzL36VBKeG6dg.kFAQKoFvZpK0G.


So the hashes we produce are identical, but with being more strict we
differ in the handling of the provided salt.

    Bernd

On 2025-Mar-11, Bernd Helmle wrote:

> Please find attached v4 of this patch. I added the following changes:
> 
> - Check for non-supported characters in the salt like passlib does.
> - Check for reserved tokens when parsing the salt string (i find this
> very strict, but it covers the cases Japin Li pointed out).
> 
> I didn't implement being more strict when it comes to the salt length:
> the salt might be provided from external sources and the password hash
> is just used for validating within the database.
> 
> When hashing the password within postgres, users always should use
> gen_salt(), which generates a reasonable salt string. 

This sounds reasonable to me.

> Maybe, in case of empty salts, we should issue a WARNING instead of
> erroring out and put additional documentation on how to use it right.

I don't know, that doesn't seem ideal to me, because it's very easy to
run stuff and never see the warnings.  If we find that people are
desperate to use empty salts, we can relax that later (turn the error to
a warning), but I'd rather not have it in the first cut.

> Hmm, i didn't understand that passlib does decode them first, i thought
> they use it encoded... at least, in our current form we're pretty much
> compatible with Drepper, passlib and OpenSSL, as far as i tested:

I am ready to believe that I misinterpreted what I read.

> So the hashes we produce are identical, but with being more strict we
> differ in the handling of the provided salt.

Okay.


I can offer a few cosmetic changes.  0001 is a pgindent run, and 0002 is
some manual adjustments after that.  There are only two nontrivial
changes

1. the calculation for rounds was using type long, which is confusing
because the range is different according to the platform.  Since it's
limited by the macro definitions to no more than 999999999, we can make
it an int32 instead.  So we use strtoint() instead of strtoul() to parse
the value, and remove the "l" suffixes from the macros that define the
limits and default, which were bugging me a bit when used in the
gen_list struct.

2. I changed "if (block % 3)" to "if ((block % 3) != 0)".

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"El destino baraja y nosotros jugamos" (A. Schopenhauer)

Hello,

I triggered a run of this on CI on all platforms.  It seems to have gone
well, so unless I hear complaints, I intend to get this out later today.

https://cirrus-ci.com/build/4613871211642880

Thanks,

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/

Am Donnerstag, dem 03.04.2025 um 20:39 +0200 schrieb Alvaro Herrera:
>
> > Maybe, in case of empty salts, we should issue a WARNING instead of
> > erroring out and put additional documentation on how to use it
> > right.
>
> I don't know, that doesn't seem ideal to me, because it's very easy
> to
> run stuff and never see the warnings.  If we find that people are
> desperate to use empty salts, we can relax that later (turn the error
> to
> a warning), but I'd rather not have it in the first cut.
>

That's a good idea, Let's go with that. Thanks again for working on
this.

> > Hmm, i didn't understand that passlib does decode them first, i
> > thought
> > they use it encoded... at least, in our current form we're pretty
> > much
> > compatible with Drepper, passlib and OpenSSL, as far as i tested:
>
> I am ready to believe that I misinterpreted what I read.
>

I hope i didn't parse it wrong either. But i didn't see forcing
something like this according in either passlib and Drepper's code.
Maybe we need have to look closer again ...

[...]

>
>
> I can offer a few cosmetic changes.  0001 is a pgindent run, and 0002
> is
> some manual adjustments after that.  There are only two nontrivial
> changes
>
> 1. the calculation for rounds was using type long, which is confusing
> because the range is different according to the platform.  Since it's
> limited by the macro definitions to no more than 999999999, we can
> make
> it an int32 instead.  So we use strtoint() instead of strtoul() to
> parse
> the value, and remove the "l" suffixes from the macros that define
> the
> limits and default, which were bugging me a bit when used in the
> gen_list struct.

+1



--
Bernd Helmle

Blücherstrasse 17
41061 Mönchengladbach

Tel.: +49 172 726 99 66

Hello,

I have pushed this now, hoping it won't explode.

Thanks!

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"No es bueno caminar con un hombre muerto"

Am Samstag, dem 05.04.2025 um 19:22 +0200 schrieb Alvaro Herrera:
> Hello,
>
> I have pushed this now, hoping it won't explode.
>
> Thanks!

Very cool, i keep my fingers crossed.

Thanks,
    Bernd

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> I have pushed this now, hoping it won't explode.

mamba is not happy:

ccache cc -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Werror=vla -Wendif-labels
-Wmissing-format-attribute-Wimplicit-fallthrough=3 -Wcast-function-type -Wshadow=compatible-local -Wformat-security
-fno-strict-aliasing-fwrapv -fexcess-precision=standard -Wno-format-truncation -Wno-stringop-truncation -g -O2 -Werror
-fPIC-DPIC -fvisibility=hidden -I. -I. -I../../src/include   -I/usr/pkg/include/libxml2 -I/usr/pkg/include
-I/usr/pkg/include -I/usr/pkg/include  -c -o pgcrypto.o pgcrypto.c 
In file included from /usr/include/ctype.h:100,
                 from ../../src/include/port.h:16,
                 from ../../src/include/c.h:1345,
                 from ../../src/include/postgres.h:48,
                 from crypt-sha.c:46:
crypt-sha.c: In function 'px_crypt_shacrypt':
crypt-sha.c:324:16: error: array subscript has type 'char' [-Werror=char-subscripts]
  324 |    if (isalpha(*ep) || isdigit(*ep) || (*ep == '.') || (*ep == '/'))
      |                ^
crypt-sha.c:324:32: error: array subscript has type 'char' [-Werror=char-subscripts]
  324 |    if (isalpha(*ep) || isdigit(*ep) || (*ep == '.') || (*ep == '/'))
      |                                ^
cc1: all warnings being treated as errors
gmake[1]: *** [<builtin>: crypt-sha.o] Error 1

What this is on about is that portable use of isalpha() or isdigit()
requires casting a "char" value to "unsigned char".  I was about to
make that simple change when I started to question if we actually
want to be using <ctype.h> here at all.  Do you REALLY intend that
any random non-ASCII letter is okay to include in the decoded_salt?
Are you okay with that filter being locale-dependent?

I'd be more comfortable with a check like

    if (strchr("...valid chars...", *ep) != NULL)

It looks like "_crypt_itoa64" might be directly usable as the
valid-chars string, too.  (BTW, why is _crypt_itoa64 not
marked const?)

            regards, tom lane

Hi,

On 2025-04-05 19:22:58 +0200, Alvaro Herrera wrote:
> I have pushed this now, hoping it won't explode.

I have a WIP patch that adds gcc specific allocator attributes for palloc et
al. Just rebased that. It warns on the new code as follows:

[1489/1804 42  82%] Compiling C object contrib/pgcrypto/pgcrypto.so.p/crypt-sha.c.o
../../../../../home/andres/src/postgresql/contrib/pgcrypto/crypt-sha.c: In function 'px_crypt_shacrypt':
../../../../../home/andres/src/postgresql/contrib/pgcrypto/crypt-sha.c:605:13: warning: pointer 'cp' may be used after
'pfree'[-Wuse-after-free]
 
  605 |         *cp = '\0';
      |         ~~~~^~~~~~
../../../../../home/andres/src/postgresql/contrib/pgcrypto/crypt-sha.c:533:9: note: call to 'pfree' here
  533 |         pfree(s_bytes);
      |         ^~~~~~~~~~~~~~

And it sure seems to have a point.  I'm surprised this isn't causing wider
issues...

Greetings,

Andres Freund

Am Sonntag, dem 06.04.2025 um 15:43 -0400 schrieb Tom Lane:
> What this is on about is that portable use of isalpha() or isdigit()
> requires casting a "char" value to "unsigned char".  I was about to
> make that simple change when I started to question if we actually
> want to be using <ctype.h> here at all.  Do you REALLY intend that
> any random non-ASCII letter is okay to include in the decoded_salt?
> Are you okay with that filter being locale-dependent?
>
> I'd be more comfortable with a check like
>
>     if (strchr("...valid chars...", *ep) != NULL)
>

Hmm, the idea was to allow a wider range of letters for the salt. This
came from my experiments that openssl allows more than just the ones
from _crypt_itoa64. But after reading this, i realized even isalpha()
isn't enough, since e.g. mulitbyte character wouldn't work anyways,
like openssl allows:

echo -n password | openssl passwd -5 -salt ÄÖÜ -stdin
$5$ÄÖÜ$NduBUgCdzvnW1lW8EMxW9DuxN6HmT0niS7H4ftRxuX0

So we would have to test for these cases, too. I looked again into
pyhon's passlib, and they don't accept any non-ASCII fancy characters,
neither.

So i think Tom has a point, we can restrict this to the characters from
_crypt_itoa64 and go that route. I am not sure about externally
generated hashes which are going to be stored in postgres and validated
later there. This can restrict the use case perhaps.

> It looks like "_crypt_itoa64" might be directly usable as the
> valid-chars string, too.  (BTW, why is _crypt_itoa64 not
> marked const?)

That's an oversight by me.

I can create a patch with the fixes (and the one Andres' reported) for
today.

Thanks

    Bernd

Am Sonntag, dem 06.04.2025 um 23:02 -0400 schrieb Andres Freund:
> Hi,
>
> On 2025-04-05 19:22:58 +0200, Alvaro Herrera wrote:
> > I have pushed this now, hoping it won't explode.
>
> I have a WIP patch that adds gcc specific allocator attributes for
> palloc et
> al. Just rebased that. It warns on the new code as follows:
>
> [1489/1804 42  82%] Compiling C object
> contrib/pgcrypto/pgcrypto.so.p/crypt-sha.c.o
> ../../../../../home/andres/src/postgresql/contrib/pgcrypto/crypt-
> sha.c: In function 'px_crypt_shacrypt':
> ../../../../../home/andres/src/postgresql/contrib/pgcrypto/crypt-
> sha.c:605:13: warning: pointer 'cp' may be used after 'pfree' [-Wuse-
> after-free]
>   605 |         *cp = '\0';
>       |         ~~~~^~~~~~
> ../../../../../home/andres/src/postgresql/contrib/pgcrypto/crypt-
> sha.c:533:9: note: call to 'pfree' here
>   533 |         pfree(s_bytes);
>       |         ^~~~~~~~~~~~~~
>
> And it sure seems to have a point.  I'm surprised this isn't causing
> wider
> issues...

Indeed. I think this assignment is useless anyways, since s_bytes is
already allocated with palloc0. I must have overseen this one when
rearranging code...but yes, strange that it didn't cause drama.


Thanks
    Bernd

Am Sonntag, dem 06.04.2025 um 15:43 -0400 schrieb Tom Lane:
> I'd be more comfortable with a check like
>
>     if (strchr("...valid chars...", *ep) != NULL)
>
> It looks like "_crypt_itoa64" might be directly usable as the
> valid-chars string, too.  (BTW, why is _crypt_itoa64 not
> marked const?)

Here is a patch that tries to address all these issues (including
Andres' report). I've adjusted the error message and use ereport(), so
it might be more useful if we deal with not just single byte letters.

I've also changed _crypt_itoa64 from unsigned char to char, since this
seems what strchr() expects (at least on my machine) and we don't deal
specifically elsewhere with that.


Thanks,
    Bernd

Bernd Helmle <mailings@oopsware.de> writes:
> Here is a patch that tries to address all these issues (including
> Andres' report). I've adjusted the error message and use ereport(), so
> it might be more useful if we deal with not just single byte letters.

I'd like to get mamba back to green, so I'll take care of this
(unless Alvaro is already on it?)

            regards, tom lane

Hi,

On 2025-04-07 09:09:39 +0200, Bernd Helmle wrote:
> Am Sonntag, dem 06.04.2025 um 23:02 -0400 schrieb Andres Freund:
> > On 2025-04-05 19:22:58 +0200, Alvaro Herrera wrote:
> > > I have pushed this now, hoping it won't explode.
> > 
> > I have a WIP patch that adds gcc specific allocator attributes for
> > palloc et
> > al. Just rebased that. It warns on the new code as follows:
> > 
> > [1489/1804 42  82%] Compiling C object
> > contrib/pgcrypto/pgcrypto.so.p/crypt-sha.c.o
> > ../../../../../home/andres/src/postgresql/contrib/pgcrypto/crypt-
> > sha.c: In function 'px_crypt_shacrypt':
> > ../../../../../home/andres/src/postgresql/contrib/pgcrypto/crypt-
> > sha.c:605:13: warning: pointer 'cp' may be used after 'pfree' [-Wuse-
> > after-free]
> >   605 |         *cp = '\0';
> >       |         ~~~~^~~~~~
> > ../../../../../home/andres/src/postgresql/contrib/pgcrypto/crypt-
> > sha.c:533:9: note: call to 'pfree' here
> >   533 |         pfree(s_bytes);
> >       |         ^~~~~~~~~~~~~~
> > 
> > And it sure seems to have a point.  I'm surprised this isn't causing
> > wider
> > issues...
> 
> Indeed. I think this assignment is useless anyways, since s_bytes is
> already allocated with palloc0. I must have overseen this one when
> rearranging code...but yes, strange that it didn't cause drama.

Valgrind did find it actually, I just missed it due to other failures:

https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=skink&dt=2025-04-05%2020%3A52%3A38&stg=recovery-check

==2917549== VALGRINDERROR-BEGIN
==2917549== Invalid write of size 1
==2917549==    at 0x11D4AD9C: px_crypt_shacrypt (crypt-sha.c:605)
==2917549==    by 0x11D54B50: run_crypt_sha (px-crypt.c:76)
==2917549==    by 0x11D54BCB: px_crypt (px-crypt.c:119)
==2917549==    by 0x11D4BCBB: pg_crypt (pgcrypto.c:228)
==2917549==    by 0x3BBCEA: ExecInterpExpr (execExprInterp.c:1001)
==2917549==    by 0x3B790E: ExecInterpExprStillValid (execExprInterp.c:2299)
==2917549==    by 0x48FD40: ExecEvalExprSwitchContext (executor.h:466)
==2917549==    by 0x48FD40: evaluate_expr (clauses.c:5012)
==2917549==    by 0x48FF08: evaluate_function (clauses.c:4519)
==2917549==    by 0x492054: simplify_function (clauses.c:4108)
==2917549==    by 0x4901FA: eval_const_expressions_mutator (clauses.c:2593)
==2917549==    by 0x432CDE: expression_tree_mutator_impl (nodeFuncs.c:3486)
==2917549==    by 0x48FFB9: eval_const_expressions_mutator (clauses.c:3727)
==2917549==  Address 0x82fa740 is 912 bytes inside a recently re-allocated block of size 8,192 alloc'd
==2917549==    at 0x4844818: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==2917549==    by 0x71EF75: AllocSetContextCreateInternal (aset.c:444)
==2917549==    by 0x3D17AE: CreateExprContextInternal (execUtils.c:260)
==2917549==    by 0x3D1CA8: CreateExprContext (execUtils.c:310)
==2917549==    by 0x3D1ED8: MakePerTupleExprContext (execUtils.c:462)
==2917549==    by 0x48FDBF: evaluate_expr (clauses.c:5013)
==2917549==    by 0x48FF08: evaluate_function (clauses.c:4519)
==2917549==    by 0x492054: simplify_function (clauses.c:4108)
==2917549==    by 0x4901FA: eval_const_expressions_mutator (clauses.c:2593)
==2917549==    by 0x432CDE: expression_tree_mutator_impl (nodeFuncs.c:3486)
==2917549==    by 0x48FFB9: eval_const_expressions_mutator (clauses.c:3727)
==2917549==    by 0x432EFD: expression_tree_mutator_impl (nodeFuncs.c:3572)
==2917549== 
==2917549== VALGRINDERROR-END
{
   <insert_a_suppression_name_here>
   Memcheck:Addr1
   fun:px_crypt_shacrypt
   fun:run_crypt_sha
   fun:px_crypt
   fun:pg_crypt
   fun:ExecInterpExpr
   fun:ExecInterpExprStillValid
   fun:ExecEvalExprSwitchContext
   fun:evaluate_expr
   fun:evaluate_function
   fun:simplify_function
   fun:eval_const_expressions_mutator
   fun:expression_tree_mutator_impl
   fun:eval_const_expressions_mutator
}
**2917549** Valgrind detected 1 error(s) during execution of "SELECT crypt('', '$5$Szzz0yzz');"

Greetings,

Andres

I'd like to get mamba back to green, so I'll take care of this

(unless Alvaro is already on it?)

I wrote:
> Bernd Helmle <mailings@oopsware.de> writes:
>> Here is a patch that tries to address all these issues (including
>> Andres' report). I've adjusted the error message and use ereport(), so
>> it might be more useful if we deal with not just single byte letters.

> I'd like to get mamba back to green, so I'll take care of this

And done.  FYI, we actually have a standard practice for error
messages that want to complain about a single character without
assuming it's a single-byte character:

                ereport(ERROR,
                        errcode(ERRCODE_INVALID_PARAMETER_VALUE),
                        errmsg("invalid character in salt string: \"%.*s\"",
                               pg_mblen(ep), ep));

            regards, tom lane