Fix minor time-of-check vs time-of-use pointer issues

[kokke]

This PR addresses two minor issues (nits):

1 : A time-of-use/time-of-check 'bug' in src/crypto/crypto_context.cc

The pointer 'env' is checked against NULL on line 1117, but it gets dereferenced at line 1101+1102.
Note comments for line 1101, 1102 and 1117.

1089    int SecureContext::TicketKeyCallback(SSL* ssl,
1090                                         unsigned char* name,
1091                                         unsigned char* iv,
1092                                         EVP_CIPHER_CTX* ectx,
1093                                         HMAC_CTX* hctx,
1094                                         int enc) {
1095      static const int kTicketPartSize = 16;
1096    
1097      SecureContext* sc = static_cast<SecureContext*>(
1098          SSL_CTX_get_app_data(SSL_get_SSL_CTX(ssl)));
1099    
1100      Environment* env = sc->env();
1101      HandleScope handle_scope(env->isolate());
                                  ^^^^^^^^^^^^^^^^ 'env' gets dereferenced here
1102      Context::Scope context_scope(env->context());
                                      ^^^^^^^^^^^^^^^^ 'env' gets dereferenced here
1103    
1104      Local<Value> argv[3];
1105    
1106      if (!Buffer::Copy(
1107              env,
1108              reinterpret_cast<char*>(name),
1109              kTicketPartSize).ToLocal(&argv[0]) ||
1110          !Buffer::Copy(
1111              env,
1112              reinterpret_cast<char*>(iv),
1113              kTicketPartSize).ToLocal(&argv[1])) {
1114        return -1;
1115      }
1116    
1117      argv[2] = env != 0 ? v8::True(env->isolate()) : v8::False(env->isolate());
                                       ^^^^^^^^^^^^^^^^            ^^^^^^^^^^^^^^^^
                   ^^^^^^^^^^               'env' is dereferenced no matter if 'env != 0'
                        'env' is checked against NULL, but it was already dereferenced
1118                                        

Suggestion: Skip the null-check on line 1117. If 'env' could be NULL, the code would have segfaulted before reaching line 1117 anyway.

---

and 2 : A time-of-check vs time-of-use bug in src/udp_wrap.cc:370

The pointer 'wrap' is checked against NULL on line 376, but it gets dereferenced at line 370.
Note comments for line 370 and 376.

367   #define X(name, fn)                                                            \
368     void UDPWrap::name(const FunctionCallbackInfo<Value>& args) {                \
369       UDPWrap* wrap = Unwrap<UDPWrap>(args.Holder());                            \
370       Environment* env = wrap->env();                                            \
                            ^^^^^^^^^^^^^^ 'wrap' gets dereferenced here
371       CHECK_EQ(args.Length(), 1);                                                \
372       int flag;                                                                  \
373       if (!args[0]->Int32Value(env->context()).To(&flag)) {                      \
374         return;                                                                  \
375       }                                                                          \
376       int err = wrap == nullptr ? UV_EBADF : fn(&wrap->handle_, flag);           \
                   ^^^^^^^^^^^^^^^^^^ 'wrap' is checked against 'nullptr' here
377       args.GetReturnValue().Set(err);                                            \
378     }

Suggested solutions:

1. Skip the null-check on line 376. If 'wrap' could be NULL, the code would have segfaulted.
2. Add a null-check before the code at line 370

So the code becomes either:

1. with skipped null-check

367   #define X(name, fn)                                                            \
368     void UDPWrap::name(const FunctionCallbackInfo<Value>& args) {                \
369       UDPWrap* wrap = Unwrap<UDPWrap>(args.Holder());                            \
370       Environment* env = wrap->env();                                            \
371       CHECK_EQ(args.Length(), 1);                                                \
372       int flag;                                                                  \
373       if (!args[0]->Int32Value(env->context()).To(&flag)) {                      \
374         return;                                                                  \
375       }                                                                          \
376       int err = fn(&wrap->handle_, flag);                                        \
377       args.GetReturnValue().Set(err);                                            \
378     }

... or

2. with an added null-check, something like this

367   #define X(name, fn)                                                            \
368     void UDPWrap::name(const FunctionCallbackInfo<Value>& args) {                \
369       UDPWrap* wrap = Unwrap<UDPWrap>(args.Holder());                            \
370       if (wrap == nullptr) {                                                     \
371         args.GetReturnValue().Set(err);                                          \
372         return;                                                                  \
373       }                                                                          \
374       Environment* env = wrap->env();                                            \
375       CHECK_EQ(args.Length(), 1);                                                \
376       int flag;                                                                  \
377       if (!args[0]->Int32Value(env->context()).To(&flag)) {                      \
378         return;                                                                  \
379       }                                                                          \
380       int err = fn(&wrap->handle_, flag);                                        \
381       args.GetReturnValue().Set(err);                                            \
382     }

For context: These two issues were found using a homemade static analysis tool that flags instances where pointers are checked against NULL after they have already been dereferenced.

[kokke]

Aw, the asan-test failed because of a timeout :( Can it be re-run without me doing a new push?

=== release test-cluster-primary-kill ===
Path: parallel/test-cluster-primary-kill
Error: Command: out/Release/node /home/runner/work/node/node/test/parallel/test-cluster-primary-kill.js
--- TIMEOUT ---

===
=== 1 tests failed
===
make[1]: *** [Makefile:513: test-ci] Error 1
make: *** [Makefile:542: run-ci] Error 2
Error: Process completed with exit code 2.

[targos]

@kokke would you like to update the PR?

[kokke]

Hi @targos - sorry for keeping you guys waiting!

I assumed whomever approved the PR, would incorporate the changes proposed by @addaleax

I have pushed those to my fork now, so I think the PR is ready to be merged (once tests complete).

Please let me know if I have misunderstood or need to do anything else to satisfy the process.

[kokke]

@targos I've updated the PR - thanks for your patience with me :)

[targos]

Thanks!

[nodejs-github-bot]

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

[github-actions]

Landed in afb4ad6...4f3eda6