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

Multiple memory handling fixes #9759

Merged
merged 2 commits into from Feb 20, 2019

Conversation

@michalpasztamobica
Copy link
Contributor

commented Feb 19, 2019

Description

Based on valgrind reports running on unit tests a couple of changes were introduced.
Some unit tests and stubs were not deleting allocated objects.
Production code:

  • TLSSocketWrapper frees allocated cert buffer in case of error from mbedtls,
  • nsapi_addr has a mem_init() function, initializing all of its memory.

Pull request type

[x] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

@SeppoTakalo
@JuhPuur
@VeijoPesonen
@mtomczykmobica
@tymoteuszblochmobica

features/netsocket/nsapi_types.h Outdated
@@ -190,6 +190,37 @@ typedef struct nsapi_addr {
* The raw bytes of the IP address stored in big-endian format
*/
uint8_t bytes[NSAPI_IP_BYTES];

/** Default constructor */
nsapi_addr() :

This comment has been minimized.

Copy link
@SeppoTakalo

SeppoTakalo Feb 19, 2019

Contributor

If you recompile, you might notice that nsapi_types.h is also included in C files. So this needs to be pure C99.

This comment has been minimized.

Copy link
@michalpasztamobica

michalpasztamobica Feb 19, 2019

Author Contributor

Thanks for pointing this out, @SeppoTakalo .
Any idea how to make sure a C structure will always be initialized? I was trying to avoid initializing every instance of nsapi_addr we have in our code...

@ciarmcom ciarmcom requested review from JuhPuur, mtomczykmobica, SeppoTakalo, tymoteuszblochmobica, VeijoPesonen and ARMmbed/mbed-os-maintainers Feb 19, 2019

@ciarmcom

This comment has been minimized.

@VeijoPesonen
Copy link
Contributor

left a comment

In the future test code, implementation and cosmetic changes could be split into separate commits for readability.

@michalpasztamobica michalpasztamobica force-pushed the michalpasztamobica:memory_handling_fixes branch Feb 20, 2019

@michalpasztamobica

This comment has been minimized.

Copy link
Contributor Author

commented Feb 20, 2019

@SeppoTakalo , I moved the nsapi_add initialization one level up - into SocketAddress. Here I added a function to ensure that the whole structure gets initialized properly and this also silenced the valgrind errors.
@VeijoPesonen , split the code into two commits, as you suggested.

@michalpasztamobica michalpasztamobica force-pushed the michalpasztamobica:memory_handling_fixes branch Feb 20, 2019

Multiple memory handling fixes
Correct memory clean-ups in multiple tests and a stub.
@@ -130,12 +132,16 @@ nsapi_error_t TLSSocketWrapper::set_client_cert_key(const void *client_cert, siz
if ((ret = mbedtls_x509_crt_parse(crt, static_cast<const unsigned char *>(client_cert),
client_cert_len)) != 0) {
print_mbedtls_error("mbedtls_x509_crt_parse", ret);
mbedtls_x509_crt_free(crt);

This comment has been minimized.

Copy link
@VeijoPesonen

VeijoPesonen Feb 20, 2019

Contributor

without knowing what mbedtls_x509_crt_free() does the name implies, for me, that it would delete the crt... But that just me. Take this comment just as me thinking out loud

This comment has been minimized.

Copy link
@michalpasztamobica

michalpasztamobica Feb 20, 2019

Author Contributor

In a way I agree with you, but if you look close the logic of our code was:

  1. crt = new;
  2. mbedtls_x509_crt_init
  3. mbedtls_x509_crt_free
    so where is the delete corresponding to what we allocated in step 1? :)

The implementation of mbedtls_x509_crt_free doesn't make things easier, but in the end I think it deallocates everything aside from the poitner passed to it itself...:

    do
    {
        cert_prv = cert_cur;
        cert_cur = cert_cur->next;

        mbedtls_platform_zeroize( cert_prv, sizeof( mbedtls_x509_crt ) );
        if( cert_prv != crt )
            mbedtls_free( cert_prv );
    }

I ran a greentea TLS testsuite and it passed fine throwing no double free exceptions, so I guess we're doing the right thing in the end.
But thanks for staying alert ;)

This comment has been minimized.

Copy link
@SeppoTakalo

SeppoTakalo Feb 20, 2019

Contributor

It frees all the storage/data that Mbed TLS have allocated for it.
But it does not free the storage of the structure itself, which can be statically allocated.

Therefore we need to call delete on the storage.

This comment has been minimized.

Copy link
@SeppoTakalo

SeppoTakalo Feb 20, 2019

Contributor

Basically this is clear, if you look the API pair.

/**
 * \brief          Initialize a certificate (chain)
 *
 * \param crt      Certificate chain to initialize
 */
void mbedtls_x509_crt_init( mbedtls_x509_crt *crt );

/**
 * \brief          Unallocate all certificate data
 *
 * \param crt      Certificate chain to free
 */
void mbedtls_x509_crt_free( mbedtls_x509_crt *crt );

Both take pointer to the storage.
So the TLS cannot (should not) free something it has not allocated. This is common API design.
Ownership of the pointer does not transfer unless clearly specified.

@michalpasztamobica

This comment has been minimized.

Copy link
Contributor Author

commented Feb 20, 2019

Please, don't merge yet. I just noticed that where GCC gives me a warning ARM is failing to compile...

@0xc0170 0xc0170 added needs: work and removed needs: review labels Feb 20, 2019

@michalpasztamobica michalpasztamobica force-pushed the michalpasztamobica:memory_handling_fixes branch Feb 20, 2019

Show resolved Hide resolved features/netsocket/SocketAddress.h Outdated
Multiple memory handling fixes
Based on valgrind reports running on unit tests following changes were introduced:
* TLSSocketWrapper frees allocated cert buffer in case of errors from mbedtls,
* nsapi_addr has a mem_init() function, initializing all of its memory during construction.

@michalpasztamobica michalpasztamobica force-pushed the michalpasztamobica:memory_handling_fixes branch to fa6a3f5 Feb 20, 2019

@SeppoTakalo
Copy link
Contributor

left a comment

Looks Good.

@michalpasztamobica

This comment has been minimized.

Copy link
Contributor Author

commented Feb 20, 2019

@0xc0170 this should now be good to go.
I had to replace

_addr = {NSAPI_UNSPEC, { 0 } };

with

    _addr.version = NSAPI_UNSPEC;
    memset(_addr.bytes, 0, NSAPI_IP_BYTES);

The first one causes an innocent warning in GCC, but ARM compiler refused to compile it. The latter compiles fine in all three compilers. The memory initialization is still correct (and so - valgrind failures are fixed).

@0xc0170 0xc0170 added needs: CI and removed needs: work labels Feb 20, 2019

@0xc0170

This comment has been minimized.

Copy link
Member

commented Feb 20, 2019

CI started

@mbed-ci

This comment has been minimized.

Copy link

commented Feb 20, 2019

Test run: SUCCESS

Summary: 12 of 12 test jobs passed
Build number : 1
Build artifacts

@cmonr cmonr merged commit b088bd4 into ARMmbed:master Feb 20, 2019

27 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/build-ARM Success
Details
jenkins-ci/build-ARMC6 Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(+0 bytes) RAM(+0 bytes)
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARM Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Local astyle testing has passed
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/doxy-spellcheck Local doxy-spellcheck testing has passed
Details
travis-ci/events Passed, runtime is 9260 cycles
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/include_check Local include_check testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8408B
Details
travis-ci/psa-autogen Local psa-autogen testing has passed
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details
travis-ci/tools-py3.5 Local tools-py3.5 testing has passed
Details
travis-ci/tools-py3.6 Local tools-py3.6 testing has passed
Details
travis-ci/tools-py3.7 Local tools-py3.7 testing has passed
Details

@cmonr cmonr removed the ready for merge label Feb 20, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.