Skip to content

Conversation

@tniessen
Copy link
Member

@tniessentniessen commented Feb 3, 2021

It's difficult to find precise conditions that would prevent infinite loops here and that can be checked without a significant performance impact.

Passing parameters that cause an infinite loop within OpenSSL will permanently block the application thread when in sync mode, or permanently disable one thread in the libuv thread pool when in async mode. Especially the latter behavior is hard to debug, and throwing reasonable errors makes that much easier.

These conditions prevent cases that will, with high probability, result in infinite loops within OpenSSL. In cases where OpenSSL does not get stuck in an infinite loop, the parameters that match these conditions would not result in randomly generated primes.

This is a fast best-effort approach that doesn't require additional BIGNUM allocations.

These conditions do not prevent all infinite loops.

@tniessentniessen added crypto Issues and PRs related to the crypto subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. labels Feb 3, 2021
@tniessentniessenforce-pushed the crypto-prime-avoid-infinite-loops branch from 73aa358 to cd94327CompareFebruary 3, 2021 17:49
@benjamingr
Copy link
Member

Just write a program that takes another program as input and returns whether or not that program with the given parameter contains an infinite loop. That sounds like a reasonable and easy task :D

Copy link
Member

@benjamingrbenjamingr left a comment

Choose a reason for hiding this comment

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

Good comments!

@tniessen
Copy link
MemberAuthor

Thank you @benjamingr!

Just write a program that takes another program as input and returns whether or not that program with the given parameter contains an infinite loop. That sounds like a reasonable and easy task :D

Oh, absolutely, just give me a couple of hours to disprove Turing! :P

Copy link
Member

@jasnelljasnell left a comment

Choose a reason for hiding this comment

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

Good catch.

@nodejs-github-bot

This comment has been minimized.

@tniessentniessenforce-pushed the crypto-prime-avoid-infinite-loops branch from a188e82 to d35d516CompareFebruary 5, 2021 16:37
@nodejs-github-bot
Copy link
Collaborator

@tniessentniessen added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 5, 2021
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@Trott
Copy link
Member

Trott commented Feb 9, 2021

Landed in fdd7a87...6e804a9

Trott pushed a commit to tniessen/node that referenced this pull request Feb 9, 2021
PR-URL: nodejs#37212 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]>
@TrottTrott closed this Feb 9, 2021
@TrottTrottforce-pushed the crypto-prime-avoid-infinite-loops branch from d35d516 to 6e804a9CompareFebruary 9, 2021 00:49
@TrottTrott merged commit 6e804a9 into nodejs:masterFeb 9, 2021
danielleadams pushed a commit that referenced this pull request Feb 16, 2021
PR-URL: #37212 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]>
danielleadams pushed a commit that referenced this pull request Feb 16, 2021
PR-URL: #37212 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]>
This was referenced Feb 16, 2021
@tniessentniessen deleted the crypto-prime-avoid-infinite-loops branch February 21, 2021 11:57
@tniessentniessen removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 21, 2021
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++Issues and PRs that require attention from people who are familiar with C++.cryptoIssues and PRs related to the crypto subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@tniessen@benjamingr@nodejs-github-bot@Trott@jasnell@targos