Re: [PATCH v4 2/3] c++: Improve constexpr error for dangling local variables [PR110619]

[Nathaniel Shead <nathanieloshead@gmail.com>]

On Fri, Jul 21, 2023 at 05:44:51PM -0400, Jason Merrill wrote:
> On 7/21/23 01:39, Nathaniel Shead wrote:
> > On Thu, Jul 20, 2023 at 11:46:47AM -0400, Jason Merrill wrote:
> > > On 7/20/23 05:36, Nathaniel Shead wrote:
> > > > Currently, when typeck discovers that a return statement will refer to a
> > > > local variable it rewrites to return a null pointer. This causes the
> > > > error messages for using the return value in a constant expression to be
> > > > unhelpful, especially for reference return values.
> > > > 
> > > > This patch removes this "optimisation".
> > > 
> > > This isn't an optimization, it's for safety, removing a way for an attacker
> > > to get a handle on other data on the stack (CWE-562).
> > > 
> > > But I agree that we need to preserve some element of UB for constexpr
> > > evaluation to see.
> > > 
> > > Perhaps we want to move this transformation to cp_maybe_instrument_return,
> > > so it happens after maybe_save_constexpr_fundef?
> > 
> > Hm, OK. I can try giving this a go. I guess I should move the entire
> > maybe_warn_about_returning_address_of_local function to cp-gimplify.cc
> > to be able to detect this? Or is there a better way of marking that a
> > return expression will return a reference to a local for this
> > transformation? (I guess I can't use whether the warning has been
> > surpressed or not because the warning might not be enabled at all.)
> 
> You could use a TREE_LANG_FLAG, looks like none of them are used on
> RETURN_EXPR.
> 
> > It looks like this warning is raised also by diag_return_locals in
> > gimple-ssa-isolate-paths, should the transformation also be made here?
> 
> Looks like it already is, in warn_return_addr_local:
> 
> >       tree zero = build_zero_cst (TREE_TYPE (val));
> >       gimple_return_set_retval (return_stmt, zero);
> >       update_stmt (return_stmt);
> 
> ...but, weirdly, only with -fisolate-erroneous-paths-*, even though it isn't
> isolating anything.  Perhaps there should be another flag for this.
> 

I see, thanks. From this I've found that my above patch isn't sufficient
anyway, as compiling with -O2 causes the warning to appear twice as the
suppression I did wasn't sufficient. As such I'll exclude this patch
from the next revision since it's not actually necessary for the problem
I was trying to solve, and I'll work on trying to solve this properly
a bit later.

> > I note that the otherwise very similar -Wdangling-pointer warning
> > doesn't do this transformation either, should that also be something I
> > look into fixing here?
> 
> With that same flag, perhaps.  I wonder if it would make sense to remove the
> isolate-paths handling of locals in favor of the dangling-pointer handling?
> I don't know either file much at all.
> 
> Jason
>