Fix uninitialized variable in x509_crt

[agross-oss]

This patch fixes an issue we encountered with more stringent compiler
warnings. The signature_is_good variable has a possibility of being
used uninitialized. This patch initializes the variable at the top
of the function.

Signed-off-by: Andy Gross andy.gross@linaro.org

We found this issue while using mbedtls inside the Zephyr project. We recently have started using the x509 features and this exposed this specific issue due to our compiler settings. I am unsure about backporting as we are using 2.16.0.

Gatekeeping note: (added by mpg) this PR should be merge both development and mbedtls-2.16. The issue is not present in mbedtls-2.7 so no need for backports.

[hanno-becker]

Thanks @agross-linaro for your contribution! To my understanding, signature_is_good is never used uninitialized, but it's just (too) hard for the compiler to notice that - do you agree? I remember being impressed that some compilers apparently are capable of that amount of reasoning.

[agross-oss]

@hanno-arm well for people using modern compilers on projects which require no warnings these kinds of things pop up. With all of the continues in that function I can see how this could potentially happen. But it probably would never happen with the way the function is being used.

I also wonder if tooling like coverity might catch this.

[hanno-becker]

@agross-linaro Could you elaborate? Do you see an execution path which would lead to signature_is_good being used uninitialized? I don't see one. The only way the branch parent != NULL can be taken is if the loop exited through the break instruction, which happens after signature_is_good is set.

well for people using modern compilers on projects which require no warnings

We require no warnings, too, so we'll fix this if some major toolchain warns about it - even if it's unjustified.

I also wonder if tooling like coverity might catch this.

I would expect Coverity to be able to perform the above reasoning, hence not warn about it.

[agross-oss]

So this can be bad if:

1. this is a non re-started call to x509_crt_find_parent_in()
2. you have a valid candidate list that has a single entry
3. The x509_crt_check_parent() returns non zero

With these 3 things, you can use the signature_is_good uninitialized.

[hanno-becker]

@agross-linaro No, in this case parent = parent->next will set parent to NULL before leaving the loop.

[agross-oss]

Oh you're right. Let me keep trying to find the path then =D

[hanno-becker]

@agross-linaro Whatever the outcome, the fact that it's not immediately clear is reason enough to change it. See the review suggestion.

[RonEld]

CLA valid as part of the Linaro CLA

[hanno-becker]

Thanks @agross-linaro for your contribution, looks good to me!

[mpg]

Looks good to me. I agree with Hanno that I don't see a code path where the variable could actually be used without being initialized, but the changes improve readbility by making this more obvious, and fixing spurious warnings in compilers or static analysers is also valuable. Thanks for your contribution.

[mpg]

@agross-linaro I don't think this requires backporting as development has not yet diverged enough from mbedtls-2.16 so this can be merged in both branches, and the issue is not present in mbedtls-2.7. I'm taking the liberty of editing the PR description in order to make this more apparent to gatekeepers.

[hanno-becker]

Ping @Patater @sbutcher-arm for gatekeeping.

[mpg]

@agross-linaro We usually ask external contributors to add an entry in the ChangeLog crediting themselves. Please add commit to this PR with ChangeLog entry in the Bugfix section, mentioning fixing a compiler warning, with you name (and affiliation) as you want it, plus this issue number. I'm labeling as "needs: work" as a reminder for that.

Again, thanks for your contribution.

[agross-oss]

Sorry for the delay, I had other pressing concerns. I added the requested Changelog entry.

[hanno-becker]

Thanks @agross-linaro for adding the entry! Just some nitpicking: Could you remove the double space, and perhaps say something like "Improve code clarity in X.509 module, removing false-positive uninitialized variable warnings on some toolchains."? "Fix compiler warning" is a bit too generic, and it doesn't entail whether the warning was justified and potentially hinting at a serious bug.

[pfalcon]

Hello, some updates here: Andy is not with Linaro any longer, that's why I assume this issue got stale. As his colleague, I'd be willing to pick this up and make last-mile changes to make it mergeable. Please let me know if it's ok if I open a separate PR with the changes requested (or if there's some other way). @agross-oss: Please let me know if that sounds ok for you too.

I would expect Coverity to be able to perform the above reasoning, hence not warn about it.

To elaborate on the nature of the compiler warning described here, compilers perform a static analysis, based on control flow graph (CFG) properties of the code (here, that each static use is dominated by a definition). Compilers don't perform analysis of dynamic code properties, like possible actual code execution paths, because it's equivalent of Halting problem.

Bottom line of this theoretical excursion: in software maintenance practice, such warnings can always appear, so thanks mbedTLS team for handling these proactively!

[pfalcon]

To avoid possible further slips due to weekend, etc., I proactively posted #2795 . Apologies if that's a bit hasty move, can be closed any time if this PR is going to be revamped.

[pfalcon]

This has now been superseded by #2795, #2813 . @mpg, @k-stachowiak, @Patater: Thanks for merging those, and can you please close this PR?

[mpg]

Closing for the reasons mentioned by @pfalcon.