http2stream.respondWithFile leaks a file descriptor every time statCheck returns false.

[aaronriekenberg]

• Version: v10.11.0
• Platform: Debian 9 stable: Linux 4.9.0-8-amd64 deps: update openssl to 1.0.1j #1 SMP Debian 4.9.110-3+deb9u4 (2018-08-21) x86_64 GNU/Linux
• Subsystem: http2

I am finding that when using http2stream.respondWithFile, if statCheck returns false to cancel the send based on stat data, the file descriptor opened for the stat call is leaked and is never closed.

Here is a simple example cut and pasted from the documentation (https://nodejs.org/api/http2.html#http2_http2stream_respondwithfile_path_headers_options):

#!/usr/bin/env node

'use strict';

const http2 = require('http2');

const server = http2.createServer();

server.on('stream', (stream) => {

  console.log('got stream');

  function statCheck(stat, headers) {
    console.log('in statCheck');
    stream.respond({ ':status': 304 });
    return false;
  }

  function onError(err) {
    if (err.code === 'ENOENT') {
      stream.respond({ ':status': 404 });
    } else {
      stream.respond({ ':status': 500 });
    }
    stream.end();
  }

  stream.respondWithFile('./somefile',
                         { 'content-type': 'text/plain' },
                         { statCheck, onError });
});

server.listen({ port: 8080 });
console.log('after server.listen');

Steps to reproduce:

1. touch ./somefile
2. Run code above in same directory as ./somefile
3. curl --http2-prior-knowledge -v -k 'http://localhost:8080/'
4. Look at /proc/${PID}/fd directory for node process. Observe for every curl a new fd is opened for ./somefile and is never closed. Observe that 'ls -l /proc/${PID}/fd | wc -l' increases by one for each curl call.

If the code above is modified to comment out the stream.respond and return false lines in statCheck, no file descriptors are leaked.

Looking at the code I think the issue is the fd is not closed when statCheck returns false in doSendFileFD because there is no call to tryClose(fd) here: https://github.com/nodejs/node/blob/v10.11.0/lib/internal/http2/core.js#L2094

This would be an easy DOS attack on any server using this pattern. If the client can make the server return 304 it can make the server run out of file descriptors and crash.

[jasnell]

/cc @nodejs/http2 ... I'll be taking a look in the next day if someone doesn't beat me to it :-)