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.

Sign up for GitHub

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

Jump to bottom

Fix conversion on various files #8135

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Fix conversion on various files #8135

wants to merge 4 commits into from

Conversation

gasbytes
Copy link
Contributor

@gasbytes gasbytes commented Oct 31, 2024 •
edited by dgarske
Loading

These ones were linux specific (x86_64 on Void Linux).
Most of them were automated using a shell script that I wrote locally, that uses a vim interactive shell.
Working on the next block of files.

Testing: "gcc (GCC) 13.2.0"

$ ./configure --enable-all CC=gcc 'CFLAGS=-Werror -Wno-pragmas -Wall -Wextra -Wunknown-pragmas --param=ssp-buffer-size=1 -Waddress -Warray-bounds -Wbad-function-cast -Wchar-subscripts -Wcomment -Wfloat-equal -Wformat-security -Wformat=2 -Wmaybe-uninitialized -Wmissing-field-initializers -Wmissing-noreturn -Wmissing-prototypes -Wnested-externs -Wnormalized=id -Woverride-init -Wpointer-arith -Wpointer-sign -Wshadow -Wsign-compare -Wstrict-overflow=1 -Wstrict-prototypes -Wswitch-enum -Wundef -Wunused -Wunused-result -Wunused-variable -Wwrite-strings -fwrapv -Wsign-conversion -fmax-errors=1'
$ make

@gasbytes gasbytes requested a review from douzzer October 31, 2024 17:59
@gasbytes gasbytes assigned wolfSSL-Bot and gasbytes and unassigned wolfSSL-Bot and gasbytes Oct 31, 2024
douzzer
douzzer previously requested changes Nov 9, 2024
View reviewed changes
Copy link
Contributor

@douzzer douzzer left a comment

Choose a reason for hiding this comment

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

rebase+reconciliation needed.

CONFLICT (content): Merge conflict in src/tls.c

looks good otherwise.

@douzzer douzzer assigned gasbytes and unassigned wolfSSL-Bot Nov 9, 2024
@gasbytes
Copy link
Contributor Author

retest this please

@gasbytes gasbytes assigned wolfSSL-Bot and unassigned gasbytes Nov 13, 2024
@dgarske
Copy link
Contributor

dgarske commented Nov 15, 2024

@gasbytes please share how you produced these conversion warnings. Which complier and what build steps. Thank you

@dgarske
Copy link
Contributor

dgarske commented Nov 15, 2024

@gasbytes please squash.

@gasbytes
Copy link
Contributor Author

gasbytes commented Nov 15, 2024 •
edited
Loading

@gasbytes please share how you produced these conversion warnings. Which complier and what build steps. Thank you

@dgarske
Of course, version of gcc:

$ gcc --version
$ gcc (GCC) 13.2.0

And this is the configuration, followed just by a make:

$ ./config.status --config
$ --enable-all CC=gcc 'CFLAGS=-Werror -Wno-pragmas -Wall -Wextra -Wunknown-pragmas --param=ssp-buffer-size=1 -Waddress -Warray-bounds -Wbad-function-cast -Wchar-subscripts -Wcomment -Wfloat-equal -Wformat-security -Wformat=2 -Wmaybe-uninitialized -Wmissing-field-initializers -Wmissing-noreturn -Wmissing-prototypes -Wnested-externs -Wnormalized=id -Woverride-init -Wpointer-arith -Wpointer-sign -Wshadow -Wsign-compare -Wstrict-overflow=1 -Wstrict-prototypes -Wswitch-enum -Wundef -Wunused -Wunused-result -Wunused-variable -Wwrite-strings -fwrapv -Wsign-conversion -fmax-errors=1'

@dgarske dgarske requested a review from douzzer November 15, 2024 14:43
@dgarske
Copy link
Contributor

dgarske commented Nov 15, 2024

Retest this please:

[make check (macos-latest, --enable-harden-tls)](https://github.com/wolfSSL/wolfssl/pull/8135#logs)

Test complete
wolfSSL error: tcp connect failed: Connection refused

Running simple test
SSL version is TLSv1.2
SSL cipher suite is TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384
SSL curve name is SECP256R1
SSL version is TLSv1.2
SSL cipher suite is TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384
SSL curve name is SECP256R1
Client message: hello wolfssl!
I hear you fa shizzle!

Running TLS test
FAIL testsuite/testsuite.test (exit status: 1)

./configure --enable-aesgcm=table --enable-all --enable-intelasm --enable-sp-math-all 
FAIL scripts/ocsp-stapling_tls13multi.test (exit status: 1)

dgarske
dgarske previously approved these changes Nov 15, 2024
View reviewed changes
@dgarske dgarske assigned douzzer and unassigned douzzer and gasbytes Nov 15, 2024
@douzzer
Copy link
Contributor

douzzer commented Nov 16, 2024

has a slew of conflicts relative to current master:

CONFLICT (content): Merge conflict in configure.ac
CONFLICT (content): Merge conflict in src/ssl.c
CONFLICT (content): Merge conflict in src/tls.c
CONFLICT (content): Merge conflict in tests/quic.c

it's a mystery to me why github thinks "Merging can be performed automatically"...

@douzzer douzzer assigned gasbytes and unassigned wolfSSL-Bot Nov 16, 2024
@gasbytes
Copy link
Contributor Author

@douzzer

I think this might be because I resolved those conflicts using GitHub's web editor, but I'm not entirely sure why GitHub now says the merge can be performed automatically. As far as I can tell, those files don’t seem to have any breaking changes compared to master, so that might also be a reason.

JacobBarthelmeh
JacobBarthelmeh requested changes Nov 19, 2024
View reviewed changes
src/internal.c
DYNAMIC_TYPE_HASHES);
(*destination)->messages = (byte*)XMALLOC((size_t)source->length,
ssl->heap,
(size_t)DYNAMIC_TYPE_HASHES);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the third argument to our malloc's are an int type. This should either be cast to an int type or not cast.

src/ssl.c Outdated
@@ -16437,8 +16442,8 @@ long wolfSSL_clear_options(WOLFSSL* ssl, long opt)
WOLFSSL_ENTER("wolfSSL_clear_options");
if(ssl == NULL)
return WOLFSSL_FAILURE;
ssl->options.mask &= ~opt;
return ssl->options.mask;
ssl->options.mask &= (long unsigned int)~opt;
Copy link
Contributor

Choose a reason for hiding this comment

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

In wolfssl/internal.h I see options.mask being an unsigned long type, not long unsigned int.

tests/quic.c Outdated
@@ -21,6 +21,10 @@

#ifdef HAVE_CONFIG_H
#include <config.h>
#else
#ifndef WOLFSSL_USER_SETTINGS
#include <wolfssl/options.h>
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a duplicate include? Right below this there is also a conditional include of wolfssl/options.h

wolfcrypt/src/evp.c Outdated
@@ -6293,14 +6293,16 @@ void wolfSSL_EVP_init(void)
case WC_AES_256_OFB_TYPE:
#endif
wc_AesFree(&ctx->cipher.aes);
ctx->flags &= ~WOLFSSL_EVP_CIPH_LOW_LEVEL_INITED;
ctx->flags &=
(long unsigned int)~WOLFSSL_EVP_CIPH_LOW_LEVEL_INITED;
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets match the type used in the struct for this and the next cast. Using unsigned long

wolfcrypt/src/logging.c
@@ -1420,7 +1420,7 @@ unsigned long wc_PeekErrorNodeLineData(const char **file, int *line,
}
}

unsigned long wc_GetErrorNodeErr(void)
int wc_GetErrorNodeErr(void)
Copy link
Contributor

Choose a reason for hiding this comment

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

If changing this to a return type of int go through the function and check all return cases. I see a cast to unsigned long on one of them.

@gasbytes
Copy link
Contributor Author

Retest this please (#8182)

@gasbytes gasbytes assigned wolfSSL-Bot and gasbytes and unassigned gasbytes and wolfSSL-Bot Nov 21, 2024
@gasbytes
Copy link
Contributor Author

Working on a couple more Renesas specific.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JacobBarthelmeh JacobBarthelmeh JacobBarthelmeh requested changes

@dgarske dgarske dgarske left review comments

@douzzer douzzer Awaiting requested review from douzzer

Requested changes must be addressed to merge this pull request.

Assignees

@gasbytes gasbytes

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@gasbytes @dgarske @douzzer @JacobBarthelmeh @wolfSSL-Bot