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

ssl.c: Move functions out to separate files #7297

Merged
merged 1 commit into from
Apr 16, 2024

Conversation

SparkiDev
Copy link
Contributor

@SparkiDev SparkiDev commented Mar 5, 2024

Description

Moved E[CD][25519||448] APIs to pk.c
Move wolfSSL loading and using of private keys and certificates to ssl_load.c
Move PKCS#7 and PKCS#12 APIs to ssl_p7p12.c.
Move PEM APIs to ssl_pem.c.
Move session and session cache APIs to ssl_sess.c. Other minor fixes.

Testing

Regression tested.

Checklist

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

@SparkiDev
Copy link
Contributor Author

retest this please

Moved E[CD][25519||448] APIs to pk.c
Move public key PEM APIs to pk.c.
Move wolfSSL loading and using of private keys and certificates to
ssl_load.c
Move PKCS#7 and PKCS#12 APIs to ssl_p7p12.c.
Move session and session cache APIs to ssl_sess.c.
Other minor fixes.
Copy link
Contributor

@dgarske dgarske left a comment

Choose a reason for hiding this comment

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

So much excellent code cleanup and reorganization. Thank you @SparkiDev. I also compared code size and the newer code is slightly smaller.

@@ -54,7 +56,7 @@ static int wolfssl_read_bio_file(WOLFSSL_BIO* bio, char** data)
char* p;

/* Allocate buffer to hold a chunk of data. */
mem = (char*)XMALLOC(READ_BIO_FILE_CHUNK, bio->heap, DYNAMIC_TYPE_OPENSSL);
mem = (char*)XMALLOC(READ_BIO_FILE_CHUNK, NULL, DYNAMIC_TYPE_TMP_BUFFER);
Copy link
Contributor

Choose a reason for hiding this comment

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

Was the loss of bio->heap on these on purpose?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes.
The memory allocated is passed back to the caller and there are numerous places that free it, including one that doesn't have access to the BIO, and they use NULL for the heap parameter.

@@ -4553,7 +4557,7 @@ WOLFSSL_API WOLFSSL_X509_NAME_ENTRY *wolfSSL_X509_NAME_get_entry(WOLFSSL_X509_NA
WOLFSSL_API void wolfSSL_X509_NAME_ENTRY_free(WOLFSSL_X509_NAME_ENTRY* ne);
WOLFSSL_API WOLFSSL_X509_NAME_ENTRY* wolfSSL_X509_NAME_ENTRY_new(void);
WOLFSSL_API void wolfSSL_X509_NAME_free(WOLFSSL_X509_NAME* name);
WOLFSSL_API char wolfSSL_CTX_use_certificate(WOLFSSL_CTX* ctx, WOLFSSL_X509* x);
WOLFSSL_API int wolfSSL_CTX_use_certificate(WOLFSSL_CTX* ctx, WOLFSSL_X509* x);
Copy link
Contributor

Choose a reason for hiding this comment

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

Was the change from char to int necessary or a clean up to make it match other functions? This will likely break ABI and looks like it's been a char a long time. Am on the fence if we should just change it anyway like what's done in this PR....

Copy link
Contributor

Choose a reason for hiding this comment

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

To clarify breaking API backwards compatibility, in that we need to update SONAME next release, not breaking WOLFSSL_ABI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OpenSSL has the API return an int.
The returned values were and are WOLFSSL_SUCCESS and WOLFSSL_FAILURE so using int won't affect the return values.
Saw it as fair game as it didn't have WOLFSSL_ABI.

@JacobBarthelmeh JacobBarthelmeh merged commit 03ed52b into wolfSSL:master Apr 16, 2024
114 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants