Search Postgresql Archives

Re: md5 issues Postgres14 on OL7

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sat, Jan 08, 2022 at 02:00:16PM -0500, Tom Lane wrote:
> This is looking pretty solid to me.  Just a couple of nitpicks:
> 
> * In most places you initialize variables holding error strings to NULL:
> 
> +	const char *logdetail = NULL;
> 
> but there are three or so spots that don't, eg PerformRadiusTransaction.
> They should be consistent.  (Even if there's no actual bug, I'd be
> unsurprised to see Coverity carp about the inconsistency.)

Hmm.  I have spotted five of them, with one in passwordcheck.

> * The comments for md5_crypt_verify and plain_crypt_verify claim that
> the error string is "optionally" stored, but I don't see anything
> optional about it.  To me, "optional" would imply coding like
> 
> 	if (logdetail)
> 	    *logdetail = errstr;
> 
> which we don't have here, and I don't think we need it.  But the
> comments need adjustment.  (They were wrong before too, but no
> time like the present to clean them up.)

Makes sense.

> * I'd be inclined to just drop the existing comments like
> 
> -	 * We do not bother setting logdetail for any pg_md5_encrypt failure
> -	 * below: the only possible error is out-of-memory, which is unlikely, and
> -	 * if it did happen adding a psprintf call would only make things worse.
> 
> rather than modify them.   Neither the premise nor the conclusion
> of these comments is accurate anymore.  (I think that the psprintf
> they are talking about is the one that will happen inside elog.c
> to construct an errdetail string.  Yeah, there's some risk there,
> but I think it's minimal because of the fact that we preallocate
> some space in ErrorContext.)

Okay, that's fine by me.

> Other than those things, I think v3 is good to go.

I have done an extra pass on all that, and the result seemed fine to
me, so applied.  I have changed the non-OpenSSL code path of pgcrypto
to deal with that in 14 (does not exist on HEAD).  Thanks a lot for
the successive reviews!

The patch was invasive enough, but we could do more here:
- Add the same facility for HMAC.  That's not worth on REL_14_STABLE
based on the existing set of callers, but I'd like to do something
about that on HEAD as that could be helpful in the future.
- The error areas related to checksum_helper.c and backup_manifest.c
could be improved more.  Now these refer only to scenarios unlikely
going to happen in the field, so I have left that out.
--
Michael

Attachment: signature.asc
Description: PGP signature


[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [Postgresql Jobs]     [Postgresql Admin]     [Postgresql Performance]     [Linux Clusters]     [PHP Home]     [PHP on Windows]     [Kernel Newbies]     [PHP Classes]     [PHP Databases]     [Postgresql & PHP]     [Yosemite]

  Powered by Linux