xiw / stack

A static checker for identifying unstable code.
http://css.csail.mit.edu/stack/
Other
359 stars 56 forks source link

Handling of C++ new and delete #39

Open devreal opened 8 years ago

devreal commented 8 years ago

I just tested your STACK tool with success as I found two bugs in our code base. Great work, many thanks! However, I came across a load of warnings that stem from C++ delete and delete[] operators, reported if the pointer to the deleted object was referenced before. The code:

int main()
{
   char *ptr = new char[128];
   *ptr = '\0';
   delete[] ptr;

   return 0;
}

produces the warning:


---
bug: anti-simplify
model: |
  %4 = icmp eq i8* %1, null, !dbg !15
  -->  false
stack:
  - /home/p/src/test/memory_test.cc:5:0
ncore: 1
core:
  - /home/p/src/test/memory_test.cc:4:0
    - null pointer dereference

This warning seems to be unnecessary since (I) ptr should never be NULL since new is supposed to throw an exception if the allocation fails and (II) delete should check for NULL values. Of course the situation is different for C malloc calls. Is there any reason why these warnings are included in your analysis of C++ codes? Is my understanding of the standard incorrect here?

zeldovich commented 8 years ago

I seem to recall that when Clang compiles a "delete" statement, it generates LLVM bytecode to check for whether the pointer being deleted is null (and if so, to not delete the object). Since STACK operates on the LLVM bytecode, it flags this "if" statement as dead code. Of course, the programmer didn't write this "if" statement, so we shouldn't be flagging it. But we couldn't find a convenient way of detecting whether this LLVM bytecode comparison was generated by Clang internally, or whether it corresponds to actual code written by the programmer (short of modifying Clang to emit special annotations for this delete check).

xiw commented 8 years ago

@devreal glad it helped. may i ask which code base is it?

@zeldovich we tried several approaches and settled on using basic block names as a simple and effective heuristics. This is implemented in src/SimplifyDelete.cc.

However, in order to make it work well, you will need to build clang yourself. See the first entry of the FAQ file (or let us know if the latest clang doesn't emit blocks with such names).

zeldovich commented 8 years ago

Oh, that's right, the default build of Clang doesn't put in those basic block names.. Forgot all about that.

devreal commented 8 years ago

Thanks for your quick feedback and sorry for not checking the FAQ beforehand. I checked and do not see these false positives with the self-built versions. It is not clear to me which of the configure switches used in the INSTALL description actually triggers the addition of the block names, which are not there in the versions of LLVM and Clang that are shipped with my.