[cfe-dev] A patch for chroot checker

[Ted Kremenek]

On Sep 14, 2010, at 1:09 AM, 章磊 wrote:

> hi, clang
> 
> This patch try to check improper use of chroot.
> 
> In order to implement this checker, i add a subclass (SymbolEnv) of SymbolData to represent some environment variables. Now it contains only one kind of environment variables(JailKind).Then adds several states to the Jail Symbol.
> 
> This is an experimental checker, and i don't know it is the right way to do this stuff.
> 
> I'll appreciate it if there are any advice about this patch.
> 


Hi Lei,

Thanks for the patch.  One thing that would really help me evaluate it are some comments in the checker that describe what it is trying to do.  Those comments could include code examples that show examples of good and bad behavior.

I think the point I'm most uncertain about is the addition of the SymbolEnv class.  Typically we haven't needed to add new kinds of symbols for specific checkers, so if you could explain it's intended usage that would help me assess whether it is useful or whether something else is more appropriate for your checker.

It looks to me that you are tracking some kind of type state with the JailState class (please include a comment above this class that indicates what it represents), but I'm not certain what value the SymbolEnv symbol serves other than to hang some global typestate in the GDM.  I don't think SymbolEnv is necessary since the following line will always return the same symbol:

  const SymbolEnv* Sym = SymMgr.getEnvironmentSymbol(SymbolEnv::JailKind);

This means that the ImmutableMap in the GDM will always have at most one entry.  It would be much more efficient to store the enum value directly in the GDM if it doesn't need to be associated with any symbol.  Also, the notion of 'JailKind' seems specific to this checker, and shouldn't be in SymbolManager.h.

Minor nit: please don't use tabs (they appear in a few places in your patch).

Cheers,
Ted