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

Fix llvm error: variable 'default_iv_length' and other may be used uninitialized #7210

Merged
merged 1 commit into from
Sep 13, 2023

Conversation

sergio-nsk
Copy link
Contributor

@sergio-nsk sergio-nsk commented Mar 6, 2023

Description

mbedtls/library/psa_crypto.c:3428:30: error: variable 'default_iv_length' may be uninitialized when used here [-Werror,-Wconditional-uninitialized]
        memcpy(iv, local_iv, default_iv_length);
                             ^~~~~~~~~~~~~~~~~
mbedtls/library/psa_crypto.c:4646:36: error: variable 'required_nonce_size' may be uninitialized when used here [-Werror,-Wconditional-uninitialized]
        memcpy(nonce, local_nonce, required_nonce_size);
                                   ^~~~~~~~~~~~~~~~~~~
mbedtls/library/ssl_cache.c:309:54: error: variable 'session_serialized_len' may be uninitialized when used here [-Werror,-Wconditional-uninitialized]
        mbedtls_platform_zeroize(session_serialized, session_serialized_len);
                                                     ^~~~~~~~~~~~~~~~~~~~~~

The warnings are false positive, but treated as errors and prevent compiling on Windows with LLVM Clang.

Gatekeeper checklist

sergio-nsk added a commit to sergio-nsk/mbedtls that referenced this pull request Mar 6, 2023
@sergio-nsk sergio-nsk force-pushed the patch-2 branch 2 times, most recently from d2590ba to 8849e6b Compare March 6, 2023 23:20
@bensze01 bensze01 self-assigned this Mar 8, 2023
@tom-cosgrove-arm
Copy link
Contributor

This doesn't give a fully clean build with -Wconditional-uninitialized - I haven't checked the backport

suites/test_suite_psa_crypto.function:9776:38: error: variable 'first_exported_length' may be uninitialized when used here
      [-Werror,-Wconditional-uninitialized]
        ASSERT_COMPARE(first_export, first_exported_length,
                                     ^~~~~~~~~~~~~~~~~~~~~
./include/test/macros.h:172:14: note: expanded from macro 'ASSERT_COMPARE'
        if ((size1) != 0)                                            \
             ^~~~~
suites/test_suite_psa_crypto.function:9676:33: note: initialize the variable 'first_exported_length' to silence this warning
    size_t first_exported_length;
                                ^
                                 = 0

@sergio-nsk
Copy link
Contributor Author

sergio-nsk commented Mar 13, 2023

I tried only UWP builds with LLVM clang and w/o test builds.

@tom-cosgrove-arm
Copy link
Contributor

Could you share how you build for UWP?

@sergio-nsk
Copy link
Contributor Author

sergio-nsk commented Mar 14, 2023

With cmake using a toolchain file, that tunes cmake variables for using LLVM clang compiler (version 15.0.7).

cmake -S . -B out -G Ninja -DCMAKE_BUILD_TYPE=Debug CMAKE_SYSTEM_NAME=WindowsStore -DCMAKE_SYSTEM_VERSION=10.0 -DCMAKE_TOOLCHAIN_FILE=cmake/toolchain-clang.cmake

I suppose the toolchain file is not interesting. The compiler flags used are more interesting, they are set to CMAKE_C_FLAGS and CMAKE_CXX_FLAGS in CMakeLists.txt:

-Wall
-Wextra
-Wuninitialized
-Wsometimes-uninitialized
-Wconditional-uninitialized
-Wno-unused-function
-Wno-error=missing-braces
-Wno-deprecated
-Wno-unused-parameter
-Wno-missing-field-initializers
-Wstrict-prototypes
-Wimplicit-fallthrough
-Wwrite-strings
-Werror

@sergio-nsk sergio-nsk changed the title Fix llvm error: variable 'default_iv_length' may be used uninitialized Fix llvm error: variable 'default_iv_length' and other may be used uninitialized Mar 14, 2023
@davidhorstmann-arm davidhorstmann-arm added the historical-reviewing Currently reviewing (for legacy PR/issues) label Jun 2, 2023
Copy link
Contributor

@tom-cosgrove-arm tom-cosgrove-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@davidhorstmann-arm davidhorstmann-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@davidhorstmann-arm davidhorstmann-arm added approved Design and code approved - may be waiting for CI or backports size-s Estimated task size: small (~2d) and removed historical-reviewing Currently reviewing (for legacy PR/issues) labels Sep 12, 2023
@paul-elliott-arm paul-elliott-arm added the needs-ci Needs to pass CI tests label Sep 12, 2023
@gilles-peskine-arm gilles-peskine-arm merged commit 0ddffb6 into Mbed-TLS:development Sep 13, 2023
@sergio-nsk sergio-nsk deleted the patch-2 branch August 14, 2024 22:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Design and code approved - may be waiting for CI or backports needs-ci Needs to pass CI tests size-s Estimated task size: small (~2d)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants