Re: Dead stores in src/common/sha2.c

On Wed, May 29, 2019 at 11:01:05AM -0400, Tom Lane wrote:
> Michael Paquier <michael(at)paquier(dot)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