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

AssertionError in TLS module #22618

Closed
tjconcept opened this issue Aug 31, 2018 · 6 comments
Closed

AssertionError in TLS module #22618

tjconcept opened this issue Aug 31, 2018 · 6 comments
Labels
tls Issues and PRs related to the tls subsystem.

Comments

@tjconcept
Copy link
Contributor

tjconcept commented Aug 31, 2018

assert.js:269
    throw err;
    ^

AssertionError [ERR_ASSERTION]: false == true
    at TLSWrap.onhandshakestart (_tls_wrap.js:66:3)

This happened identically on two processes running for several weeks. They had a mutual TLS connection over localhost (using the tls module) with regular data between the two. I do not believe any new code paths in the code was triggered, so I assume the bug is timing or leaking related. I can't immediately reproduce it.

I'm sorry I can't provide more details, but at least this is a signal that there's something here.

@apapirovski
Copy link
Member

Hi @tjconcept — you'll need to upgrade to 10.8.0 or 10.9.0. There was a bug in earlier versions related to integer overflow.

@addaleax addaleax added the tls Issues and PRs related to the tls subsystem. label Aug 31, 2018
@addaleax
Copy link
Member

addaleax commented Aug 31, 2018

There’s a very good chance this was fixed by #22214 in v10.9.0.

Edit: Oops, @apapirovski’s comment did not show. Anyway, I agree. 😄

addaleax added a commit to addaleax/node that referenced this issue Aug 31, 2018
@tjconcept
Copy link
Contributor Author

I upgraded straight away and restarted, so I'll just be looking out for this.

Does this also explain why both processes crashed simultaneously? I'm slightly worried what that might mean to a cluster running this way..

What's the proper process for this report?

addaleax added a commit that referenced this issue Sep 3, 2018
Refs: #22618

PR-URL: #22625
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@addaleax
Copy link
Member

addaleax commented Sep 3, 2018

Does this also explain why both processes crashed simultaneously?

@tjconcept The issue would usually occur after 2^31 milliseconds after the process started, or 24.855 days. The processes probably did not fail exactly simultaneously, though.

@tjconcept
Copy link
Contributor Author

Thanks. It ran for 37 days in both instances. Both had the TLS connection open from the beginning.

addaleax added a commit that referenced this issue Sep 3, 2018
Refs: #22618

PR-URL: #22625
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@apapirovski
Copy link
Member

Does this also explain why both processes crashed simultaneously?

Since they had a mutual TLS connection, when the handshake renewal was initiated, the assertion got triggered on both and that led to the crash. That code path only runs in timers (where it wouldn't crash) and in TLS, where it would.

I'm going to close this out since there's nothing else that can be done but feel free to reopen if you feel like we haven't addressed sufficiently.

targos pushed a commit that referenced this issue Sep 5, 2018
Refs: #22618

PR-URL: #22625
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
targos pushed a commit that referenced this issue Sep 6, 2018
Refs: #22618

PR-URL: #22625
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

No branches or pull requests

3 participants