Fix HTTPS pooled client crash on unexpected SSL EOF (#3307)#3316
Fix HTTPS pooled client crash on unexpected SSL EOF (#3307)#3316Felix-Gong wants to merge 2 commits into
Conversation
| BIO_fd_non_fatal_error(saved_errno) != 0 || | ||
| nr < 0; | ||
| PLOG_IF(WARNING, is_fatal_error) << "Fail to read from ssl_fd=" << fd(); | ||
| if (is_fatal_error) { |
There was a problem hiding this comment.
This branch is changing errno from saved_errno to ESSL, which is a minor semantic improvement but not a bug fix.
There was a problem hiding this comment.
If you change this behavior, you'd better check the callers' code and make sure they don't depend on the value of errno.
There was a problem hiding this comment.
The caller only checks errno == EAGAIN and errno == EINTR for retry logic. ESSL matches neither, so control flow is unchanged — it still enters SetFailed().
The only difference is the error message. But I'll revert this change to keep the fix minimal.
Felix-Gong
force-pushed
the
fix-https-pooled-error-code
branch
from
9b8eff6 to
f11c336
Compare
|
BTW, Can you add some unit tests to verify this fix? |
When OpenSSL 3.x detects unexpected EOF (peer closed without close_notify), SSL_read returns 0 with SSL_ERROR_SSL. The code didn't return -1, causing error_code=0 to propagate to Controller::SetFailed() which triggers CHECK(false). Fix by returning -1 with errno=ESSL when SSL errors are detected in DoRead(), instead of falling through and returning nr. Discovered during RISC-V porting and integration testing. Fixes apache#3307 Signed-off-by: Felix Gong <gongxiaofei24@iscas.ac.cn>
Felix-Gong
force-pushed
the
fix-https-pooled-error-code
branch
from
f11c336 to
7a1dfe7
Compare
I'll think about how to add a test that covers the full DoRead() path for the unexpected SSL EOF scenario, and submit it later. |
@wwbmmm I have added unit test |
|
I think you should add the unit test |
I'd prefer to keep the test in a separate PR (#3321) because the fix and the test are independent changes. This way the fix can be reviewed and merged first, and the test PR can be rebased to verify CI passes. If you think it's better to combine them, I can merge them into one PR. |
I think it would be more appropriate to merge them into one PR. |
Test that Socket::DoRead() returns -1 with errno=ESSL when the remote side closes the TCP connection without sending close_notify, verifying the fix in commit 7a1dfe7.
|
Done. The unit test |
3 participants
Summary
Root Cause
When OpenSSL 3.x detects unexpected EOF (peer closed without close_notify), SSL_read returns 0 with SSL_ERROR_SSL. The code didn't return -1, causing error_code=0 to propagate to Controller::SetFailed() which triggers CHECK(false).
Fix
In the default branch of DoRead(), return -1 with errno=ESSL when SSL errors are detected, instead of falling through and returning nr.
Fixes #3307