Skip to content

Conversation

@codebytere
Copy link
Member

This PR fixes an incompatibility between BoringSSL and OpenSSL in compilation through Electron.

OpenSSL has the signature

int EVP_PKEY_CTX_set0_rsa_oaep_label(EVP_PKEY_CTX *ctx, unsigned char *label, int len); 

whereas BoringSSL has:

int EVP_PKEY_CTX_set0_rsa_oaep_label(EVP_PKEY_CTX *ctx, uint8_t *label, size_t label_len); 

Changing this in BoringSSL would not have the correct effect, since label (as returned by OpenSSL_memdup) is a void* & C++ doesn't cast from void* to T* without an explicit cast. OpenSSL has a lot of functions/macros that aren't typesafe which means we don't get the usual C++ type-checking and so this fails to compile without this patch.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-botnodejs-github-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. labels Dec 12, 2019
@davidben
Copy link
Contributor

davidben commented Dec 12, 2019

Correction: this doesn't have anything to do with those signatures. We required unsigned char and uint8_t to be the same type, and it is true in every platform I'm aware of. The signatures are identical.

Rather it's that OpenSSL doesn't use any type signature at all. It's documented with that signature, but it's really a macro without any type-checking. The cast is needed to conform Node to the OpenSSL's documentation because C++ does not implicitly cast void*.

@nodejs-github-bot

This comment has been minimized.

@addaleaxaddaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 12, 2019
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

codebytere added a commit that referenced this pull request Dec 14, 2019
OpenSSL uses a macro without typechecking; since C++ does not implicitly cast void* this is needed to conform Node.js to the OpenSSL documentation. PR-URL: #30917 Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Rich Trott <[email protected]>
@codebytere
Copy link
MemberAuthor

Landed in cb76f27.

@codebyterecodebytere deleted the cast-label branch December 14, 2019 18:55
MylesBorins pushed a commit that referenced this pull request Dec 17, 2019
OpenSSL uses a macro without typechecking; since C++ does not implicitly cast void* this is needed to conform Node.js to the OpenSSL documentation. PR-URL: #30917 Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Rich Trott <[email protected]>
@MylesBorinsMylesBorins mentioned this pull request Dec 17, 2019
targos pushed a commit that referenced this pull request Jan 14, 2020
OpenSSL uses a macro without typechecking; since C++ does not implicitly cast void* this is needed to conform Node.js to the OpenSSL documentation. PR-URL: #30917 Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Rich Trott <[email protected]>
@targostargos mentioned this pull request Jan 15, 2020
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
OpenSSL uses a macro without typechecking; since C++ does not implicitly cast void* this is needed to conform Node.js to the OpenSSL documentation. PR-URL: #30917 Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Rich Trott <[email protected]>
@MylesBorinsMylesBorins mentioned this pull request Feb 8, 2020
codebytere added a commit to electron/electron that referenced this pull request Feb 12, 2020
codebytere added a commit to electron/electron that referenced this pull request Feb 12, 2020
codebytere added a commit to electron/electron that referenced this pull request Feb 15, 2020
codebytere added a commit to electron/electron that referenced this pull request Feb 18, 2020
codebytere added a commit to electron/electron that referenced this pull request Feb 21, 2020
codebytere added a commit to electron/electron that referenced this pull request Feb 21, 2020
codebytere added a commit to electron/electron that referenced this pull request Feb 24, 2020
codebytere added a commit to electron/electron that referenced this pull request Feb 24, 2020
* chore: bump node in DEPS to v12.16.0 * Fixup asar support setup patch nodejs/node#30862 * Fixup InternalCallbackScope patch nodejs/node#30236 * Fixup GN buildfiles patch nodejs/node#30755 * Fixup low-level hooks patch nodejs/node#30466 * Fixup globals require patch nodejs/node#31643 * Fixup process stream patch nodejs/node#30862 * Fixup js2c modification patch nodejs/node#30755 * Fixup internal fs override patch nodejs/node#30610 * Fixup context-aware warn patch nodejs/node#30336 * Fixup Node.js with ltcg config nodejs/node#29388 * Fixup oaepLabel patch nodejs/node#30917 * Remove redundant ESM test patch nodejs/node#30997 * Remove redundant cli flag patch nodejs/node#30466 * Update filenames.json * Remove macro generation in GN build files nodejs/node#30755 * Fix some compilation errors upstream * Add uvwasi to deps nodejs/node#30258 * Fix BoringSSL incompatibilities * Fixup linked module patch nodejs/node#30274 * Add missing sources to GN uv build libuv/libuv#2347 * Patch some uvwasi incompatibilities * chore: bump Node.js to v12.6.1 * Remove mark_arraybuffer_as_untransferable.patch nodejs/node#30549 * Fix uvwasi build failure on win * Fixup --perf-prof cli option error * Fixup early cjs module loading * fix: initialize diagnostics properly nodejs/node#30025 * Disable new esm syntax specs nodejs/node#30219 * Fixup v8 weakref hook spec nodejs/node#29874 * Fix async context timer issue * Disable monkey-patch-main spec It relies on nodejs/node#29777, and we don't override prepareStackTrace. * Disable new tls specs nodejs/node#23188 We don't support much of TLS owing to schisms between BoringSSL and OpenSSL. Co-authored-by: Shelley Vohr <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author readyPRs 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++.cryptoIssues and PRs related to the crypto subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

@codebytere@davidben@nodejs-github-bot@Trott@addaleax@cjihrig@tniessen@richardlau