[PATCH] ChrootChecker: Bind chroot's result zero and reduce verbose warning

[Hiroo MATSUMOTO]

Hi, Jordan


As you say, this patch negatively effects other checker's behavior
like missing malloc free.
I'll try ways of SimpleStreamChecker and MallocChecker.


Regards.
Hiroo MATSUMOTO



2014-03-04 11:28 GMT+09:00 Jordan Rose <jordan_rose at apple.com>:

> I don't think we want to assume the chroot always succeeds, and we
> definitely don't want to stop processing if there's an error (because there
> could be unrelated mistakes). Instead, we should check when we later access
> the chroot-state that it hasn't already failed.
>
> That is, this test case should still warn:
>
> int *buf = malloc(4);
> if (chroot("/") < 0)
>         return; // forgot to free buf!
>
> There's precedent for this in SimpleStreamChecker, which checks if a
> symbol has been constrained to null before declaring that it's leaked, and
> in MallocChecker, which tracks realloc failures to warn that the original
> buffer is still live. Neither of those are quite what you'd want here, but
> they're a good start.
>
> This checker isn't written very well anyway (for example, using FindGDM
> and addGDM directly instead of going through ProgramState's get and set).
> If you're interested, cleaning it up would be a nice improvement.
>
> Thanks for pointing out the errors. If you're not interested in solving
> them yourself right now, please file a bug at http://llvm.org/bugs/.
>
> Jordan
>
>
> On Mar 3, 2014, at 3:19 , Hiroo MATSUMOTO <hiroom2.mail at gmail.com> wrote:
>
> > ChrootChecker tracks a chroot failed case. It will generate warning
> > even though chroot is used properly.
> >
> > When finding improper using chroot, ChrootChecker doesn't stop
> > tracking. It will generate verbose warning.
> >
> > For example, ChrootChecker will generate warnings from below code
> > which can switch proper using and improper using with IMPROPER_USE.
> >
> > When IMPROPER_USE is not defined, 1 warning will be generated.
> > When IMPROPER_USE is defined, 3 warnings will be generated.
> >
> >
> > #include <stdio.h>
> > #include <stdlib.h>
> > #include <unistd.h>
> >
> > int main(int argc, char *argv[])
> > {
> >   if (argc < 2) {
> >       fprintf(stderr, "usage: %s newroot\n", argv[0]);
> >       return 1;
> >   }
> >
> >   if (chroot(argv[1]) < 0) {
> >       perror("chroot"); /** proper using and improper using */
> >       return 1;
> >   }
> >
> > #ifndef IMPROPER_USE
> >   if (chdir("/") < 0) {
> >       perror("chdir");
> >       return 1;
> >   }
> > #endif
> >
> >   if (execv("/bin/sh", argv) < 0) { /** improper using */
> >       perror("execv"); /** improper using */
> >       return 1;
> >   }
> >
> >   return 0;
> > }
> >
> >
> > This patch will bind return value of chroot to zero. And this patch
> > will stop tracking when finding improper using chroot.
> >
> >
> > Index: lib/StaticAnalyzer/Checkers/ChrootChecker.cpp
> > ===================================================================
> > --- lib/StaticAnalyzer/Checkers/ChrootChecker.cpp     (revision 202679)
> > +++ lib/StaticAnalyzer/Checkers/ChrootChecker.cpp     (working copy)
> > @@ -87,11 +87,13 @@
> >  void ChrootChecker::Chroot(CheckerContext &C, const CallExpr *CE) const
> {
> >    ProgramStateRef state = C.getState();
> >    ProgramStateManager &Mgr = state->getStateManager();
> > +  SValBuilder &svalBuilder = C.getSValBuilder();
> > +  SVal success =
> svalBuilder.makeZeroVal(svalBuilder.getContext().IntTy);
> >
> >    // Once encouter a chroot(), set the enum value ROOT_CHANGED directly
> in
> >    // the GDM.
> >    state = Mgr.addGDM(state, ChrootChecker::getTag(), (void*)
> ROOT_CHANGED);
> > -  C.addTransition(state);
> > +  C.addTransition(state->BindExpr(CE, C.getLocationContext(), success));
> >  }
> >
> >  void ChrootChecker::Chdir(CheckerContext &C, const CallExpr *CE) const {
> > @@ -140,7 +142,7 @@
> >    void *const* k = C.getState()->FindGDM(ChrootChecker::getTag());
> >    if (k)
> >      if (isRootChanged((intptr_t) *k))
> > -      if (ExplodedNode *N = C.addTransition()) {
> > +      if (ExplodedNode *N = C.generateSink()) {
> >          if (!BT_BreakJail)
> >            BT_BreakJail.reset(new BuiltinBug(
> >                this, "Break out of jail", "No call of chdir(\"/\")
> immediately "
> >
> > _______________________________________________
> > cfe-commits mailing list
> > cfe-commits at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140304/4d87939e/attachment.html>