http2.connect mishandles the socket option when connecting to an HTTPS server

[murgatroid99]

• Version: 12.10.0
• Platform: Linux DESKTOP-OKC3QBQ 4.4.0-18362-Microsoft doc: fixed punctuation #476-Microsoft Fri Nov 01 16:53:00 PST 2019 x86_64 x86_64 x86_64 GNU/Linux
• Subsystem: http2

What steps will reproduce the bug?

Call http2.connect with an 'https:' authority passing the socket tls.connect option with a net.Socket that has already finished. This snippet demonstrates:

const http2 = require('http2');
const net = require('net');

const {
  HTTP2_HEADER_PATH,
  HTTP2_HEADER_STATUS
} = http2.constants;

// Create a normal session, as a control case
http2.connect('https://google.com', (clientSession) => {
  const req = clientSession.request({ [HTTP2_HEADER_PATH]: '/' });
  req.on('response', (headers) => {
    console.log('Response 1:', headers[HTTP2_HEADER_STATUS]);
    req.on('data', (chunk) => { });
    req.on('end', () => { 
      clientSession.close()
    });
  });
});

// Create a session using a socket that has finished connecting
const socket = net.connect(443, 'google.com', () => {
  http2.connect('https://google.com', {socket}, (clientSession) => {
    const req = clientSession.request({ [HTTP2_HEADER_PATH]: '/' });
    req.on('response', (headers) => {
      console.log('Response 2:', headers[HTTP2_HEADER_STATUS]);
      req.on('data', (chunk) => { });
      req.on('end', () => { 
        clientSession.close()
      });
    });
  });
});

// Create a session using a socket that has not yet finished connecting
const socket2 = net.connect(443, 'google.com');
http2.connect('https://google.com', {socket2}, (clientSession) => {
  const req = clientSession.request({ [HTTP2_HEADER_PATH]: '/' });
  req.on('response', (headers) => {
    console.log('Response 3:', headers[HTTP2_HEADER_STATUS]);
    req.on('data', (chunk) => { });
    req.on('end', () => { 
      clientSession.close();
      socket2.destroy();
    });
  });
});

How often does it reproduce? Is there a required condition?

This is completely consistent.

What is the expected behavior?

The expected output is:

Response 1: 301
Response 2: 301
Response 3: 301

What do you see instead?

Response 1: 301
Response 3: 301

The second request never progresses, but is still considered complete, and allows the process to end without any error output. Running this script with node --trace-tls shows wildly different behavior when running just the second request, as compared with running either other request.

Additional information

I discovered this when trying to implement HTTP CONNECT based proxy traversal for an HTTP/2 client. When you make a http.request call with method: 'CONNECT', when it succeeds the connect event provides a Socket equivalent that is already connected. Trying to use this to establish a secure HTTP/2 connection by passing it as the socket argument to http2.connect does not work and this is why.

The problem is that the way a Http2Session determines whether its socket (net.Socket or tls.TLSSocket) is still connecting is mismatched with the way TLSSocket communicates that it is still connecting. In particular, in this snippet of the Http2Session constructor, it uses socket.connecting to determine whether it should wait for the connect event from a net.Socket, or the secureConnect event from a tls.TLSSocket, to do the rest of its setup. But this snippet of the TLSSocket initializer shows that if a socket is passed in, it propagates that socket's connecting property but it does not yet emit the secureConnect event even if the underlying socket has finished connecting, probably because it still needs to perform the TLS handshake first.

Some other interesting information that didn't really go in the code snippet: in the bad case, the http2.connect callback is definitely called, but if you check session.connecting immediately, synchronously, the value is false. If you make a request immediately instead of waiting for the connect callback, it won't work either.