Skip to content

fix: BIO_set_retry_write when BIO_CTRL_FLUSH to allow writer returns WouldBlock on flush#1922

Open
ihciah wants to merge 1 commit intorust-openssl:masterfrom
ihciah:master
Open

fix: BIO_set_retry_write when BIO_CTRL_FLUSH to allow writer returns WouldBlock on flush#1922
ihciah wants to merge 1 commit intorust-openssl:masterfrom
ihciah:master

Conversation

@ihciah
Copy link

@ihciah ihciah commented May 10, 2023

This PR fixes a BIO_flush related issue. According to openssl doc(link):

BIO_flush(), because it can write data may return 0 or -1 indicating that the call should be retried later in a similar manner to BIO_write(). The BIO_should_retry() call should be used and appropriate action taken is the call fails.

We have to call BIO_set_retry_write on retriable_error like WouldBlock. Otherwise, when the flush returns WouldBlock, it will fail.

Since we usually wrap openssl with a TcpStream, there is no buffer inside and flush always returns Ok(()), this issue is not obvious. However, if users use a buffered io, the flush support will be essential.

However, only fixes rust-openssl is not enough. I found a problem in openssl that makes this change not working. The issue can be found at openssl/openssl#20919 and I will try to fix it.

@ihciah
Copy link
Author

ihciah commented May 10, 2023

As a memo:
To check the feature, clone the forked openssl repo, compile it and inject envs:

env::set_var("OPENSSL_LIB_DIR", "/some/path/openssl");
env::set_var("OPENSSL_INCLUDE_DIR", "/some/path/openssl/include");
env::set_var("OPENSSL_STATIC", "1");

@sfackler
Copy link
Collaborator

Could you add a test for this change?

@ihciah
Copy link
Author

ihciah commented May 12, 2023

Could you add a test for this change?

Sure. I've added a new test for it. But since openssl still has bug(openssl/openssl#20919) on flush, to make the the test pass, I only tested read and write.

Once the openssl issue solved, change first_flush to true(L563) and the flush retriable can be tested.

@ihciah
Copy link
Author

ihciah commented May 25, 2023

Hi! Just a remind. @sfackler

@sfackler
Copy link
Collaborator

I will look at this once the upstream change lands so it can actually be tested.

@ihciah
Copy link
Author

ihciah commented Aug 16, 2023

Hi! @sfackler
The upstream fix is merged: openssl/openssl#21298

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.

2 participants