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

Fix resource leaks from the POSIX runtime #16

Draft
wants to merge 2 commits into
base: 9.0.0-dev
Choose a base branch
from

Conversation

lzaoral
Copy link

@lzaoral lzaoral commented Apr 26, 2022

Otherwise, memsafety would always fail due to leaked memory.

Otherwise, memsafety would always fail due to leaked memory.
@lzaoral lzaoral added the bug label Apr 26, 2022
@lzaoral lzaoral self-assigned this Apr 26, 2022
This is not the right way how to it because with this change, KLEE creates
a symbolic size array of symbolic size arrays of symbolic bytes.  Which is
way too time and resource consuming.
@lzaoral lzaoral requested review from mchalupa and trtikm May 9, 2022 12:08
@@ -43,6 +43,7 @@ LLVMCC.Flags += $(LLVMCC.ExtraFlags) \
-I@CMAKE_BINARY_DIR@/include \
-emit-llvm \
-std=gnu89 \
-g \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this change needed? If it is important for functioning, it would be nice to have it in a separate commit for the documentation purposes.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll move it to a separate commit (and send a similar PR to upstream). It's useful so that all reports in these files are not missing location info.

klee_destroy_fds();

char **ptr;
for (ptr = argvPtr; *ptr; ptr++) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why to bother with freeing the arguments if they are made global for KLEE?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My local copy of KLEE with POSIX runtime enabled was still detecting those leaks.

Copy link
Member

@mchalupa mchalupa May 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds like a bug in the leak detection inside KLEE to me. I would expect KLEE not to report global memory as leaked.

ie = rl.end(); it != ie; ++it) {
const MemoryObject *mo = it->first.first;
assert(!mo->isLocal);
mo->isGlobal = false;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When 'mo' is neither global nor local, what kind of variable it is?

@@ -511,6 +511,10 @@ void klee_mark_global(void *object) {
;
}

void klee_unmark_global(void *object) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why we need this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

klee-replay takes a generated test, creates an executable from it and runs it. This is needed to make the executable compile.

char *s = malloc(numChars+1);
int argSize = klee_range(0, maxSize + 1, "argSize");

char *s = malloc(argSize + 1);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if s==NULL?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The allocation in KLEE never fails, so this should be ok (as long as it is linked to bitcode by KLEE and not by Symbiotic).

@lzaoral
Copy link
Author

lzaoral commented May 9, 2022

If #16 (comment) is indeed true, this PR, in general, is redundant work. I'll convert to a Draft for now.

@lzaoral lzaoral marked this pull request as draft May 9, 2022 15:51
@lzaoral lzaoral removed their assignment Feb 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants