Hi Ted,<br><div style="margin-left: 40px;"><br>Thank you very much for your reply, next time i will add more comments to explain my intention.<br><br>Comments inline below.<br></div><br><div class="gmail_quote">2010/9/15 Ted Kremenek <span dir="ltr"><<a href="mailto:kremenek@apple.com">kremenek@apple.com</a>></span><br>
<blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><div><div></div><div class="h5"><br>
On Sep 14, 2010, at 1:09 AM, ียภฺ wrote:<br>
<br>
> hi, clang<br>
><br>
> This patch try to check improper use of chroot.<br>
><br>
> 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.<br>

><br>
> This is an experimental checker, and i don't know it is the right way to do this stuff.<br>
><br>
> I'll appreciate it if there are any advice about this patch.<br>
><br>
<br>
<br>
</div></div>Hi Lei,<br>
<br>
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.<br>

<br></blockquote><div><br>What i am trying to do is to check CWE-243: Failure to Change Working Directory in chroot Jail. Because The chroot() function call does not change the process's current working directory, so in order to properly invoke chroo() one should call chdir("/") after a call to chroot() before other function that may break the chroot Jail.<br>
<br>Coverity Prevent says "To fix this defect, immediately call chdir("/") after a call to chroot()". IMO this may generate many false positives, but i can't figure out a better way to do this. So this checker's behavior is similar to what Coverity does:<br>
<ol><li>After the call to chroot(), there shouldn't be any function call before calling chdir("/");</li><li>but chroot() and chdir() are OK.<br></li></ol><br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">

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.<br>

<br></blockquote><div><br>I didn't mean to add new kinds of symbols for specific checkers, it's just because i don't know which Symbol is proper to represent the chroot jail (may be some other process's environment variables). I think it's process related, and different from normal variables.<br>
 </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
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:<br>

<br>
  const SymbolEnv* Sym = SymMgr.getEnvironmentSymbol(SymbolEnv::JailKind);<br>
<br>
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.<br>
</blockquote><div><br>Because i think it's process related, i want SymbolEnv to have only one object with JailKind. So i only profiles <br> EnvKind, and make both getEnvironMentSymbol() to return the same symbol.<br>If i should store the enum value directly in the GDM, how?<br>
<br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<br>
Minor nit: please don't use tabs (they appear in a few places in your patch).<br></blockquote><div><br>Sorry, but how to check this?<br> </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">

<br>
Cheers,<br>
<font color="#888888">Ted</font></blockquote></div><br><br clear="all"><br>-- <br>Best regards!<br><br>Lei Zhang<br>