Re: [patch for gcc12 stage1][version 2] add -ftrivial-auto-var-init and variable attribute "uninitialized" to gcc

[Kees Cook <keescook@chromium.org>]

On Fri, Apr 23, 2021 at 08:05:29PM +0100, Richard Sandiford wrote:
> Finally getting to this now that the GCC 11 rush is over.  Sorry for
> the slow response.
> 
> I've tried to review most of the code below, but skipped the testsuite
> parts in the interests of time.  I'll probably have more comments in
> future rounds, just wanted to get the ball rolling.
> 
> This is realy Richi's area more than mine though, so please take this
> with a grain of salt.
> 
> Qing Zhao <qing.zhao@oracle.com> writes:
> > 2.  initialize all paddings to zero when -ftrivial-auto-var-init is present.
> > In expr.c (store_constructor):
> >
> >         Clear the whole structure when
> >         -ftrivial-auto-var-init and the structure has paddings.
> >
> > In gimplify.c (gimplify_init_constructor):
> >
> >         Clear the whole structure when
> >         -ftrivial-auto-var-init and the structure has paddings.
> 
> Just to check: are we sure we want to use zero as the padding fill
> value even for -ftrivial-auto-var-init=pattern?  Or should it be
> 0xAA instead, to match the integer fill pattern?
> 
> I can see the arguments both ways, just thought it was worth asking.

I have no opinion myself, but I can give background.  Originally, Clang
implemented using pattern, but there was discussion around it and the
decision there was to go with zero init, as it seemed to more closely
match the C spec:
https://github.com/llvm/llvm-project/commit/d39fbc7e20d84364e409ce59724ce20625637062

> > +This is C and C++'s default.
> > +
> > +@item
> > +@samp{pattern} Initialize automatic variables with values which will likely
> > +transform logic bugs into crashes down the line, are easily recognized in a
> > +crash dump and without being values that programmers can rely on for useful
> > +program semantics.
> > +The values used for pattern initialization might be changed in the future.
> > +
> > +@item
> > +@samp{zero} Initialize automatic variables with zeroes.
> > +@end itemize
> > +
> > +The default is @samp{uninitialized}.
> > +
> > +You can control this behavior for a specific variable by using the variable
> > +attribute @code{uninitialized} (@pxref{Variable Attributes}).
> > +
> 
> I think it's important to say here that GCC still considers the
> variables to be uninitialised and still considers reading them to
> be undefined behaviour.  The option is simply trying to improve the
> security and predictability of the program in the presence of these
> uninitialised variables.

Excellent point, yes. That'd be good to call out.

> > […]
> > @@ -1831,6 +2000,17 @@ gimplify_decl_expr (tree *stmt_p, gimple_seq *seq_p)
> >  	       as they may contain a label address.  */
> >  	    walk_tree (&init, force_labels_r, NULL, NULL);
> >  	}
> > +      /* When there is no explicit initializer, if the user requested,
> > +	 We should insert an artifical initializer for this automatic
> > +	 variable for non vla variables.  */
> 
> I think we should explain why we can skip VLAs here.

FWIW, in testing, VLAs do get initialized, so I guess there's a separate
place where it happens.


Thanks for the review!

-- 
Kees Cook