On Wed, May 29, 2019 at 11:01:05AM -0400, Tom Lane wrote: > Michael Paquier <mich...@paquier.xyz> writes: > > On Wed, May 29, 2019 at 01:24:19PM +0000, Hamlin, Garick L wrote: > >> I ran clang checker and noticed these. It looks like the > >> sha2 implementation is trying to zero out state on exit, but > >> clang checker finds at least 'a' is a dead store. > >> > >> Should we fix this? > >> Is something like the attached sensible? > >> Is there a common/better approach to zero-out in PG ? > > > This code comes from the SHA-2 implementation of OpenBSD, so it is not > > adapted to directly touch it. What's the current state of this code > > in upstream? Should we perhaps try to sync with the upstream > > implementation instead? > > After a quick search I am not seeing that this area has actually > > changed: > > http://fxr.watson.org/fxr/source/crypto/sha2.c?v=OPENBSD > > Hm ... plastering "volatile"s all over it isn't good for readability > or for quality of the generated code. (In particular, I'm worried > about this patch causing all those variables to be forced into memory > instead of registers.)
Yeah, I don't actually think it's a great approach which is why I was wondering what if PG had a right approach. I figured it was the clearest way to start the discussion. Especially, since I wasn't sure if people would want to fix it. > At the same time, I'm not sure if we should just write this off as an > ignorable warning. If the C compiler concludes these are dead stores > it'll probably optimize them away, leading to not accomplishing the > goal of wiping the values. Yeah, I mean it's odd to put code in to zero/hide state knowing it's probably optimized out. We could also take it out, but maybe it does help somewhere? ... or put in a comment that says: This probably gets optimized away, but we don't consider it much of a risk. > On the third hand, that goal may not be worth much, particularly not > if the variables do get kept in registers. I haven't looked at the asm. Maybe they are in registers... Garick
