Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pointer comparison can be used to drop unrelated execution paths #12

Open
tomsik68 opened this issue Nov 24, 2020 · 0 comments
Open

Pointer comparison can be used to drop unrelated execution paths #12

tomsik68 opened this issue Nov 24, 2020 · 0 comments

Comments

@tomsik68
Copy link
Member

I was investigating results in reachsafety category again and an interesting issue popped up.
Symbiotic version is:

version: 7.0.0-dev
LLVM version: 8.0.1
symbiotic            -> 2e2ceb4735887125c1c81e4194f8cad8eb0409e7 (Release)
dg                   -> ccbc3536de659896c4ecac7f4b619733b14b5b9a (Release)
sbt-slicer           -> 85c8cb482fecdd065329a55551bae93af2408c7b (Release)
sbt-instrumentation  -> 62c481990db07679f3e3e0e086d5c27a6d28f757 (Release)
klee                 -> 236d99208f15e125aa89008c9652965ad89d3005 (Release)

I ran Symbiotic like this: ./symbiotic --no-slice --sv-comp --save-files --prp sv-benchmarks/c/properties/unreach-call.prp ldv_malloc.c and it reported true, but my expected result was false.

ldv_malloc.c:

extern int __VERIFIER_nondet_int();
extern void abort(void);
extern void *malloc(unsigned long);
void reach_error(void) {}

int ldv_is_err(void *ptr) {
    const unsigned long errs = ((unsigned long) -4095);
    unsigned long p = (unsigned long) ptr;
    return p > errs;
}

void assume_abort_if_not(int cond) {
    if (!cond) {
        abort();
    }
}

void *ldv_malloc(unsigned long size) {
    if (__VERIFIER_nondet_int())
        return 0;
    void *p = malloc(size);
    assume_abort_if_not(!ldv_is_err(p));
    return p;
}

int main()
{
    void *a = ldv_malloc(512);
    if (a != 0)
        reach_error();
    free(a);
    return 0;
}

Commenting out the call of assume_abort_if_not solves the problem. I suspect that assume_abort_if_not drops all the execution paths where klee decides not to return 0 from ldv_malloc. The expected behavior is to define constraints on the pointer's numeric value. I believe this is the cause of many incorrect true answers we have in Linux drivers ReachSafety as ldv_is_err is used very often in those benchmarks.

Possibly related to fix for staticafi/symbiotic#174

mchalupa added a commit that referenced this issue Nov 24, 2020
We must use the symbolic address when we compare to a number.

Fixes #12.
lzaoral added a commit to lzaoral/klee that referenced this issue Apr 20, 2021
Fixes:
$ cat test.c
int main(void) {}
$ clang -m32 -emit-llvm -c test.c -o test.bc
$ klee test.bc
 KLEE: output directory is "/home/lukas/klee/klee-out-9"
 KLEE: Using Z3 solver backend
 LLVM ERROR: 64-bit code requested on a subtarget that doesn't support it!
  #0 0x00007fb59688bbf6 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (/lib64/libLLVM-12.so+0xc07bf6)
  staticafi#1 0x00007fb596889ae4 llvm::sys::RunSignalHandlers() (/lib64/libLLVM-12.so+0xc05ae4)
  staticafi#2 0x00007fb596889c69 (/lib64/libLLVM-12.so+0xc05c69)
  staticafi#3 0x00007fb595772310 __restore_rt (/lib64/libc.so.6+0x3d310)
  staticafi#4 0x00007fb595772292 raise (/lib64/libc.so.6+0x3d292)
  staticafi#5 0x00007fb59575b8a4 abort (/lib64/libc.so.6+0x268a4)
  staticafi#6 0x00007fb5967d7b5e llvm::report_fatal_error(llvm::Twine const&, bool) (/lib64/libLLVM-12.so+0xb53b5e)
  staticafi#7 0x00007fb5967d7c8e (/lib64/libLLVM-12.so+0xb53c8e)
  staticafi#8 0x00007fb5990e1b26 (/lib64/libLLVM-12.so+0x345db26)
  staticafi#9 0x00007fb5990e2120 (/lib64/libLLVM-12.so+0x345e120)
 staticafi#10 0x00007fb5990e5ac4 (/lib64/libLLVM-12.so+0x3461ac4)
 staticafi#11 0x00000000004919ed klee::RaiseAsmPass::runOnModule(llvm::Module&) /home/lukas/klee/build/../lib/Module/RaiseAsm.cpp:102:31
 staticafi#12 0x00007fb5969b1d02 llvm::legacy::PassManagerImpl::run(llvm::Module&) (/lib64/libLLVM-12.so+0xd2dd02)
 staticafi#13 0x0000000000486cac klee::KModule::instrument(klee::Interpreter::ModuleOptions const&) /home/lukas/klee/build/../lib/Module/KModule.cpp:237:23
 staticafi#14 0x000000000043562f klee::Executor::setModule(std::vector<std::unique_ptr<llvm::Module, std::default_delete<llvm::Module> >, std::allocator<std::unique_ptr<llvm::Module, std::default_delete<llvm::Module> > > >&, klee::Interpreter::ModuleOptions const&) /home/lukas/klee/build/../lib/Core/Executor.cpp:537:23
 staticafi#15 0x0000000000417dd5 main /home/lukas/klee/build/../tools/klee/main.cpp:1411:46
 staticafi#16 0x00007fb59575cb75 __libc_start_main (/lib64/libc.so.6+0x27b75)
 staticafi#17 0x0000000000424d6e _start (build/bin/klee+0x424d6e)
lzaoral pushed a commit to lzaoral/klee that referenced this issue Oct 4, 2022
This commit is a squash of the following commits changing the same piece of
code so that the tests still pass:

* Fix the comparison of object pointers

  We must use the symbolic address when we compare to a number.

  Fixes staticafi#12.

* Fix comparing to freed pointers

* Fix comparing pointers

* For pointers into the same memory object, compare their offsets,
  do not compare their symbolic addresses.

* Fix comparison of pointers to null

* Fix comparison of pointers for equality

  If we know the segments, we do not need to use the addresses
  (since we do not assume the inequality to other objects it would
  introduce false positives).

* Fix comparing freed pointers

* Freed pointers are usually concrete, so we must make them an exception
  in the code.

* Allow comparing symbolic pointers for (in)equality

  That can be derived without using symbolic addresses.

* Executor: fix handling freed pointers

  There was missing the "else" branch, so we overwrote
  the pointer in the case it was not resolved.

  Fixes staticafi/symbiotic#89
lzaoral pushed a commit to lzaoral/klee that referenced this issue Oct 5, 2022
This commit is a squash of the following commits changing the same piece of
code so that the tests still pass:

* Fix the comparison of object pointers

  We must use the symbolic address when we compare to a number.

  Fixes staticafi#12.

* Fix comparing to freed pointers

* Fix comparing pointers

* For pointers into the same memory object, compare their offsets,
  do not compare their symbolic addresses.

* Fix comparison of pointers to null

* Fix comparison of pointers for equality

  If we know the segments, we do not need to use the addresses
  (since we do not assume the inequality to other objects it would
  introduce false positives).

* Fix comparing freed pointers

* Freed pointers are usually concrete, so we must make them an exception
  in the code.

* Allow comparing symbolic pointers for (in)equality

  That can be derived without using symbolic addresses.

* Executor: fix handling freed pointers

  There was missing the "else" branch, so we overwrote
  the pointer in the case it was not resolved.

  Fixes staticafi/symbiotic#89
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant