net: validate non-string host for socket.connect

[daeyeon]

This fixes another issue discovered while reviewing #57112.

Internally, socket.connect checks the host string, implicitly coercing an array to a string in the process. That leads to the error below.

import net from 'node:net';

net.createConnection({
  host: ['192.168.0.1'],
  port: 8080,
});

#  node[2568546]: static void node::TCPWrap::Connect(const v8::FunctionCallbackInfo<v8::Value>&, std::function<int(const char*, T*)>) [with T = sockaddr_in] at ../src/tcp_wrap.cc:325
#  Assertion failed: args[1]->IsString()

----- Native stack trace -----

 1: 0x102d7b7 node::Assert(node::AssertionInfo const&) [node]
 2: 0x11aa8c2 void node::TCPWrap::Connect<sockaddr_in>(v8::FunctionCallbackInfo<v8::Value> const&, std::function<int (char const*, sockaddr_in*)>) [node]
 3: 0x11a9a73 node::TCPWrap::Connect(v8::FunctionCallbackInfo<v8::Value> const&) [node]
 4: 0x7f5b17e0f186

----- JavaScript stack trace -----

1: internalConnect (node:net:1096:26)
2: defaultTriggerAsyncIdScope (node:internal/async_hooks:464:18)
3: node:net:1353:9
4: processTicksAndRejections (node:internal/process/task_queues:85:11)

Signed-off-by: Daeyeon Jeong [email protected]

[nodejs-github-bot]

Review requested:

• @nodejs/net

[anonrig]

Why don't we just validate this on C++? We could just replace the IsString() assertion.

[daeyeon]

Good point, but since the net module has JS layer, validating it in JS seems better for performance. Plus, since other validations are handled in this function, keeping this pattern feels more cohesive.

[anonrig]

How come validating it on JS is better? I'm not following.

[daeyeon]

For simple string validations, JS is typically faster since it avoids crossing the JS/C++ boundary. For complex cases, C++ might be more efficient. Let me know if I'm missing anything.

[anonrig]

Right now you are adding a branch to both happy and bad path in the benefit of improving bad path (invalid input).

In terms of performance, validating at the C++ and removing that assertion should be the most optimum solution.

[daeyeon]

Ah, I get it now. The function where the assertion occurs is also used internally in places other than socket.connect. In those cases, IsString() assertion seems more appropriate than throwing an exception with option.host. I'll look into it further.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.31%. Comparing base (85c0f7a) to head (7a7ae44).

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #57198      +/-   ##
==========================================
- Coverage   90.33%   90.31%   -0.02%     
==========================================
  Files         630      630              
  Lines      184513   184515       +2     
  Branches    36076    36067       -9     
==========================================
- Hits       166674   166650      -24     
- Misses      10953    10961       +8     
- Partials     6886     6904      +18     

Files with missing lines	Coverage Δ
lib/net.js	94.84% <100.00%> (+<0.01%)	⬆️

... and 23 files with indirect coverage changes