Skip to content
This repository has been archived by the owner on Oct 3, 2021. It is now read-only.

Busybox fixes #513

Merged
merged 4 commits into from
Nov 16, 2017
Merged

Busybox fixes #513

merged 4 commits into from
Nov 16, 2017

Conversation

tautschnig
Copy link
Contributor

@tautschnig tautschnig commented Nov 15, 2017

Addresses issues about the busybox benchmarks raised in #444 and #480 and #511.

Copy link
Member

@PhilippWendler PhilippWendler left a comment

Choose a reason for hiding this comment

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

The commit b16e0cd "Busybox: Remove conflicting memory safety property" claims it is about #480, but in fact it targets a different problem. This problem is actually not necessarily a problem: Some files have _false-unreach-call and _true-valid-memsafety, but we have noticed in the past that most programs do not not do a proper memory cleanup before calling __VERIFIER_error, so it is highly probably that these programs have a leak and are not _true-valid-memsafety. This commit now simplify removes all claims about memsafety from these files. I am ok with that. @dbeyer?

The busybox part of #480 is fixed by the other commits of this PR, and #444 and #511 are addressed as well.

Thus, I would approve if the above question is resolved.

@tautschnig
Copy link
Contributor Author

I'm obviously happy to drop b16e0cd from this PR if this is not the way to go.

@dbeyer
Copy link
Member

dbeyer commented Nov 15, 2017

Calling __VERIFIER_error does not cause a __false-valid-memtrack, but a __false-valid-memcleanup.
Thus, it is perfectly fine to have _false-unreach-call and _true-valid-memsafety as label for one file.

@dbeyer dbeyer assigned tautschnig and unassigned dbeyer Nov 15, 2017
@tautschnig
Copy link
Contributor Author

Thus, it is perfectly fine to have _false-unreach-call and _true-valid-memsafety as label for one file.

So check.py is wrong then? Happy to include a fix for this in this PR (and drop the other commit). Please confirm/advise @dbeyer @PhilippWendler.

@PhilippWendler
Copy link
Member

Ok, I think the check is older than the distinction between memsafety and memcleanup. @tautschnig You could drop this commit from the PR, and I will replace the check with a more appropriate one.

The version without undefined behaviour, where additional properties can be
checked, already exists in the repository.
This is a follow-up to d115b6f, where the null-pointer dereference has been
discussed.
@tautschnig
Copy link
Contributor Author

Thanks @PhilippWendler - patch dropped and series rebased.

Copy link
Member

@PhilippWendler PhilippWendler left a comment

Choose a reason for hiding this comment

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

I would be fine with merging it, but the question about abort vs. __VERIFIER_assume could be solved first.

@@ -107,7 +107,7 @@ static void bb_error_msg_and_die(const char *s, ...)
// file ./libbb-dump.i line 1
static void bb_show_usage(void)
{
;
abort();
Copy link
Member

Choose a reason for hiding this comment

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

Related to the discussions in some other PRs: should this be __VERIFIER_assume(false)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would work for the moment, but would cause havoc when someone steps up to add termination analysis/labels to the benchmark?!

Copy link
Member

Choose a reason for hiding this comment

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

I think 'abort' is best: (a) does not have a loop and (b) is a proper C call.

@dbeyer dbeyer merged commit 300bf38 into sosy-lab:master Nov 16, 2017
@tautschnig tautschnig deleted the busybox-fixes branch November 12, 2019 16:24
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

Successfully merging this pull request may close these issues.

3 participants