[cfe-dev] [analyzer] Zombie symbols.

[Yury Gribov via cfe-dev]

Anna, Devin,

Could you comment on this? It sounds like an important design issue.

On 03/14/2016 03:55 PM, Artem Dergachev via cfe-dev wrote:
> A certain peculiar property of the removeDeadBindings() mechanism seems
> to have been overlooked since quite a long time.
>
> My colleague Ilya Palachev has recently shown me a specific leak
> false-negative. It could be simplified down to the following fairly
> short SimpleStreamChecker test that currently fails:
>
>
>    void test() {
>      FILE *fp = fopen("myfile.txt", "w");
>      fp = 0; // Hmm, we should have overwritten the symbol.
>    } // expected-warning{{Opened file is never closed; potential
> resource leak}}
>
>
> So what's going on? In fact, it is not quite correct to say that
> "checkDeadSymbols() is  called for symbols that are no longer available
> in the program state". It is just the opposite: it is only called for
> symbols that *are* available in the program state, otherwise how does it
> know that they even exist!
>
> In order to die, the symbol needs to be marked as dead on one of the
> removeDeadBindings() runs, and also never marked as live during such run
> (otherwise the "live" mark takes a higher priority). Such marks can be
> added by Environment (which marks values of active expressions as live
> and values of inactive expressions as dead), Store (which runs a
> worklist to find live keys and mark their values live, and then find
> more live keys, etc.), Constraint Manager (which simply marks all of its
> symbols as dead, as he is not the one to be interested in keeping them
> alive), or checkers (through checkLiveSymbols() callback).
>
> So in the program state before the assignment, the file descriptor is
> live, because the store marks it as live, because it is a value stored
> in a region of a live variable.
>
> And in the state after the assignment, the file descriptor is not
> present at all, because the checker fails to inform SymbolReaper of this
> symbol's existence in its checkLiveSymbols() callback, and there are no
> other sources of such information for the SymbolReaper.
>
> And the assignment itself doesn't instantly trigger the dead symbol
> removal. It is only triggered once in a while automatically "between"
> the statements, and the kind of the statement is of little concern.
>
>
> SimpleStreamChecker could be fixed with the following callback:
>
>
>    void SimpleStreamChecker::checkLiveSymbols(ProgramStateRef State,
>                                               SymbolReaper &SymReaper)
> const {
>      StreamMapTy TrackedStreams = State->get<StreamMap>();
>      for (StreamMapTy::iterator I = TrackedStreams.begin(),
>                                 E = TrackedStreams.end(); I != E; ++I) {
>        SymbolRef Sym = I->first;
>        SymReaper.maybeDead(Sym);
>      }
>    }
>
>
> However, even if i propose this fix, somebody has to, kind of, update
> the presentation of writing the checker in 24 hours, and explain all the
> stuff from this mail in the process, and i guess the checker would no
> more be as simple as it used to be.
>
> A few other checkers are not suffering from this issue because another
> entity performs the maybeDead() call for them: for example,
> MallocChecker adds default bindings on their symbol's symregions, and
> leaves it to the Store to mark the base symbols as dead. However, i
> guess they shouldn't rely on the Store for doing this job.
>
> So i've got a few ideas:
>
> 0. Fix all checkers as discussed above.
> 1. Scan all symbols in the SymbolManager to find zombie symbols,
> deprecate the maybeDead() mechanism.
> 2. Wait until some kind of smart data map takes care of this extra work.
> 3. Call checkDeadSymbols on exceptional occasions, like bindings and
> invalidations, explicitly marking the relevant symbols as maybe dead.
>
> 1. and 2. sound most reasonable to me (3. is hacky and we cannot expect
> to cover all the cases and probably code duplication). Not sure about
> performance of 1., maybe i'd have a look, but it'd most likely simplify
> some code. Otherwise i guess we'd have to just 2., unless anybody has
> anything else in mind or i'm missing something(?)
>
> Best regards,
> Artem.
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev