Add variable initialization to large SSL TLS function

[sander-visser]

Signed-off-by: sander-visser github@visser.se

Description

Added variable initialization for maintainability. Reported as false positive by cpp-check 1.89

Status

READY

Requires Backporting

No

Migrations

NO

Additional comments

Any additional information that could be of interest

Todos

• Tests
• Documentation
• Changelog updated
• Backported

Steps to test or reproduce

Outline the steps to test or reproduce the PR here.

[yanesca]

@sander-visser Thank you for your contribution!

Could you please help me finding the line where this uninitialised variable is used?

(As far as I can tell mac_key_len is either initialised on line 972 or on line 1005, otherwise the function exits on line 1080 before using it.)

[sander-visser]

@sander-visser Thank you for your contribution!

Could you please help me finding the line where this uninitialised variable is used?

(As far as I can tell mac_key_len is either initialised on line 972 or on line 1005, otherwise the function exits on line 1080 before using it.)

If the mode is neither CHACHAPOLY CCM GCM CBC nor STREAM (or mbed TLS is configured to not support all modes and comehow an unsupported mode still gets processed this far) then it would still be uninitialized

[yanesca]

If the mode is neither CHACHAPOLY CCM GCM CBC nor STREAM (or mbed TLS is configured to not support all modes and comehow an unsupported mode still gets processed this far) then it would still be uninitialized

Indeed, in this case it still would be uninitialised, but we would be exiting the function on line 1080 wouldn't we?

[gilles-peskine-arm]

Or mbedtls_md_setup fails when initializing the HMAC contexts, so there's a goto end with mac_key_len uninitialized. It's still ok because mac_key_len is not used in this case. So the code is correct, but it's still fragile and it's good to fix it. I wouldn't blame a static analyzer for a false positive here.

@sander-visser Can you please add a changelog entry in ChangeLog.d, under the Changes category since there's no actual runtime bug?

[sander-visser]

Or mbedtls_md_setup fails when initializing the HMAC contexts, so there's a goto end with mac_key_len uninitialized. It's still ok because mac_key_len is not used in this case. So the code is correct, but it's still fragile and it's good to fix it. I wouldn't blame a static analyzer for a false positive here.

@sander-visser Can you please add a changelog entry in ChangeLog.d, under the Changes category since there's no actual runtime bug?

Im ok with rejecting this PR - anyway the cange is so minor it does not need to be mentioned?

[gilles-peskine-arm]

We normally acknowledge all contributions via the changelog file. But of course as the contributor you can waive this. Anyway this habit dates back from the time when the change history wasn't public, and nowadays acknowledgement is built into the git author field.

Even if there's no behavior change, this is a robustness improvement, since a small change in the code could result in the variable being used without initialization in some configurations. So thanks for catching it!

[sander-visser]

We normally acknowledge all contributions via the changelog file. But of course as the contributor you can waive this. Anyway this habit dates back from the time when the change history wasn't public, and nowadays acknowledgement is built into the git author field.

Even if there's no behavior change, this is a robustness improvement, since a small change in the code could result in the variable being used without initialization in some configurations. So thanks for catching it!

Thank - im ok to waive conrib for this change. BTW: similar construct is present for header_len in ssl_cli.c

[yanesca]

Looks good to me.

@sander-visser Thank you again for your contribution.