Fix NULL argument handling in mbedtls_xxx_free() functions

[Troy-Butler]

Description

Added conditional statements to mbedtls_xxx_free() functions to handle NULL arguments. Resolves #8915 Replaces PR #8955

PR checklist

Please tick as appropriate and edit the reasons (e.g.: "backport: not needed because this is a new feature")

• [x] changelog not required
• [x] 3.6 backport https://github.com/Mbed-TLS/mbedtls/pull/9045
• [x] 2.28 backport not required
• [x] tests not required

[paul-elliott-arm]

@Troy-Butler - you don't need to close and recreate each time, btw. Just push more commits to your feature branch.

[irwir]

There is no reason to check for NULL if the argument pointer is not dereferenced. For example, in mbedtls_lmots_public_free the argument is directly passed to zeroize function, which can properly handle NULL pointers. It should be mentioned that some very old code did similar checks too - see aes.c module.

Also, in such cases return statement adds unnecessary lines that could be eliminated by using reversed condition if (ctx != NULL) ... Of course, an early return might be preferable if function is long.

[tom-cosgrove-arm]

in such cases return statement adds unnecessary lines

I personally would prefer an early return even in a short function, rather than having "arrow code"

[irwir]

I personally would prefer an early return even in a short function, rather than having "arrow code"

//1
if (ctx == NULL) {
    return;
}
mbedtls_..._free(ctx);

//2
if (ctx != NULL) {
    mbedtls_..._free(ctx);
}

There seems to be exactly the same amount of "arrow code" in both examples, but the second needs less statements and thus could be read easier.

[tom-cosgrove-arm]

Yes, obviously, if it's just one line then fair enough use option 2, but for 3 or more statements inside the if then don't :)

[Troy-Butler]

I personally would prefer an early return even in a short function, rather than having "arrow code"

//1
if (ctx == NULL) {
    return;
}
mbedtls_..._free(ctx);

//2
if (ctx != NULL) {
    mbedtls_..._free(ctx);
}

There seems to be exactly the same amount of "arrow code" in both examples, but the second needs less statements and thus could be read easier.

I opted for an early return to remain consistent with existing null checks in other files.

If you want me to change my approach, please let me know.

[tom-cosgrove-arm]

I opted for an early return to remain consistent with existing null checks in other files.

If you want me to change my approach, please let me know.

Consistency is good; please don't change these

[Roytak]

Hi, happy to see the fix, anyways, I just noticed it in mbedtls_ssl_config_free as well and if I am not mistaken you missed that one.

[Troy-Butler]

mbedtls_ssl_config_free

Not sure how that happened, but I'll take care of it.

[Troy-Butler]

Is there anything else I need to do?

[Troy-Butler]

I see that a "3.6 backport" checkbox has been added to this pull request. I was told that no testing was required for this issue - has that changed?

Also, how do I go about getting reviewers assigned to this pull request?

[tom-cosgrove-arm]

Needs backport to 3.6. Please don't close this PR (and gatekeepers, don't merge this one yet)

[Troy-Butler]

Hi, this looks good so far, but it definitely needs backporting to 3.6 (mbedtls-3.6 branch)

How do I do that?

[paul-elliott-arm]

Hi, this looks good so far, but it definitely needs backporting to 3.6 (mbedtls-3.6 branch)

How do I do that?

You could just cherry pick the 2 commits onto the mbedtls-3.6 branch and then make a new PR from that, but don't forget to specify the target branch for the PR to be mbedtls-3.6. After that, link the new PR here, and we can review it.