On Sun, 6 Dec 2020 at 00:42, Bruce Momjian <br...@momjian.us> wrote: > > On Sat, Dec 5, 2020 at 09:54:33PM +0900, Michael Paquier wrote: > > On Fri, Dec 04, 2020 at 10:52:29PM -0500, Bruce Momjian wrote: > > > OK, I worked with Sawada-san and added the attached patch. The updated > > > full patch is at the same URL: :-) > > > > > > > > > https://github.com/postgres/postgres/compare/master...bmomjian:key.diff > > > > Oh, I see that you use SHA256 during firstboot, which is why you > > bypass the resowner paths in cryptohash_openssl.c. Wouldn't it be > > better to use IsBootstrapProcessingMode() then? > > I tried that, but we also use the resource references before the > resource system is started even in non-bootstrap mode. Maybe we should > be creating a resource owner for this, but it is so early in the boot > process I don't know if that is safe. > > > > @@ -72,14 +72,15 @@ pg_cryptohash_create(pg_cryptohash_type type) > > > ctx = ALLOC(sizeof(pg_cryptohash_ctx)); > > > if (ctx == NULL) > > > return NULL; > > > + explicit_bzero(ctx, sizeof(pg_cryptohash_ctx)); > > > > > > state = ALLOC(sizeof(pg_cryptohash_state)); > > > if (state == NULL) > > > { > > > - explicit_bzero(ctx, sizeof(pg_cryptohash_ctx)); > > > FREE(ctx); > > > return NULL; > > > } > > > + explicit_bzero(state, sizeof(pg_cryptohash_state)); > > > > explicit_bzero() is used to give the guarantee that any sensitive data > > gets cleaned up after an error or on free. So I think that your use > > is incorrect here for an initialization. > > It turns out what we were missing was a clear of state->resowner in > cases where the resowner was null. I removed the bzero calls completely > and it now runs fine. > > > > if (state->evpctx == NULL) > > > { > > > - explicit_bzero(state, sizeof(pg_cryptohash_state)); > > > - explicit_bzero(ctx, sizeof(pg_cryptohash_ctx)); > > > #ifndef FRONTEND > > > ereport(ERROR, > > > (errcode(ERRCODE_OUT_OF_MEMORY), > > > > And this original position is IMO more correct. > > Do we even need them? > > > Anyway, at quick glance, the backtrace of upthread is indicating a > > double-free with an attempt to free a resource owner that has been > > already free'd. > > I think that is now fixed too. Updated patch at the same URL: > > > https://github.com/postgres/postgres/compare/master...bmomjian:key.diff
Thank you for updating the patch! I think we need explicit_bzero() also in freeing the keywrap context. BTW, when we need -R option pg_ctl command to start the server, how can we start it in the single-user mode? Regards, -- Masahiko Sawada http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
