Http2: Cannot read property 'finishWrite' of null

[panmenghan]

• Version:
  v14.14.0 + v12.19.0
• Platform:
  Win10(2004, 64bit) + Ubuntu 18.04.4(wsl2, Linux 4.19.128-microsoft-standard SMP Tue Jun 23 12:58:10 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux)
• Subsystem:
  http2

What steps will reproduce the bug?

server.js

const http2 = require('http2')
const fs = require('fs')
const childProcess = require('child_process')

const PORT = 8443

module.exports = main()

async function main() {
  if (!fs.existsSync('server.key')) {
    childProcess.execSync(`openssl req -subj '/CN=localhost/O=localhost/C=US' -nodes -new -x509 -keyout server.key -out server.cert`)
  }

  const options = {
    key: fs.readFileSync('server.key'),
    cert: fs.readFileSync('server.cert')
  }

  return new Promise((resolve, reject) => {
    const server = http2.createSecureServer(options)
    server.on('stream', (stream) => {
      stream.on('error', error => {console.log('server:', 'stream', error)})
      // stream.respond({
      //   'content-type': 'text/html; charset=utf-8',
      //   ':status': 200
      // })
      // stream.end('<h1>Hello World</h1>')
    })
    server.listen(PORT, () => {
      console.log('server:', `https://localhost:${PORT}`)
      resolve(server)
    })
    server.on('error', reject)
    let sockets = []
    server.on('connection', socket => {
      console.log('server:', 'new client', socket.address())
      socket.setNoDelay()
      sockets.push(socket)
    })
    server.kill = () => {
      sockets.forEach(socket => socket.destroy())
      server.close()
    }
  })
}

client.js

const childProcess = require('child_process')
const http2 = require('http2')
const net = require('net')
const assert = require('assert')
const {Duplex} = require('stream')

const ARGV = process.argv.slice(-1)[0]
console.log({ARGV})

main()

async function main() {
  const url = 'https://localhost:8443'
  // {
  //   // use socket
  //   const server = await makeServer()
  //   const socket = net.connect({host: 'localhost', port: '8443'}, async () => {
  //     socket.on('error', error => socket.destroy(error))
  //     await makeRequest(url, socket, server)
  //     server.kill()
  //   })
  // }

  {
    // use duplex
    const server = await makeServer()
    const socket = net.connect({host: 'localhost', port: '8443'}, async () => {
      const wrapSocket = new WrapSocket(socket)
      await makeRequest(url, wrapSocket, server)
      server.kill()
    })
  }
}

async function makeServer() {
  let server
  if (ARGV.includes('spawn')) {
    server = childProcess.spawn('node', ['server.js'], {stdio: 'inherit'})
    await sleep(500)
  } else {
    server = await require('./server')
  }

  process.on('uncaughtException', error => {
    console.log('client:', 'uncaughtException!!!\n', error)
    server.kill()
  })
  return server
}

async function makeRequest(url, socket, server) {
  const h2Session = http2.connect(url, {
    rejectUnauthorized: false,
    socket
  })
  h2Session.on('error', error => {console.log('client:', 'h2Session error', error.message)})
  const stream = h2Session.request({[http2.constants.HTTP2_HEADER_PATH]: '/'})
  stream.on('error', error => {console.log('client:', 'stream error', error.message)})
  stream.on('response', headers => {});

  if (ARGV.includes('patch')) {
    patch(h2Session, socket)
  }

  await sleep(500)
  // abort the request when waiting the server response
  if (ARGV.includes('remote')) {
    server.kill() // by remote side socket
  } else {
    socket.destroy() // by self(local side socket)
  }

  await sleep(500)
  // should?: destroyed === true
  console.log('client:', 'h2Session.destroyed', h2Session.destroyed)

  // should?: h2Session.socket === undefined or destroyed === true
  console.log('client:', 'h2Session.socket.destroyed', h2Session.socket && h2Session.socket.destroyed)

  // should: destroyed === true
  console.log('client:', 'socket.destroyed', socket.destroyed)
  h2Session.close(() => { // <-- crash
    console.log('client:', 'h2Session.close')
  })

  await sleep(200)
  console.log('client:', 'all errors caught')
}

function patch(h2Session, socket) {
  // require --expose-internals
  const {kSocket} = require('internal/http2/util')
  h2Session[kSocket]._handle._parentWrap.on('close', ()=>{
    h2Session[kSocket] && h2Session[kSocket].destroy()
  })
}

class WrapSocket extends Duplex {
  constructor(socket) {
    super({autoDestroy: true, allowHalfOpen: false})
    socket.on('end', data => this.push(null))
    socket.on('close', () => this.destroy())
    socket.on('error', error => this.destroy(error))
    socket.on('data', data => this.push(data))
    this.socket = socket
  }

  _write(data, encoding, callback) {
    this.socket.write(data, encoding, callback)
  }

  _final(callback) {
    callback()
  }

  _read(size) {
    // this.socket.on('data', data => this.push(data))
  }

  _destroy(error, callback) {
    callback(error)
  }
}

async function sleep(ms) {
  return new Promise(resolve => {setTimeout(() => resolve(), ms)})
}

the error(bug):

// only can be caught by process.on('uncaughtException')
TypeError: Cannot read property 'finishWrite' of null
    at JSStreamSocket.finishWrite (internal/js_stream_socket.js:210:12)
    at Immediate.<anonymous> (internal/js_stream_socket.js:195:14)
    at processImmediate (internal/timers.js:461:21)

client.js argv(command line options) mean:

local, remote: close the socket by local side(client), or close the socket by remote side(server).
spawn: spawn the server.js in new process, or not.
patch: undo the change introduced by https://github.com/nodejs/node/pull/34105, or not.

Win10(2004)

> node -v
v14.14.0 + v12.19.0
> node client.js local
(error)
> node client.js local+spawn
(error)
> node client.js remote
(no error)
> node client.js remote+spawn
(error, v14.14.0) 
(no error, v12.19.0)
> node --expose-internals client.js local+patch
(no error)
> node --expose-internals client.js remote+spawn+patch
(error, v14.14.0(patch not work!)) 

> node -v
v12.18.3
> node client.js local
(...and all combinations)
(no error)

Ubuntu 18.04.4(wsl2)

> node -v
v14.14.0 + v12.19.0
> node client.js local
(error)
> node client.js local+spawn
(error)
> node client.js remote
(no error)
> node client.js remote+spawn
(no error)
> node --expose-internals client.js local+patch
(no error)

> node -v
v12.18.3
> node client.js local
(...and all combinations)
(no error)

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

What is the expected behavior?

No "TypeError: Cannot read property 'finishShutdown' of null" error

What do you see instead?

Additional information

[mmomtchev]

This is a collection of various minor issues all linked to the edge case of the underlying stream of the TLS listener being null after being indirectly removed by the socket.destroy(). When passing through JSStreamSocket (because of the WrapSocket class), it throws an exception, however when interfaced to the C++ stream, it ends with a segfault. The example is overly complex, you just need to destroy the underlying socket of an open TLS connection and you will run into problems. @mildsunrise
TLSWrap is full of if (stream_ != nullptr) and a couple of them are missing - TLSWrap::EncOut() is one example

[mmomtchev]

The segfault does not occur with the new crypto_tls code, it occurs only on v14.x
The TypeError is still there.

[panmenghan]

@mmomtchev
I first post the issue to there.
Reproduce the error against https://www.example.com. (but can not reproduce the error every time, on win10)
Now I also test with node v15.0.0, but the error is still there(win10).

[mmomtchev]

I was able to compress it down to this unit test:

'use strict';

const common = require('../common');
if (!common.hasCrypto)
  common.skip('missing crypto');
const h2 = require('http2');
const net = require('net');
const fixtures = require('../common/fixtures');
const { Duplex } = require('stream');

const server = h2.createSecureServer({
  key: fixtures.readKey('agent1-key.pem'),
  cert: fixtures.readKey('agent1-cert.pem')
});

class JSSocket extends Duplex {
  constructor(socket) {
    super();
    socket.on('close', () => this.destroy());
    socket.on('data', (data) => this.push(data));
    this.socket = socket;
  }

  _write(data, encoding, callback) {
    this.socket.write(data, encoding, callback);
  }

  _read(size) {
  }
}

server.listen(0, common.mustCall(function() {
  const socket = net.connect({
    host: 'localhost',
    port: this.address().port
  }, () => {
    const client = h2.connect(`https://localhost:${this.address().port}`, {
      rejectUnauthorized: false,
      socket: new JSSocket(socket)
    });
    const req = client.request();

    setTimeout(() => socket.destroy(), 500);
    setTimeout(() => client.close(), 1000);
    setTimeout(() => server.close(), 1500);

    req.on('close', common.mustCall(() => { }));
  });
}));

With JSSocket - exception on Node 14.x and master
Without JSSocket (pass socket instead of new JSSocket(socket)) - segfault on Node 14.x and ok since the crypto_tls merge on master
The final 'close' event is never called

[panmenghan]

The reason I try to spawn the server in a new process and kill the server is I want server send RST packet(not FIN packet) to end the connection. Maybe the error RST end the connection is not the same as FIN.

[mmomtchev]

@jasnell, @addaleax is http2.connect(url, { socket: s }) a supported code path?
Because the officially documented option is a function called createConnection returning a socket?
However should a { socket: s } option be passed, it will get passed to tls.connect which supports it and will use it.
I wonder if this is intended?

[panmenghan]

using createConnection instead:

    ...

    const client = h2.connect(`https://localhost:${this.address().port}`, {
      rejectUnauthorized: false,
      createConnection: () => tls.connect({
        socket: new JSSocket(socket),
        host: 'localhost', port: this.address().port, ALPNProtocols: ['h2']
      }),
      // socket: new JSSocket(socket)
    });

   ...

the same error:

node:internal/js_stream_socket:210
    handle.finishWrite(req, errCode);
           ^
TypeError: Cannot read property 'finishWrite' of null
    at JSStreamSocket.finishWrite (node:internal/js_stream_socket:210:12)
    at Immediate.<anonymous> (node:internal/js_stream_socket:195:14)
    at processImmediate (node:internal/timers:462:21)

but this can prevent the error:

...

      createConnection: () => {
        const jsSocket = new JSSocket(socket)
        const tlsSocket = tls.connect({
          socket: jsSocket,
          host: 'localhost', port: this.address().port, ALPNProtocols: ['h2']
        })
        // tlsSocket.on('close', ()=>{jsSocket.destroy()})
        jsSocket.on('close', ()=>{tlsSocket.destroy()}) // <-- add this line
        return tlsSocket
    }

...

[mmomtchev]

@panmenghan the exception is trivial to fix on the createConnection path but it is only the tip of the iceberg on the officially unsupported socket path
Maybe it is time to create another issue and submit the trivial fix for the exception

[panmenghan]

@mmomtchev Thanks, please fix it. I already rewrite my code to use createConnection instead.
But why https not crash, only http2 crash? http2 use tls as transport should the same as https?

[mmomtchev]

@panmenghan the bug is in the http2 code

[damanis]

The example from first comment is reproducible in NodeJS 14.16.1.
Should not it be fixed (related commits included to 14.15.2) ?