Memory primitives should handle invalid pointers #6045
Comments
feliperodri
added
bug
aws
|
cc. @SaswatPadhi |
|
In fact, why does cbmc return |
|
Quoting from https://stackoverflow.com/a/2022402:
So, I think there are two issues raised by this bug report:
Here is a clear soundness bug, even without using $ cat test.c
#include <stdlib.h>
#include <assert.h>
int main() {
char *a = malloc(0);
char *b = malloc(0);
__CPROVER_assume(a != NULL && b != NULL);
assert(a != b);
}
$ cbmc --malloc-may-fail --malloc-fail-null --pointer-check test.c | tail
** Results:
/Users/saspadhi/test.c function main
[main.assertion.1] line 10 assertion a != b: SUCCESS
<builtin-library-malloc> function malloc
[malloc.assertion.1] line 26 max allocation size exceeded: SUCCESS
[malloc.assertion.2] line 31 max allocation may fail: SUCCESS
** 0 of 3 failed (1 iterations)
VERIFICATION SUCCESSFULAccording to C standard, this should fail because
|
SaswatPadhi
changed the title
malloc(0)
|
[...]
See http://cprover.diffblue.com/memory-primitives.html.
Why not?
[...]
I don't think "According to the C standard, this should fail" is correct. The C standard says that the behaviour is implementation-defined. If, however, we want to simulate all permitted implementations, then, yes, we would also have to consider the case where multiple calls to
Why? |
|
Hi Michael,
You're right, I should have said, "according to the C standard, this could fail." Are we currently verifying all properties assuming a particular compiler implementation?
I don't know enough about
I suggested this to "simulate all platforms", as you mentioned. The standard says it's implementation-defined, so on an arbitrary implementation it would be a non-deterministic pointer. |
|
[...]
We do honor the system compiler at times, but here it's not just the compiler: also the C library matters. And, no, we don't consistently consider all possible cases or all the specifics of an existing implementation. We should strive to fix this and make sure all such aspects are configurable.
Yes:
But a non-deterministic pointer would include pointers to existing objects, resulting dereferencing suddenly becoming valid for such cases. So, yes, our modeling of |
|
Thanks a lot for the detailed explanation.
Ah yes, so perhaps a non-deterministic pointer that's invalid, i.e. either
May be @feliperodri can comment on this. He was debugging some s2n issue. (I was just digging deep into the bug report.) |
|
@tautschnig The problem is that checking for nullness is not enough to make sure a pointer is valid. Take a look at the following example: // main.c
#include <stdlib.h>
#include <assert.h>
int main() {
size_t len;
void *ptr = malloc(len);
__CPROVER_assume(ptr != NULL);
assert(__CPROVER_r_ok(ptr, len));
}I'd expect this assertion to fail, because for Also, I still don't think we're modelling malloc correctly for the case
Therefore, I expect two things:
For a more concrete example, take a look at the proof harness for s2n_hash_block_size. If we change the allocation to use a non-deterministic integer, this proof will always fail for the case when |
|
This might not be an urgent problem. There are two important remarks here:
Command line: 1 // main.c
2 #include <stdlib.h>
3 #include <assert.h>
4
5 int main() {
6 size_t len;
7 size_t *ptr = malloc(len);
8 __CPROVER_assume(ptr != NULL);
9 *ptr = len;
10 }CBMC indicates there is a "pointer outside dynamic object bounds" violation in line 9. See the output: Thus, we will get an error either way, but I still think these are wrong: |
|
@feliperodri Thank you for your further clarification! Perhaps the main point we need to work out is what exactly an "invalid" pointer is? Here are my thoughts:
What is your definition of an "invalid" pointer? As the standard prescribes that "the behavior is as if the size were some nonzero value" the pointer returned by a call
I agree with this statement, except the definition of "invalid pointer" is to be clarified. |
@tautschnig Precisely! Take a look at the next example. Command line: // main.c
#include <assert.h>
int main() {
int *ptr;
assert(__CPROVER_r_ok(ptr, 0));
}In this case, CBMC considers that What does |
|
CBMC's encoding of pointers, at present, has three groups of object: the Such a pointer cannot be |
|
@tautschnig thank you for the clarification.
@SaswatPadhi any thoughts? |
|
In addition to the MEM04-C page, which Felipe mentioned above, I was also looking at these slides [http://index-of.es/Exploit/HES10-jvanegue_zero-allocations.pdf], and they discuss various verification approaches, but I think the most relevant bits are in slides 24 -- 35.
This was exactly my confusion as well. Michael pointed me to http://cprover.diffblue.com/memory-primitives.html, which states:
So basically, unless I am still not clear about the difference between $ cat test.c
#include <assert.h>
int main() {
int *ptr;
assert(__CPROVER_r_ok(ptr, 0));
}
$ cbmc --pointer-check --trace test.c
[...]
Violated property:
file /Users/saspadhi/test.c function main line 5 thread 0
assertion __CPROVER_r_ok(ptr, 0)
!((signed long int)(signed long int)!((FALSE || !(POINTER_OBJECT((const void *)ptr) == POINTER_OBJECT(NULL))) &&
!IS_INVALID_POINTER((const void *)ptr) &&
(FALSE || !(POINTER_OBJECT((const void *)ptr) == POINTER_OBJECT(__CPROVER_deallocated))) && (FALSE || !(POINTER_OBJECT((const void *)ptr) == POINTER_OBJECT(__CPROVER_dead_object))) &&
(FALSE || (IS_DYNAMIC_OBJECT((const void *)ptr) ==> !(POINTER_OFFSET((const void *)ptr) < 0l || (__CPROVER_size_t)POINTER_OFFSET((const void *)ptr) + (__CPROVER_size_t)0 > OBJECT_SIZE((const void *)ptr)))) &&
(FALSE || (!IS_DYNAMIC_OBJECT((const void *)ptr) ==> !(POINTER_OFFSET((const void *)ptr) < 0l || (__CPROVER_size_t)POINTER_OFFSET((const void *)ptr) + (__CPROVER_size_t)0 > OBJECT_SIZE((const void *)ptr)))) &&
(POINTER_OBJECT(NULL) == POINTER_OBJECT((const void *)ptr) && NULL != (const void *)ptr ==> FALSE)) != 0l)
** 1 of 1 failed (2 iterations)
VERIFICATION FAILED |
|
|
feliperodri
changed the title
malloc(0)
|
Sorry to come late to this party. I have a different expectation of behavior here: I expect the example @feliperodri gives back at the start of this discussion to pass. In general, I expect the following to pass: If this sort of thing doesn't pass it will add needless complication to the contracts of any function that takes a pointer and a size where there size is allowed to be 0 (and presumably in that case the function does nothing). memcpy type functions for example. If the size is 0 and p is not NULL then we have a pointer that is not valid to access. I would expect the predicate __CPROVER_r_ok(p,0) to be vacuously true for such pointers since we are saying we need accessibility only for 0 bytes. If it doesn't work like this I'm going to have to write (size == 0 || __CPROVER_r_ok(p,size)) everywhere, and that just adds clutter. Now, ok, I know that __CPROVER_r_ok doesn't currently work in assumptions, but we want that fixed too. |
|
Continuing to party late... I don't share @SaswatPadhi's expectation that the value returned by malloc(0) could be pretty much anything. The man page for malloc says that if malloc(0) returns a non-null pointer then you may not access that pointer. But... it says explicitly that you may free that pointer. That tells you a lot. The following code should be a no-op; If malloc(0) could return arbitrary pointers this could result in freeing other chunks of memory. @SaswatPadhi gives an example above, which passes, but he expects to fail (because "According to C standard, this should fail because a and b could actually be the same location"): I expect this to pass. Note that malloc isn't defined by the C language, but rather by the standard library. Here is what the Linux man page (standard library documentation) says about malloc:
Similarly, the OpenBSD man page says this:
Since the non-null returned values are required to be unique this code should pass. |
|
Actually the ISO C standard does talk about
(Source: http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2310.pdf; section 7.22.3; page 256) Since the standard says that the behavior is "implementation-defined", Linux and BSD are not wrong in additionally requiring the uniqueness property, but my point was that it's not part of the ISO C standard. In other words, an ISO C compliant compiler on some (not-so-sane) platform could return non-unique pointers on |
|
Hi Saswat. Thanks for the correction. You are quite right the standard also covers these standard library function. But, my reading of the standard is that malloc(0) may return NULL or a pointer (implementation choice), but that if it returns a pointer it must be unique (distinct form other currently live malloced pointers). Why do I think it says that? As the quoted text says, you can either return NULL, or you can act like it was a malloc of some non-zero size (that didn't return null) (except that you can't access through the returned pointer). What does the standard say about non-null values returned by malloc for nonzero sizes? It says "Each such allocation shall yield a pointer to an object disjoint from any other object.". Of course, this is English so it is still open to interpretation. Would we consider two pointers to the same object to be disjoint if the size of the object were 0? Well, remembering that "the behavior is as if the size were some nonzero value" I am going to say no, I don't think we are supposed to consider them disjoint. When the Linux and BSD man pages say that they will return NULL or a unique pointer, I don't think they are saying anything different to the standard, I read them as saying the same thing differently, a clarification rather than a narrowing. This seems to be the common interpretation. For example, the Solaris malloc man says: "If size, nelem, or elsize is 0, the allocation functions return a unique non-null pointer that can be passed to free()" MacOS malloc says nothing explicit about behavior when size is 0 that I can see, sadly, while the Windows malloc manual says something much more explicit about allocating 0 size things on the heap, which seems to imply that the results will be unique. Suppose you don't buy my line of reasoning above, try this one instead: Another way to to look it is is this. Since the C standard says that if malloc(0) returns a non-null value then the value is like that of a non-zero sized malloc (except you can't access it). And, since not being able to access through said pointer is the only thing it says is different about the result of non-null zero sized malloc vs a non-null non-zero sized malloc then I think that says that you can free it. This is something that the various unix malloc man pages say explicitly. I don't think they are narrowing the standard here, just clarifying. So, what does that entail? To me it means that I expect this to be OK: But, if this is ok it would imply that when p and q are both non-null they are also distinct, otherwise we'd be calling free twice on the same pointer, which the C standard says is undefined:
So, I think that in this way the C standard also implicitly requires that non-null values returned by malloc be unique (regardless of the size malloced). |
|
Thanks a lot for the detailed explanation! Thinking more about this part,
your reasoning makes a lot of sense. |
|
I think maybe we can close this. Are you ok with that @SaswatPadhi @feliperodri @tautschnig ? |
CBMC version: 5.27.0 (cbmc-5.26.0-148-g7ced011bd-dirty)
Operating system: macOS Mojave 10.14.6
Exact command line resulting in the issue:
cbmc main.c --malloc-may-fail --malloc-fail-null --traceWhat behaviour did you expect:
line 7 assertion __CPROVER_r_ok(ptr, 0): FAILUREWhat happened instead:
VERIFICATION SUCCESSFULTo explain the behavior, let's consider the following example.
I expect the assertion to fail, because
ptrcould be an invalid pointer; however, it succeeds.I expect that the first thing
__CPROVER_r_okwould check is whether the pointer is valid or not. If not valid, return false.I also try to verify this program using
--pointer-primitive-checkflag, but I got another unexpected behavior.Command line:
cbmc main.c --malloc-may-fail --malloc-fail-null --pointer-primitive-check --traceAgain, if cbmc checked for validity first, I think we'd also avoid this problem.
The text was updated successfully, but these errors were encountered: