Test the library when malloc(0) returns NULL #264
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gilles-peskine-arm
added
enhancement
gilles-peskine-arm
removed
the
needs: preceding PR
|
#257 is merged. Restarting CI. |
Patater
suggested changes
Sep 19, 2019
gilles-peskine-arm
added
needs: work
|
CI reported failures. I'll fix these and also rebase on top of |
gilles-peskine-arm
force-pushed
the
test_malloc_0_null
branch
from
f338f77 to
720e77d
Compare
Add a very basic test of calloc to the selftest program. The selftest program acts in its capacity as a platform compatibility checker rather than in its capacity as a test of the library. The main objective is to report whether calloc returns NULL for a size of 0. Also observe whether a free/alloc sequence returns the address that was just freed and whether a size overflow is properly detected.
gilles-peskine-arm
force-pushed
the
test_malloc_0_null
branch
from
83e6b39 to
6673464
Compare
Patater
added
the
needs: review
gilles-peskine-arm
removed
the
needs: work
|
The remaining CI failures are unrelated to this PR (random DTLS failures, which is a known thing, and "export keys functionality" in |
AndrzejKurek
suggested changes
Sep 23, 2019
| component_test_malloc_0_null () { | ||
| msg "build: malloc(0) returns NULL (ASan+UBSan build)" | ||
| scripts/config.pl full | ||
| scripts/config.pl unset MBEDTLS_MEMORY_BUFFER_ALLOC_C |
There was a problem hiding this comment.
Since Mbed-TLS/mbedtls#2469 you don't have to do that.
There was a problem hiding this comment.
I know, but that's only in mbedtls. The change isn't in mbed-crypto yet and this PR is for mbed-crypto.
AndrzejKurek
previously approved these changes
Sep 23, 2019
Patater
suggested changes
Sep 24, 2019
tests/scripts/all.sh
Outdated
| msg "selftest: malloc(0) returns NULL (ASan+UBSan build)" | ||
| # By default ASan terminates the process if called with a size that's | ||
| # insanely large. We do this deliberately in selftest, so disable this | ||
| # behavior. |
There was a problem hiding this comment.
We don't call calloc with a size that is extremely large anymore, so this comment needs updating.
There was a problem hiding this comment.
I amended the offending commit (which was the last one).
Exercise the library functions with calloc returning NULL for a size
of 0. Make this a separate job with UBSan (and ASan) to detect
places where we try to dereference the result of calloc(0) or to do
things like
buf = calloc(size, 1);
if (buf == NULL && size != 0) return INSUFFICIENT_MEMORY;
memcpy(buf, source, size);
which has undefined behavior when buf is NULL at the memcpy call even
if size is 0.
This is needed because other test components jobs either use the system
malloc which returns non-NULL on Linux and FreeBSD, or the
memory_buffer_alloc malloc which returns NULL but does not give as
useful feedback with ASan (because the whole heap is a single C
object).
gilles-peskine-arm
force-pushed
the
test_malloc_0_null
branch
from
6673464 to
31b0a3c
Compare
Patater
reviewed
Sep 24, 2019
Patater
approved these changes
Sep 24, 2019
AndrzejKurek
approved these changes
Sep 25, 2019
|
CI failures are export keys, to be fixed by other PRs. |
Patater
added
needs: backports
This was referenced Sep 30, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
On our main test platforms (Linux, FreeBSD), malloc(0) returns a non-NULL pointer. With
MBEDTLS_MEMORY_BUFFER_ALLOC_C,calloc(0,_)does return NULL, but memory_buffer_alloc doesn't play well with UBSan. Add a test component with UBSan wherecallocreturns NULL when asked to allocate 0 bytes, wrapping around the platformcalloc.Fix #137: now it's tested.
Should be backported to mbedtls LTS branches.