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 <[email protected]> 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
> [email protected]
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
