[cfe-dev] [analyzer] Zombie symbols.

[Artem Dergachev via cfe-dev]

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.