Re: Modern SHA2- based password hashes for pgcrypto

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