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

Jordan Rose jordan_rose at apple.com
Mon Mar 3 18:28:14 PST 2014


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




More information about the cfe-commits mailing list