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

src: protect global state with mutexes #20542

Closed
wants to merge 2 commits into from
Closed

src: protect global state with mutexes #20542

wants to merge 2 commits into from

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented May 5, 2018 •
edited
Loading

Protect environment variables and inherently per-process state
(e.g. the process title) with mutexes, to better accomodate
Node’s usage in multi-threading environments.

Thanks to Stephen Belanger for reviewing this change in its original PR.

Refs: ayojs/ayo#82

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

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels May 5, 2018
@addaleax addaleax added embedding Issues and PRs related to embedding Node.js in another project. worker Issues and PRs related to Worker support. labels May 5, 2018
@addaleax
Copy link
Member Author

addaleax commented May 5, 2018

CI: https://ci.nodejs.org/job/node-test-commit/18248/

@mscdex
Copy link
Contributor

mscdex commented May 5, 2018

Typo in commit message: s/accomate/accommodate/

@addaleax
src: protect global state with mutexes
945c9fe 
Protect environment variables and inherently per-process state
(e.g. the process title) with mutexes, to better accomodate
Node’s usage in multi-threading environments.

Thanks to Stephen Belanger for reviewing this change in its original PR.

Refs: ayojs/ayo#82
@addaleax
Copy link
Member Author

addaleax commented May 5, 2018

@mscdex heh, thanks for catching – done 😄

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 5, 2018
@addaleax
Copy link
Member Author

addaleax commented May 5, 2018

FreeBSD re-run: https://ci.nodejs.org/job/node-test-commit-freebsd/17533/

bnoordhuis
bnoordhuis reviewed May 6, 2018
View reviewed changes
src/node.cc Outdated
@@ -2600,6 +2608,7 @@ static void GetLinkedBinding(const FunctionCallbackInfo<Value>& args) {

static void ProcessTitleGetter(Local<Name> property,
const PropertyCallbackInfo<Value>& info) {
Mutex::ScopedLock lock(process_title_mutex);
Copy link
Member

Choose a reason for hiding this comment

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

Not necessary, uv_get_process_title() and uv_set_process_title() already use locks internally.

(Perhaps uv_os_getenv(), uv_os_setenv() and uv_os_unsetenv() should do the same.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Done, thanks for pointing this out.

@addaleax
Copy link
Member Author

addaleax commented May 6, 2018

Fresh CI: https://ci.nodejs.org/job/node-test-commit/18267/

@mscdex
Copy link
Contributor

mscdex commented May 7, 2018

s/accomodate/accommodate/ in commit message

@addaleax
Copy link
Member Author

addaleax commented May 9, 2018

Landed in 2c52f65

@addaleax addaleax closed this May 9, 2018
@addaleax addaleax deleted the global-state-mutex branch May 9, 2018 16:27
addaleax added a commit that referenced this pull request May 9, 2018
@addaleax
src: protect global state with mutexes
2c52f65 
Protect environment variables and inherently per-process state with
mutexes, to better accommodate Node’s usage in
multi-threading environments.

Thanks to Stephen Belanger for reviewing this change in its original PR.

Refs: ayojs/ayo#82
PR-URL: #20542
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
targos pushed a commit that referenced this pull request May 12, 2018
@addaleax @targos
src: protect global state with mutexes
a89cc28 
Protect environment variables and inherently per-process state with
mutexes, to better accommodate Node’s usage in
multi-threading environments.

Thanks to Stephen Belanger for reviewing this change in its original PR.

Refs: ayojs/ayo#82
PR-URL: #20542
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@addaleax addaleax mentioned this pull request May 14, 2018
Merged
addaleax added a commit that referenced this pull request May 14, 2018
@addaleax
2018-05-15, Version 10.2.0 (Current), @???
3b86468 
Notable Changes:

* **addons**:
  - Fixed a memory leak for users of `AsyncResource` and N-API. (Michael Dawson) [#20668](#20668)
* **assert**:
  - The `error` parameter of `assert.throws()` can be an object containing regular expressions now. (Ruben Bridgewater) [#20485](#20485)
* **crypto**:
  - The `authTagLength` option has been made more flexible. (Tobias Nießen) [#20235](#20235), [#20039](#20039)
* **http**:
  - Handling of `close` and `aborted` events has been made more consistent. (Robert Nagy) [#20075](#20075), [#20611](#20611)
* Embedder support:
  - Functions for creating V8 `Isolate` and `Context` objects with Node.js-specific behaviour have been added to the API. (helloshuangzi) [#20639](#20639)
  - Node.js `Environment`s clean up resources before exiting now. (Anna Henningsen) [#19377](#19377)
  - Support for multi-threaded embedding has been improved. (Anna Henningsen) [#20542](#20542), [#20539](#20539), [#20541](#20541)
* **timers**:
  - `timeout.refresh()` has been added to the public API. (Jeremiah Senkpiel) [#20298](#20298)

PR-URL: #20724
addaleax added a commit that referenced this pull request May 15, 2018
@addaleax
2018-05-15, Version 10.2.0 (Current), @???
411ecf2 
Notable Changes:

* **addons**:
  - Fixed a memory leak for users of `AsyncResource` and N-API. (Michael Dawson) [#20668](#20668)
* **assert**:
  - The `error` parameter of `assert.throws()` can be an object containing regular expressions now. (Ruben Bridgewater) [#20485](#20485)
* **crypto**:
  - The `authTagLength` option has been made more flexible. (Tobias Nießen) [#20235](#20235), [#20039](#20039)
* **http**:
  - Handling of `close` and `aborted` events has been made more consistent. (Robert Nagy) [#20075](#20075), [#20611](#20611)
* Embedder support:
  - Functions for creating V8 `Isolate` and `Context` objects with Node.js-specific behaviour have been added to the API. (helloshuangzi) [#20639](#20639)
  - Node.js `Environment`s clean up resources before exiting now. (Anna Henningsen) [#19377](#19377)
  - Support for multi-threaded embedding has been improved. (Anna Henningsen) [#20542](#20542), [#20539](#20539), [#20541](#20541)
* **esm**:
  - Builtin modules (e.g. `fs`) now provide named exports in ES6 modules. (Gus Caplan) [#20403](#20403)
* **timers**:
  - `timeout.refresh()` has been added to the public API. (Jeremiah Senkpiel) [#20298](#20298)

PR-URL: #20724
addaleax added a commit that referenced this pull request May 15, 2018
@addaleax
2018-05-15, Version 10.2.0 (Current), @???
d0eec7b 
Notable Changes:

* **addons**:
  - Fixed a memory leak for users of `AsyncResource` and N-API. (Michael Dawson) [#20668](#20668)
* **assert**:
  - The `error` parameter of `assert.throws()` can be an object containing regular expressions now. (Ruben Bridgewater) [#20485](#20485)
* **crypto**:
  - The `authTagLength` option has been made more flexible. (Tobias Nießen) [#20235](#20235), [#20039](#20039)
* **http**:
  - Handling of `close` and `aborted` events has been made more consistent. (Robert Nagy) [#20075](#20075), [#20611](#20611)
* Embedder support:
  - Functions for creating V8 `Isolate` and `Context` objects with Node.js-specific behaviour have been added to the API. (Allen Yonghuang Wang) [#20639](#20639)
  - Node.js `Environment`s clean up resources before exiting now. (Anna Henningsen) [#19377](#19377)
  - Support for multi-threaded embedding has been improved. (Anna Henningsen) [#20542](#20542), [#20539](#20539), [#20541](#20541)
* **esm**:
  - Builtin modules (e.g. `fs`) now provide named exports in ES6 modules. (Gus Caplan) [#20403](#20403)
* **timers**:
  - `timeout.refresh()` has been added to the public API. (Jeremiah Senkpiel) [#20298](#20298)

PR-URL: #20724
MylesBorins pushed a commit that referenced this pull request May 22, 2018
@addaleax @MylesBorins
2018-05-23, Version 10.2.0 (Current)
d765fb6 
* addons:
  - Fixed a memory leak for users of `AsyncResource` and N-API.
    (Michael Dawson)
    #20668
* assert:
  - The `error` parameter of `assert.throws()` can be an object containing
    regular expressions now. (Ruben Bridgewater)
    #20485
* crypto:
  - The `authTagLength` option has been made more flexible (Tobias Nießen)
    #20235)
    #20039
* esm:
  - Builtin modules (e.g. `fs`) now provide named exports in ES6 modules.
    (Gus Caplan)
    #20403
* http:
  - Handling of `close` and `aborted` events has been made more consistent.
    (Robert Nagy)
    #20075
    #20611
* module:
  - add --preserve-symlinks-main (David Goldstein)
    #19911
* timers:
  - `timeout.refresh()` has been added to the public API.
    (Jeremiah Senkpiel)
    #20298
* Embedder support:
  - Functions for creating V8 `Isolate` and `Context` objects with
    Node.js-specific behaviour have been added to the API.
    (Allen Yonghuang Wang)
    #20639
  - Node.js `Environment`s clean up resources before exiting now.
    (Anna Henningsen)
    #19377
  - Support for multi-threaded embedding has been improved.
    (Anna Henningsen)
    #20542
    #20539
    #20541

PR-URL: #20724
MylesBorins pushed a commit that referenced this pull request May 23, 2018
@addaleax @MylesBorins
2018-05-23, Version 10.2.0 (Current)
f6b6b3f 
* addons:
  - Fixed a memory leak for users of `AsyncResource` and N-API.
    (Michael Dawson)
    #20668
* assert:
  - The `error` parameter of `assert.throws()` can be an object containing
    regular expressions now. (Ruben Bridgewater)
    #20485
* crypto:
  - The `authTagLength` option has been made more flexible (Tobias Nießen)
    #20235)
    #20039
* esm:
  - Builtin modules (e.g. `fs`) now provide named exports in ES6 modules.
    (Gus Caplan)
    #20403
* http:
  - Handling of `close` and `aborted` events has been made more consistent.
    (Robert Nagy)
    #20075
    #20611
* module:
  - add --preserve-symlinks-main (David Goldstein)
    #19911
* timers:
  - `timeout.refresh()` has been added to the public API.
    (Jeremiah Senkpiel)
    #20298
* Embedder support:
  - Functions for creating V8 `Isolate` and `Context` objects with
    Node.js-specific behaviour have been added to the API.
    (Allen Yonghuang Wang)
    #20639
  - Node.js `Environment`s clean up resources before exiting now.
    (Anna Henningsen)
    #19377
  - Support for multi-threaded embedding has been improved.
    (Anna Henningsen)
    #20542
    #20539
    #20541

PR-URL: #20724
MylesBorins pushed a commit that referenced this pull request May 23, 2018
@addaleax @MylesBorins
2018-05-23, Version 10.2.0 (Current)
5cbb905 
* addons:
  - Fixed a memory leak for users of `AsyncResource` and N-API.
    (Michael Dawson)
    #20668
* assert:
  - The `error` parameter of `assert.throws()` can be an object containing
    regular expressions now. (Ruben Bridgewater)
    #20485
* crypto:
  - The `authTagLength` option has been made more flexible (Tobias Nießen)
    #20235)
    #20039
* esm:
  - Builtin modules (e.g. `fs`) now provide named exports in ES6 modules.
    (Gus Caplan)
    #20403
* http:
  - Handling of `close` and `aborted` events has been made more consistent.
    (Robert Nagy)
    #20075
    #20611
* module:
  - add --preserve-symlinks-main (David Goldstein)
    #19911
* timers:
  - `timeout.refresh()` has been added to the public API.
    (Jeremiah Senkpiel)
    #20298
* Embedder support:
  - Functions for creating V8 `Isolate` and `Context` objects with
    Node.js-specific behaviour have been added to the API.
    (Allen Yonghuang Wang)
    #20639
  - Node.js `Environment`s clean up resources before exiting now.
    (Anna Henningsen)
    #19377
  - Support for multi-threaded embedding has been improved.
    (Anna Henningsen)
    #20542
    #20539
    #20541

PR-URL: #20724
MylesBorins pushed a commit that referenced this pull request May 23, 2018
@addaleax @MylesBorins
2018-05-23, Version 10.2.0 (Current)
82f1811 
* addons:
  - Fixed a memory leak for users of `AsyncResource` and N-API.
    (Michael Dawson)
    #20668
* assert:
  - The `error` parameter of `assert.throws()` can be an object containing
    regular expressions now. (Ruben Bridgewater)
    #20485
* crypto:
  - The `authTagLength` option has been made more flexible (Tobias Nießen)
    #20235)
    #20039
* esm:
  - Builtin modules (e.g. `fs`) now provide named exports in ES6 modules.
    (Gus Caplan)
    #20403
* http:
  - Handling of `close` and `aborted` events has been made more consistent.
    (Robert Nagy)
    #20075
    #20611
* module:
  - add --preserve-symlinks-main (David Goldstein)
    #19911
* timers:
  - `timeout.refresh()` has been added to the public API.
    (Jeremiah Senkpiel)
    #20298
* Embedder support:
  - Functions for creating V8 `Isolate` and `Context` objects with
    Node.js-specific behaviour have been added to the API.
    (Allen Yonghuang Wang)
    #20639
  - Node.js `Environment`s clean up resources before exiting now.
    (Anna Henningsen)
    #19377
  - Support for multi-threaded embedding has been improved.
    (Anna Henningsen)
    #20542
    #20539
    #20541

PR-URL: #20724
@ebickle
Copy link
Contributor

ebickle commented Mar 16, 2020 •
edited
Loading

@addaleax To you recall why the mutex was added to NewRootCertStore() in node_crypto.cc? I can only find two main code paths into it - one being the SecureContext functions via JavaScript and from Start/InitializeOncePerProcess in node.cc. All appear to be synchronous.

Wanted to make sure the Mutex isn't actually needed before making other changes in that space. It was a bit surprising to see the thread synchronization primitive there when the rest of crypto (being fairly security-sensitive) lacks it.

Apologies for commenting on a two year old PR; figured tossing it here in the GitHub history would help in case anyone else stumbled across this and had the same question 😀

@cjihrig
Copy link
Contributor

cjihrig commented Mar 16, 2020

one being the SecureContext functions via JavaScript

Doesn't that mean that two Workers could modify it at the same time?

@addaleax
Copy link
Member Author

@ebickle @cjihrig was faster than me – yes, this basically is meant to protect against Worker threads accessing this simultaneously and leading to data corruption.

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

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@jasnell jasnell jasnell approved these changes

@devsnek devsnek devsnek approved these changes

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++. embedding Issues and PRs related to embedding Node.js in another project. lib / src Issues and PRs related to general changes in the lib or src directory. worker Issues and PRs related to Worker support.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@addaleax @mscdex @ebickle @cjihrig @bnoordhuis @jasnell @devsnek @nodejs-github-bot