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

Fixes for build and runtime issues #5483

Merged
merged 1 commit into from
Aug 22, 2022
Merged

Conversation

embhorn
Copy link
Member

@embhorn embhorn commented Aug 18, 2022

Description

Fixes the following:

  • Documentation for freeing fixed point ECC cache
  • Free packet memory when WOLFSSL_CALLBACKS timeout CB is null
  • Build error with HAVE_EX_DATA_CLEANUP_HOOKS and HAVE_EX_DATA not defined
  • wolfSSL_BIO_clear_retry_flags to also clear the WOLFSSL_BIO_FLAG_WRITE flag
  • fixed a build error in the example server when WOLFSSL_CALLBACKS and WOLFSSL_EARLY_DATA are defined

Fixes zd14659

Testing

commit checks

Checklist

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

@embhorn embhorn self-assigned this Aug 18, 2022
@embhorn
Copy link
Member Author

embhorn commented Aug 18, 2022

Fixes #5482

@maxammann
Copy link

maxammann commented Aug 19, 2022

The "Build error with HAVE_EX_DATA_CLEANUP_HOOKS and HAVE_EX_DATA not defined" was related to the usage of a wrong type called X509. This type does not exist in wolfSSL afaik, only WOLFSSL_X509 exists. Also function definitions above or after wolfSSL_X509_set_ex_data_with_cleanup use WOLFSSL_X509.

EDIT: Refering to the following line which seems odd: https://github.com/wolfSSL/wolfssl/blob/master/wolfssl/ssl.h#L4802

Copy link

@maxammann maxammann left a comment

Choose a reason for hiding this comment

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

Looks good for me except for the mentioned issue about the X509 type.

Comment on lines 639 to 640
FP_ECC (fixed-point ecc), should be defined. Threaded applications should
call this function upon exit.

Choose a reason for hiding this comment

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

I think it would be nice to notice this aswell in the manual. As a user of wolfSSL I would look for the "Thread Safety" section if I would implement a multi-threaded server: https://www.wolfssl.com/documentation/manuals/wolfssl/chapter09.html#thread-safety

Copy link
Member Author

@embhorn embhorn Aug 19, 2022

Choose a reason for hiding this comment

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

Great idea! I added this blurb to that section:

Some optimizations allocate memory on a per thread basis. If fixed point ECC cache is enabled (FP_ECC), then threads should release the cached buffers using wc_ecc_fp_free() before the thread exits.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not seeing this pushed to manual anywhere. Can you please check if you opened the PR to add this?

doc/dox_comments/header_files/ecc.h Outdated Show resolved Hide resolved
src/ssl.c Outdated Show resolved Hide resolved
@embhorn
Copy link
Member Author

embhorn commented Aug 19, 2022

The "Build error with HAVE_EX_DATA_CLEANUP_HOOKS and HAVE_EX_DATA not defined" was related to the usage of a wrong type called X509. This type does not exist in wolfSSL afaik, only WOLFSSL_X509 exists. Also function definitions above or after wolfSSL_X509_set_ex_data_with_cleanup use WOLFSSL_X509.

EDIT: Refering to the following line which seems odd: https://github.com/wolfSSL/wolfssl/blob/master/wolfssl/ssl.h#L4802

The compatibility layer headers map WOLFSSL_X509 to X509:

typedef WOLFSSL_X509 X509;

I've pushed a fix for the line you mentioned.

 WOLFSSL_API int wolfSSL_X509_set_ex_data_with_cleanup(
-    X509 *x509,
+    WOLFSSL_X509 *x509,

@kaleb-himes
Copy link
Contributor

retest this please.

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.

None yet

5 participants