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
