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

20240718-BIO_DGRAM-memory-leak #7760

Merged
merged 1 commit into from
Jul 19, 2024
Merged

20240718-BIO_DGRAM-memory-leak #7760

merged 1 commit into from
Jul 19, 2024

Conversation

douzzer
Copy link
Contributor

@douzzer douzzer commented Jul 18, 2024 •
edited
Loading

src/bio.c and related:

  • refactor WOLFSSL_BIO.num and WOLFSSL_BIO.ptr as unions, for clarity and bug resistance (no functional changes).

  • in wolfSSL_BIO_free(), add WOLFSSL_BIO_DGRAM to the test for closing bio->num.fd, fixing a descriptor leak.

  • use SOCKET_INVALID consistently as the invalid value for WOLFSSL_BIO.num.fd, and use SOCKET_T consistently as the internal type for file descriptors.

  • move the definitions for SOCKET_T and SOCKET_INVALID from wolfio.h to the filesystem section of wc_port.h, and allow override definitions of SOCKET_T.

detected and tested with wolfssl-multi-test.sh ... pq-hybrid-all-rpk-valgrind-unittest. also tested with wolfssl-multi-test.sh ... super-quick-check.

@douzzer douzzer force-pushed the 20240718-BIO_DGRAM-memory-leak branch 2 times, most recently from d6b4313 to 0582de7 Compare July 18, 2024 18:22
@douzzer douzzer requested a review from dgarske July 18, 2024 18:26
@douzzer douzzer force-pushed the 20240718-BIO_DGRAM-memory-leak branch from 0582de7 to 63d6f11 Compare July 18, 2024 18:50
@douzzer
Copy link
Contributor Author

douzzer commented Jul 18, 2024

retest this please

@SparkiDev SparkiDev self-assigned this Jul 19, 2024
SparkiDev
SparkiDev previously requested changes Jul 19, 2024
View reviewed changes
wolfssl/ssl.h Outdated
@@ -1753,15 +1753,15 @@ WOLFSSL_API int wolfSSL_BIO_get_md_ctx(WOLFSSL_BIO *bio,
WOLFSSL_API WOLFSSL_BIO_METHOD* wolfSSL_BIO_f_buffer(void);
WOLFSSL_API long wolfSSL_BIO_set_write_buffer_size(WOLFSSL_BIO* bio, long size);
WOLFSSL_API WOLFSSL_BIO_METHOD* wolfSSL_BIO_f_ssl(void);
WOLFSSL_API WOLFSSL_BIO* wolfSSL_BIO_new_socket(int sfd, int flag);
WOLFSSL_API WOLFSSL_BIO* wolfSSL_BIO_new_dgram(int fd, int closeF);
WOLFSSL_API WOLFSSL_BIO* wolfSSL_BIO_new_socket(SOCKET_T sfd, int flag);
Copy link
Contributor

@SparkiDev SparkiDev Jul 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Changing public API. And it doesn't match OpenSSL. (Yes OpenSSL is wrong.)
Make a new one that is SOCKET_T is my suggestion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not bothering with the new APIs for now -- may not in practice ever be needed.

in any case I've reverted all the changes to public APIs. now, all substantive header changes in the PR are in internal.h.

wolfssl/internal.h Outdated
@@ -2761,7 +2761,7 @@ struct WOLFSSL_BIO {
int wrIdx; /* current index for write buffer */
int rdIdx; /* current read index */
int readRq; /* read request */
int num; /* socket num or length */
SOCKET_T num; /* socket num or length */
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason we have to use num for socket?

Copy link
Member

Choose a reason for hiding this comment

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

num is used for the size in some BIO's and the file descriptors too. Not sure if SOCKET_T is the correct type to use for that. I would remove num and create new members for each use case to make it easier to identify what needs fixing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

after comparing notes with Juliusz, I've opted to refactor .num, and .ptr for completeness, as unions. it looks very nice now.

@SparkiDev SparkiDev assigned douzzer and unassigned SparkiDev Jul 19, 2024
julek-wolfssl
julek-wolfssl previously requested changes Jul 19, 2024
View reviewed changes
src/bio.c Outdated
@@ -1755,7 +1755,7 @@ int wolfSSL_BIO_reset(WOLFSSL_BIO *bio)
* @param close_flag BIO_NOCLOSE or BIO_CLOSE
* @return New BIO object or NULL on failure
*/
WOLFSSL_BIO *wolfSSL_BIO_new_fd(int fd, int close_flag)
WOLFSSL_BIO *wolfSSL_BIO_new_fd(SOCKET_T fd, int close_flag)
Copy link
Member

Choose a reason for hiding this comment

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

OpenSSL API takes int. Leave it as is and cast inside.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

✔️

src/bio.c Outdated
@@ -2311,7 +2311,7 @@ int wolfSSL_BIO_flush(WOLFSSL_BIO* bio)
}


WOLFSSL_BIO* wolfSSL_BIO_new_socket(int sfd, int closeF)
WOLFSSL_BIO* wolfSSL_BIO_new_socket(SOCKET_T sfd, int closeF)
Copy link
Member

Choose a reason for hiding this comment

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

Also int

Copy link
Contributor Author

Choose a reason for hiding this comment

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

✔️

src/bio.c Outdated
@@ -2337,7 +2337,7 @@ int wolfSSL_BIO_flush(WOLFSSL_BIO* bio)
}


WOLFSSL_BIO* wolfSSL_BIO_new_dgram(int fd, int closeF)
WOLFSSL_BIO* wolfSSL_BIO_new_dgram(SOCKET_T fd, int closeF)
Copy link
Member

Choose a reason for hiding this comment

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

Also int

Copy link
Contributor Author

Choose a reason for hiding this comment

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

✔️

src/bio.c Outdated
@@ -2772,7 +2772,7 @@ int wolfSSL_BIO_flush(WOLFSSL_BIO* bio)
}

#ifndef NO_FILESYSTEM
long wolfSSL_BIO_set_fd(WOLFSSL_BIO* b, int fd, int closeF)
long wolfSSL_BIO_set_fd(WOLFSSL_BIO* b, SOCKET_T fd, int closeF)
Copy link
Member

Choose a reason for hiding this comment

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

Also int

Copy link
Contributor Author

Choose a reason for hiding this comment

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

✔️

src/bio.c Outdated
@@ -3146,7 +3150,7 @@ int wolfSSL_BIO_set_ex_data(WOLFSSL_BIO *bio, int idx, void *data)
return WOLFSSL_FAILURE;
}

int wolfSSL_BIO_get_fd(WOLFSSL_BIO *bio, int* fd)
SOCKET_T wolfSSL_BIO_get_fd(WOLFSSL_BIO *bio, SOCKET_T* fd)
Copy link
Member

Choose a reason for hiding this comment

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

Also int

Copy link
Contributor Author

Choose a reason for hiding this comment

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

✔️

wolfssl/internal.h Outdated
@@ -2761,7 +2761,7 @@ struct WOLFSSL_BIO {
int wrIdx; /* current index for write buffer */
int rdIdx; /* current read index */
int readRq; /* read request */
int num; /* socket num or length */
SOCKET_T num; /* socket num or length */
Copy link
Member

Choose a reason for hiding this comment

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

num is used for the size in some BIO's and the file descriptors too. Not sure if SOCKET_T is the correct type to use for that. I would remove num and create new members for each use case to make it easier to identify what needs fixing.

@douzzer douzzer force-pushed the 20240718-BIO_DGRAM-memory-leak branch from 63d6f11 to 3192d19 Compare July 19, 2024 18:59
@douzzer douzzer assigned wolfSSL-Bot and unassigned douzzer Jul 19, 2024
@douzzer douzzer force-pushed the 20240718-BIO_DGRAM-memory-leak branch from 3192d19 to 3d26fd2 Compare July 19, 2024 19:18
@douzzer
src/bio.c and related:
787397b 
* refactor WOLFSSL_BIO.num and WOLFSSL_BIO.ptr as unions, for clarity and bug resistance (no functional changes).

* in wolfSSL_BIO_free(), add WOLFSSL_BIO_DGRAM to the test for closing bio->num.fd, fixing a descriptor leak.

* use SOCKET_INVALID consistently as the invalid value for WOLFSSL_BIO.num.fd, and use SOCKET_T consistently as the internal type for file descriptors.

* move the definitions for SOCKET_T and SOCKET_INVALID from wolfio.h to the filesystem section of wc_port.h, and allow override definitions of SOCKET_T.

detected and tested with wolfssl-multi-test.sh ... pq-hybrid-all-rpk-valgrind-unittest. also tested with wolfssl-multi-test.sh ... super-quick-check.
@douzzer douzzer force-pushed the 20240718-BIO_DGRAM-memory-leak branch from 3d26fd2 to 787397b Compare July 19, 2024 19:50
@dgarske dgarske dismissed stale reviews from julek-wolfssl and SparkiDev July 19, 2024 21:24

Fixed

@dgarske dgarske merged commit 4d8a6b8 into wolfSSL:master Jul 19, 2024
121 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dgarske dgarske dgarske approved these changes

@wolfSSL-Bot wolfSSL-Bot Awaiting requested review from wolfSSL-Bot

@SparkiDev SparkiDev Awaiting requested review from SparkiDev

@julek-wolfssl julek-wolfssl Awaiting requested review from julek-wolfssl

Assignees

@wolfSSL-Bot wolfSSL-Bot

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@douzzer @dgarske @SparkiDev @julek-wolfssl @wolfSSL-Bot