tls: prevent multiple connection errors #23636
Conversation
|
Can you explain why this fixes the issue, or maybe add a regression test? It seems odd to change streams state if this is specific to TLS… |
|
The problem is that two errors are emitted - one from JS and one from OpenSSL. The stream state thing is already used throughout the latter code path. As for the test, I modified the code from #23631 as a regression test, but hitting 127.0.0.1 didn't reproduce the error. I'd have to take a closer look at EDIT: Although, instead of modifying |
|
I see … I guess this doesn’t make things worse, but it also looks like something we should definitely clean up at some point, I don’t think we should be using |
|
@addaleax pushed a second commit as an alternative to the first. It gets rid of the |
|
I've tried a couple things but haven't had any luck porting the one liner from #23631 to a runnable test case. Maybe someone who knows more about OpenSSL (@bnoordhuis, @tniessen ?) has some pointers on creating a bad Node server that generates: |
cjihrig
force-pushed
the
23631
branch
2 times, most recently
from
487d81b
to
ef41ba3
Compare
onConnectEnd(), which is called by TLSSocket, has a guard to prevent being called multiple times, but it does not prevent the OpenSSL error handler from being called, leading to multiple error events. This commit adds that piece of missing logic. PR-URL: nodejs#23636 Fixes: nodejs#23631 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Wyatt Preul <[email protected]> Reviewed-By: James M Snell <[email protected]>
onConnectEnd(), which is called by TLSSocket, has a guard to prevent being called multiple times, but it does not prevent the OpenSSL error handler from being called, leading to multiple error events. This commit adds that piece of missing logic. PR-URL: #23636 Fixes: #23631 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Wyatt Preul <[email protected]> Reviewed-By: James M Snell <[email protected]>
onConnectEnd(), which is called by TLSSocket, has a guard to prevent being called multiple times, but it does not prevent the OpenSSL error handler from being called, leading to multiple error events. This commit adds that piece of missing logic. PR-URL: #23636 Fixes: #23631 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Wyatt Preul <[email protected]> Reviewed-By: James M Snell <[email protected]>
onConnectEnd(), which is called by TLSSocket, has a guard to prevent being called multiple times, but it does not prevent the OpenSSL error handler from being called, leading to multiple error events. This commit adds that piece of missing logic. PR-URL: #23636 Fixes: #23631 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Wyatt Preul <[email protected]> Reviewed-By: James M Snell <[email protected]>
onConnectEnd(), which is called by TLSSocket, has a guard to prevent being called multiple times, but it does not prevent the OpenSSL error handler from being called, leading to multiple error events. This commit adds that piece of missing logic. PR-URL: #23636 Fixes: #23631 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Wyatt Preul <[email protected]> Reviewed-By: James M Snell <[email protected]>
onConnectEnd(), which is called by TLSSocket, has a guard to prevent being called multiple times, but it does not prevent the OpenSSL error handler from being called, leading to multiple error events. This commit adds that piece of missing logic. PR-URL: #23636 Fixes: #23631 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Wyatt Preul <[email protected]> Reviewed-By: James M Snell <[email protected]>
onConnectEnd(), which is called byTLSSocket, has a guard to prevent being called multiple times, but it does not prevent the OpenSSL error handler from being called, leading to multipleerrorevents. This commit adds that piece of missing logic.Fixes: #23631
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesCI: https://ci.nodejs.org/job/node-test-pull-request/17937/