https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104979
--- Comment #1 from CVS Commits <cvs-commit at gcc dot gnu.org> --- The master branch has been updated by David Malcolm <dmalc...@gcc.gnu.org>: https://gcc.gnu.org/g:4cebae0924248beb2077894c6dc725c306fc0a69 commit r12-7790-g4cebae0924248beb2077894c6dc725c306fc0a69 Author: David Malcolm <dmalc...@redhat.com> Date: Wed Mar 23 17:40:29 2022 -0400 analyzer: fix accessing wrong stack frame on interprocedural return [PR104979] PR analyzer/104979 reports a leak false positive when handling an interprocedural return to a caller: LHS = CALL(ARGS); where the LHS is a certain non-trivial compound expression. The root cause is that parts of the LHS were being erroneously evaluated with respect to the stack frame of the called function, rather than tha of the caller. When LHS contained a local variable within the caller as part of certain nested expressions, this local variable was looked for within the called frame, rather than that of the caller. This lookup in the wrong stack frame led to the local variable being treated as uninitialized, and thus the write to LHS was considered as writing to a garbage location, leading to the return value being lost, and thus being considered as a leak. The region_model code uses the analyzer's path_var class to try to extend the tree type with stack depth information. Based on the above, I think that the path_var class is fundamentally broken, but it's used in a few other places in the analyzer, so I don't want to rip it out until the next stage 1. In the meantime, this patch reworks how region_model::pop_frame works so that the destination region for an interprocedural return value is computed after the frame is popped, so that the region_model has the stack frame for the *caller* at that point. Doing so fixes the issue. I attempted a more ambitious fix which moved the storing of the return svalue into the destination region from region_model::pop_region into region_model::update_for_return_gcall, with pop_frame returning the return svalue. Unfortunately, this regressed g++.dg/analyzer/pr93212.C, which returns a pointer into a stale frame. unbind_region_and_descendents and poison_any_pointers_to_descendents are only set up to poison regions with bindings into the stale frame, not individual svalues, and updating that became more invasive than I'm comfortable with in stage 4. The patch also adds assertions to verify that we have the correct function when looking up locals/SSA names in a stack frame. There doesn't seem to be a general-purpose way to get at the function of an SSA name, so the assertions go from SSA name to def-stmt to basic_block, and from there use the analyzer's supergraph to get the function from the basic_block. If there's a simpler way to do this, please let me know. gcc/analyzer/ChangeLog: PR analyzer/104979 * engine.cc (impl_run_checkers): Create the engine after the supergraph, and pass the supergraph to the engine. * region-model.cc (region_model::get_lvalue_1): Pass ctxt to frame_region::get_region_for_local. (region_model::update_for_return_gcall): Pass the lvalue for the result to pop_frame as a tree, rather than as a region. (region_model::pop_frame): Update for above change, determining the destination region after the frame is popped and thus with respect to the caller frame rather than the called frame. Likewise, set the value of the region to the return value after the frame is popped. (engine::engine): Add supergraph pointer. (selftest::test_stack_frames): Set the DECL_CONTECT of PARM_DECLs. (selftest::test_get_representative_path_var): Likewise. (selftest::test_state_merging): Likewise. * region-model.h (region_model::pop_frame): Convert first param from a const region * to a tree. (engine::engine): Add param "sg". (engine::m_sg): New field. * region.cc: Include "analyzer/sm.h" and "analyzer/program-state.h". (frame_region::get_region_for_local): Add "ctxt" param. Add assertions that VAR_DECLs are locals, and that expr is for the correct function. * region.h (frame_region::get_region_for_local): Add "ctxt" param. gcc/testsuite/ChangeLog: PR analyzer/104979 * gcc.dg/analyzer/boxed-malloc-1-29.c: Deleted test, moving the now fixed test_29 to... * gcc.dg/analyzer/boxed-malloc-1.c: ...here. * gcc.dg/analyzer/stale-frame-1.c: Add test coverage. Signed-off-by: David Malcolm <dmalc...@redhat.com>
