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.

Sign up for GitHub

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

Jump to bottom

crypto: add missing rand.h includes #38864

Closed
wants to merge 1 commit into from
Closed

crypto: add missing rand.h includes #38864

wants to merge 1 commit into from

Conversation

codebytere
Copy link
Member

@codebytere codebytere commented May 31, 2021 •
edited
Loading

Before #35093 this include was explicitly there but it was remove in the refactor - without this Electron fails to compile with:

../../third_party/electron_node/src/crypto/crypto_util.cc:65:18: error: use of undeclared identifier 'RAND_status'
    int status = RAND_status();
                 ^
../../third_party/electron_node/src/crypto/crypto_util.cc:71:9: error: use of undeclared identifier 'RAND_poll'
    if (RAND_poll() == 0)
        ^
../../third_party/electron_node/src/crypto/crypto_util.cc:82:10: error: use of undeclared identifier 'RAND_bytes'
  return RAND_bytes(buffer, length) != -1;
         ^
3 errors generated.
ninja: build stopped: subcommand failed.

If there's another preferred approach let me know!

cc @jasnell

@codebytere codebytere requested a review from jasnell May 31, 2021 10:25
@github-actions github-actions bot added c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. needs-ci PRs that need a full CI run. labels May 31, 2021
codebytere added a commit to electron/electron that referenced this pull request May 31, 2021
@codebytere
fix: add missing openssl/rand.h include
a3236a5 
Before nodejs/node#35093 this include was
explicitly there but it was remove in the refactor.

Upstreamed at nodejs/node#38864.
codebytere added a commit to electron/electron that referenced this pull request May 31, 2021
@codebytere
fix: add missing openssl/rand.h include
0152250 
Before nodejs/node#35093 this include was
explicitly there but it was remove in the refactor.

Upstreamed at nodejs/node#38864.
codebytere added a commit to electron/electron that referenced this pull request May 31, 2021
@codebytere
fix: add missing openssl/rand.h include
92bd5ac 
Before nodejs/node#35093 this include was
explicitly there but it was remove in the refactor.

Upstreamed at nodejs/node#38864.
@codebytere codebytere changed the title crypto: add missing rand.h include crypto: add missing rand.h includes May 31, 2021
codebytere added a commit to electron/electron that referenced this pull request May 31, 2021
@codebytere
fix: add missing openssl/rand.h include
a6ce5eb 
Before nodejs/node#35093 this include was
explicitly there but it was remove in the refactor.

Upstreamed at nodejs/node#38864.
@addaleax addaleax removed the needs-ci PRs that need a full CI run. label May 31, 2021
@nodejs-github-bot

This comment has been minimized.

Sign in to view
XadillaX
XadillaX approved these changes Jun 1, 2021
View reviewed changes
Copy link
Contributor

@XadillaX XadillaX left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I just wonder that where's #include <openssl/rand.h> in Node.js, or why Node.js can be compiled successfully.

@addaleax
Copy link
Member

addaleax commented Jun 1, 2021

@XadillaX Electron uses boringssl instead of openssl, so I think the headers might just be structured a bit differently there.

@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/38473/

@RaisinTen RaisinTen added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 3, 2021
codebytere added a commit that referenced this pull request Jun 3, 2021
@codebytere
crypto: add missing rand.h include
3b38757 
PR-URL: #38864
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Khaidi Chu <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
@codebytere
Copy link
Member Author

Landed in 3b38757

@codebytere codebytere closed this Jun 3, 2021
@codebytere codebytere deleted the add-missing-include branch June 3, 2021 14:15
codebytere added a commit to electron/electron that referenced this pull request Jun 8, 2021
@codebytere
fix: add missing openssl/rand.h include
55d3b72 
Before nodejs/node#35093 this include was
explicitly there but it was remove in the refactor.

Upstreamed at nodejs/node#38864.
codebytere added a commit to electron/electron that referenced this pull request Jun 9, 2021
@codebytere
fix: add missing openssl/rand.h include
f62c781 
Before nodejs/node#35093 this include was
explicitly there but it was remove in the refactor.

Upstreamed at nodejs/node#38864.
codebytere added a commit to electron/electron that referenced this pull request Jun 10, 2021
@codebytere
fix: add missing openssl/rand.h include
98b0d2f 
Before nodejs/node#35093 this include was
explicitly there but it was remove in the refactor.

Upstreamed at nodejs/node#38864.
targos pushed a commit that referenced this pull request Jun 11, 2021
@codebytere @targos
crypto: add missing rand.h include
82c2939 
PR-URL: #38864
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Khaidi Chu <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
@danielleadams danielleadams mentioned this pull request Jun 14, 2021
Merged
@richardlau
Copy link
Member

Since #35093 is semver-major this doesn't apply to v14.x so marking as dont-land.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RaisinTen RaisinTen RaisinTen approved these changes

@richardlau richardlau richardlau approved these changes

@tniessen tniessen tniessen approved these changes

@XadillaX XadillaX XadillaX approved these changes

@cjihrig cjihrig cjihrig approved these changes

@addaleax addaleax addaleax approved these changes

@jasnell jasnell Awaiting requested review from jasnell

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@codebytere @nodejs-github-bot @addaleax @richardlau @cjihrig @XadillaX @tniessen @RaisinTen