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
