report: add missing locks for report_on_fatalerror accessors

[addaleax]

Overlooked in 2fa74e3.

Refs: #32207

Checklist

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

[cjihrig]

LGTM. I'm assuming the same thing should be done in #32497?

[nodejs-github-bot]

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

[gireeshpunathil]

will defining report_on_fatalerror as std::atomic<bool> be a better approach?

[addaleax]

@gireeshpunathil You could do that, but I doubt that it’s worth the extra complexity.

[nodejs-github-bot]

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

[sam-github]

This makes threads running js (workers and main) all get mutex on access via the js setter/getter, but don't all accesses to that value after the intial CLI parsing need protection?

Specifically,

node/src/node_errors.cc

Line 409 in accc984

if (per_process::cli_options->report_on_fatalerror) {

?

[sam-github]

Do Set/GetFipsCrypto have the same issues? I didn't do a thorough review, I just did some grepping for some of the other process scope options, and they appear to be subject to the same problem as the on_error.

@gireeshpunathil 's atomic suggestion seems a pretty good one for all bools, it makes it safer by shifting the responsibility to the variable to be safe, rather than to the users of the variable.

Atomic won't work for types that are not trivially copyable, though, so strings will need to use the mutex.

[addaleax]

Landed in e06512b

@sam-github If you want to switch these over to using atomics, I think that’s okay – most importantly, I’d like to be consistent, and I’d like to be explicit about using global mutable state when we’re doing it. And yes, the FIPS accessors look like they should also be protected by a mutex.

[sam-github]

@addaleax Did you see #32535 (comment)? Is no locking required on the other access?

Btw, I tried atomic bools, they aren't supported by the options parser, and adding an entire new specialization for the options parser didn't seem worth it. cc: @gireeshpunathil

[addaleax]

@sam-github Yeah, as I said to @gireeshpunathil – it doesn’t seem worth the extra complexity.

And yes, I think technically the other accesses need a mutex, too.

Thinking about it, #31717 introduced a mechanism for this that might actually help a lot here – wrapping the per-process options struct in ExclusiveAccess would make sure that we acquire a lock before we access it. If you or @gireeshpunathil are interested in implementing that, feel free to go ahead, otherwise I’d be up for it. (Or if you think it’s a terrible idea, let me know that too, of course 😅.)

[sam-github]

I like the #31717 approach, though I suspect it would break the options parsing even more thoroughly than atomics. On the plus side, it would also handle std::string (which can't be atomic), so that's a win.

[addaleax]

@sam-github To be clear, my suggestion is to wrap the entire struct in ExclusiveAccess, not individual members. It shouldn’t affect the options parser code, or at least only very small parts of it.