Initialize vars & change types to appease Windows/VS #8181

gojimmypi · 2024-11-13T21:26:12Z

Description

Addresses these warnings encountered during a Windows / Visual Studio build:

edit: also introduces #define WOLFSSL_MAX_8BIT 0xffU in type.h

1>C:\workspace\wolfssl\wolfssl\wolfcrypt\curve25519.h(151,1): warning C4141: 'dllexport': used more than once
1>C:\workspace\wolfssl\wolfssl\wolfcrypt\curve25519.h(151,1): warning C4141: 'dllexport': used more than once
1>C:\workspace\wolfssl\src\dtls.c(721,36): warning C4244: 'function': conversion from 'word32' to 'word16', possible loss of data
1>C:\workspace\wolfssl\src\dtls.c(732,67): warning C4244: 'function': conversion from 'word32' to 'word16', possible loss of data
1>C:\workspace\wolfssl\wolfssl\wolfcrypt\curve25519.h(151,1): warning C4141: 'dllexport': used more than once
1>C:\workspace\wolfssl\wolfssl\wolfcrypt\curve25519.h(151,1): warning C4141: 'dllexport': used more than once
1>C:\workspace\wolfssl\wolfssl\wolfcrypt\curve25519.h(151,1): warning C4141: 'dllexport': used more than once
1>C:\workspace\wolfssl\wolfssl\wolfcrypt\curve25519.h(151,1): warning C4141: 'dllexport': used more than once
1>C:\workspace\wolfssl\src\ssl_sess.c(3891,46): warning C4244: '=': conversion from 'int' to 'word16', possible loss of data
1>C:\workspace\wolfssl\src\ssl_sess.c(4167,22): warning C4244: '=': conversion from 'unsigned int' to 'byte', possible loss of data
1>C:\workspace\wolfssl\src\ssl_sess.c(4183,23): warning C4244: '=': conversion from 'unsigned int' to 'byte', possible loss of data
1>C:\workspace\wolfssl\src\ssl_asn1.c(288,36): warning C4244: 'function': conversion from 'const int' to 'byte', possible loss of data
1>C:\workspace\wolfssl\src\bio.c(1382,25): warning C4244: '=': conversion from 'int' to 'byte', possible loss of data
1>C:\workspace\wolfssl\src\x509.c(1476,59): warning C4244: '=': conversion from 'int' to 'byte', possible loss of data
1>C:\workspace\wolfssl\src\ssl_p7p12.c(951,21): warning C4245: 'initializing': conversion from 'int' to 'word32', signed/unsigned mismatch
1>C:\workspace\wolfssl\wolfssl\wolfcrypt\curve25519.h(151,1): warning C4141: 'dllexport': used more than once
1>C:\workspace\wolfssl\wolfssl\wolfcrypt\curve25519.h(151,1): warning C4141: 'dllexport': used more than once
1>C:\workspace\wolfssl\src\tls13.c(4036,17): warning C4244: 'function': conversion from 'word32' to 'word16', possible loss of data
1>C:\workspace\wolfssl\src\tls13.c(6316,20): warning C4244: '=': conversion from 'word32' to 'word16', possible loss of data
1>C:\workspace\wolfssl\src\tls13.c(10862,39): warning C4244: '=': conversion from 'word32' to 'byte', possible loss of data
1>C:\workspace\wolfssl\src\tls13.c(10866,39): warning C4244: '=': conversion from 'word32' to 'byte', possible loss of data
1>C:\workspace\wolfssl\src\tls13.c(11036,39): warning C4244: '=': conversion from 'int' to 'byte', possible loss of data
1>C:\workspace\wolfssl\src\tls13.c(11040,39): warning C4244: '=': conversion from 'int' to 'byte', possible loss of data
1>C:\workspace\wolfssl\src\tls13.c(11949,49): warning C4244: 'function': conversion from 'int' to 'word16', possible loss of data
1>C:\workspace\wolfssl\src\tls13.c(11949,57): warning C4244: 'function': conversion from 'word32' to 'word16', possible loss of data
1>C:\workspace\wolfssl\wolfssl\wolfcrypt\curve25519.h(151,1): warning C4141: 'dllexport': used more than once
1>C:\workspace\wolfssl\wolfssl\wolfcrypt\curve25519.h(151,1): warning C4141: 'dllexport': used more than once
1>C:\workspace\wolfssl\wolfcrypt\src\asn.c(13614,1): warning C4701: potentially uninitialized local variable 'finalName' used
1>C:\workspace\wolfssl\wolfcrypt\src\asn.c(13614,1): warning C4703: potentially uninitialized local pointer variable 'finalName' used
1>C:\workspace\wolfssl\src\pk.c(3666,1): warning C4701: potentially uninitialized local variable 'hashType' used
1>C:\workspace\wolfssl\src\pk.c(7961,1): warning C4701: potentially uninitialized local variable 'derSz' used
1>C:\workspace\wolfssl\src\pk.c(16559,1): warning C4701: potentially uninitialized local variable 'curveOid' used
1>C:\workspace\wolfssl\src\pk.c(16559,1): warning C4703: potentially uninitialized local pointer variable 'curveOid' used
1>C:\workspace\wolfssl\src\ssl_load.c(5045,1): warning C4701: potentially uninitialized local variable 'derSize' used
1>C:\workspace\wolfssl\src\pk.c(7901,1): warning C4701: potentially uninitialized local variable 'derSz' used
1>C:\workspace\wolfssl\src\pk.c(16607,1): warning C4701: potentially uninitialized local variable 'keySz' used
1>C:\workspace\wolfssl\wolfssl\wolfcrypt\curve25519.h(151,1): warning C4141: 'dllexport': used more than once
1>C:\workspace\wolfssl\wolfcrypt\src\dsa.c(176,5): warning C4003: not enough arguments for function-like macro invocation 'SAVE_VECTOR_REGISTERS'
1>C:\workspace\wolfssl\wolfcrypt\src\ecc.c(12596,39): warning C4334: '<<': result of 32-bit shift implicitly converted to 64 bits (was 64-bit shift intended?)
1>C:\workspace\wolfssl\wolfcrypt\src\ecc.c(12597,39): warning C4334: '<<': result of 32-bit shift implicitly converted to 64 bits (was 64-bit shift intended?)
1>C:\workspace\wolfssl\wolfcrypt\src\ecc.c(12598,39): warning C4334: '<<': result of 32-bit shift implicitly converted to 64 bits (was 64-bit shift intended?)
1>C:\workspace\wolfssl\wolfcrypt\src\ecc.c(12599,39): warning C4334: '<<': result of 32-bit shift implicitly converted to 64 bits (was 64-bit shift intended?)
1>C:\workspace\wolfssl\wolfcrypt\src\ecc.c(12600,39): warning C4334: '<<': result of 32-bit shift implicitly converted to 64 bits (was 64-bit shift intended?)
1>C:\workspace\wolfssl\wolfcrypt\src\ecc.c(12601,39): warning C4334: '<<': result of 32-bit shift implicitly converted to 64 bits (was 64-bit shift intended?)
1>C:\workspace\wolfssl\wolfcrypt\src\ecc.c(12608,74): warning C4334: '<<': result of 32-bit shift implicitly converted to 64 bits (was 64-bit shift intended?)
1>C:\workspace\wolfssl\wolfcrypt\src\ecc.c(12609,48): warning C4334: '<<': result of 32-bit shift implicitly converted to 64 bits (was 64-bit shift intended?)
1>C:\workspace\wolfssl\wolfcrypt\src\ecc.c(13590,1): warning C4701: potentially uninitialized local variable 'mp' used
1>C:\workspace\wolfssl\wolfcrypt\src\ecc.c(13275,1): warning C4701: potentially uninitialized local variable 'mp' used
1>C:\workspace\wolfssl\wolfcrypt\src\ecc.c(13413,1): warning C4701: potentially uninitialized local variable 'mp' used
1>C:\workspace\wolfssl\wolfcrypt\src\pkcs7.c(13303,1): warning C4701: potentially uninitialized local variable 'tag' used
1>C:\workspace\wolfssl\wolfcrypt\src\pkcs7.c(14151,1): warning C4701: potentially uninitialized local variable 'tag' used
1>C:\workspace\wolfssl\wolfcrypt\src\pkcs7.c(14219,1): warning C4701: potentially uninitialized local variable 'version' used
1>C:\workspace\wolfssl\wolfcrypt\src\pkcs7.c(5513,1): warning C4701: potentially uninitialized local variable 'outerContentType' used
1>C:\workspace\wolfssl\wolfcrypt\src\pkcs7.c(12110,1): warning C4701: potentially uninitialized local variable 'length' used
1>C:\workspace\wolfssl\wolfcrypt\src\pkcs7.c(12000,1): warning C4701: potentially uninitialized local variable 'contentType' used
1>C:\workspace\wolfssl\wolfcrypt\src\pkcs7.c(12013,1): warning C4701: potentially uninitialized local variable 'tag' used
1>C:\workspace\wolfssl\wolfcrypt\src\pkcs7.c(5177,1): warning C4701: potentially uninitialized local variable 'length' used

edit: This PR does not include fixes for the warning C4141: 'dllexport': used more than once, noted above.

Fixes zd# n/a

Testing

Confirmed working with benchmark and test apps.

Checklist

added tests
updated/added doxygen
updated appropriate READMEs
Updated manual and documentation

src/bio.c

src/dtls.c

src/ssl_asn1.c

src/ssl_sess.c

src/tls13.c

src/x509.c

gojimmypi · 2024-11-14T19:46:48Z

Jenkins retest this please

dgarske

Nice cleanups and extra checks. Passes my review and testing. Over to @SparkiDev to finalize.

douzzer

Mostly looks good! Had a few nits and a question or two.

douzzer · 2024-11-17T01:50:54Z

src/bio.c

+        if ((closeFlag != WOLFSSL_BIO_NOCLOSE ) && \
+            (closeFlag != WOLFSSL_BIO_CLOSE)) {
+            return BAD_FUNC_ARG;
+        }
+


This check duplicates the check right above it. The earlier check should be using the WOLFSSL_ version of the constants, so by all means fix that.

Oh, that's an excellent catch! I removed that code, and edited the one above.

douzzer · 2024-11-17T01:52:27Z

src/bio.c

        bio->mem_buf = bufMem;
-        bio->shutdown = closeFlag;
+        bio->shutdown = (byte)closeFlag;


bio->shutdown is actually a single bit in a bitfield, so the right construction here is closeFlag ? 1 : 0.

What do you think of using these, instead?

bio->shutdown = closeFlag ? WOLFSSL_BIO_CLOSE : WOLFSSL_BIO_NOCLOSE

douzzer · 2024-11-17T01:55:21Z

src/ssl_asn1.c

-            SetImplicit(tmp[0], mem->tag, 0, imp, 0);
+            /* Encode the implicit tag; There's other stuff in the upper bits
+             * of the integer tag, so strip out everything else for value. */
+            SetImplicit(tmp[0], (byte)(mem->tag & ASN_IMPLICIT_TAG_MASK),


When you're explicitly casting to byte, there is no reason to explicitly mask. The (byte) always masks as needed.

Indeed there's no need to explicitly mask, and I suppose with the new comments it's ok. But I generally prefer code that's more intuitive to immediately understand. That's also why I spelled out the full size of 0x000000FF and not just 0xFF. Happy to change it if you prefer otherwise.

I would prefer the masking removed and it suggests that not all of the bottom 8 bits will be used. We want it all.

ok, @SparkiDev - masking macro removed.

douzzer · 2024-11-17T01:55:47Z

src/ssl_asn1.c

+#define ASN_IMPLICIT_TAG_MASK 0x000000FF
+


Not needed -- see below.

Intentional, but open to change - particularly since known non-zero data gets truncated.

douzzer · 2024-11-17T02:01:36Z

src/tls13.c

    int    sendSz;
    word16 extSz;
-    word32 length;
-    word32 idx = RECORD_HEADER_SZ + HANDSHAKE_HEADER_SZ;
+    word16 idx = RECORD_HEADER_SZ + HANDSHAKE_HEADER_SZ;


Seems like the wrong idea to use word16 for idx -- makes it more vulnerable to overflow. Though to be fair, there are currently no checks in the body of the function for overflow. Perhaps revert idx to word32, and add casts where necessary, immediately preceded by overflow tests?

Ah, yes. Good point. word32 for idx. What would you use for an error code? Add something like this in here tls13.c?

if (idx > WOLFSSL_MAX_16BIT) { return BAD_LENGTH_E; }

Although the prior change seemed happy with the VS/Windows build, today I was seeing the "conversion from 'word32' to 'word16', possible loss of data" error again today.

So in addition to removing the ASN_IMPLICIT_TAG_MASK in ssl.asn1.c, I've also added explicit (word16) type casts to the 32-bit idx in the tls13.c file.

wolfcrypt/src/dsa.c

gojimmypi requested review from SparkiDev and dgarske November 13, 2024 21:26

gojimmypi assigned gojimmypi, dgarske and SparkiDev Nov 13, 2024

dgarske removed request for dgarske and SparkiDev November 13, 2024 21:49

dgarske assigned wolfSSL-Bot and unassigned dgarske and SparkiDev Nov 13, 2024

dgarske requested a review from douzzer November 13, 2024 21:49

SparkiDev requested changes Nov 13, 2024

View reviewed changes

gojimmypi force-pushed the dev-compiler-message branch from 7791535 to 028195d Compare November 13, 2024 23:43

gojimmypi requested a review from SparkiDev November 13, 2024 23:46

gojimmypi force-pushed the dev-compiler-message branch 4 times, most recently from 2ad9df3 to 8e4cc38 Compare November 14, 2024 18:10

dgarske assigned SparkiDev and unassigned gojimmypi Nov 15, 2024

dgarske previously approved these changes Nov 15, 2024

View reviewed changes

douzzer requested changes Nov 17, 2024

View reviewed changes

douzzer assigned gojimmypi and douzzer and unassigned wolfSSL-Bot Nov 17, 2024

gojimmypi dismissed dgarske’s stale review via 2f04bf0 November 17, 2024 03:53

gojimmypi force-pushed the dev-compiler-message branch from 8e4cc38 to 2f04bf0 Compare November 17, 2024 03:53

gojimmypi requested a review from douzzer November 17, 2024 03:58

Initialize vars & change types to appease Windows/VS

5d86031

gojimmypi force-pushed the dev-compiler-message branch from 2f04bf0 to 5d86031 Compare November 18, 2024 01:50

dgarske approved these changes Nov 18, 2024

View reviewed changes

dgarske assigned wolfSSL-Bot and unassigned douzzer and gojimmypi Nov 18, 2024

SparkiDev approved these changes Nov 19, 2024

View reviewed changes

SparkiDev assigned douzzer and unassigned SparkiDev Nov 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initialize vars & change types to appease Windows/VS #8181

Initialize vars & change types to appease Windows/VS #8181

gojimmypi commented Nov 13, 2024 •

edited

Loading

gojimmypi commented Nov 14, 2024

dgarske left a comment

douzzer left a comment

douzzer Nov 17, 2024

gojimmypi Nov 17, 2024

douzzer Nov 17, 2024

gojimmypi Nov 17, 2024

douzzer Nov 17, 2024

gojimmypi Nov 17, 2024

SparkiDev Nov 18, 2024

gojimmypi Nov 18, 2024

douzzer Nov 17, 2024

gojimmypi Nov 17, 2024

douzzer Nov 17, 2024

gojimmypi Nov 17, 2024

gojimmypi Nov 18, 2024

Initialize vars & change types to appease Windows/VS #8181

Are you sure you want to change the base?

Initialize vars & change types to appease Windows/VS #8181

Conversation

gojimmypi commented Nov 13, 2024 • edited Loading

Description

Testing

Checklist

gojimmypi commented Nov 14, 2024

dgarske left a comment

Choose a reason for hiding this comment

douzzer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gojimmypi commented Nov 13, 2024 •

edited

Loading