http2: Node crashes with an assertion error if an http2 server is closed after receiving and rejecting very large headers #35233

Activity

1. bnoordhuis

   added

   http2Issues and PRs related to the http2 subsystem.Issues and PRs related to the http2 subsystem.

   v14.x

   confirmed-bugIssues and PRs for confirmed bugs.Issues and PRs for confirmed bugs.

   on Sep 17, 2020
2. bnoordhuis commented on Sep 17, 2020

   

   bnoordhuis

   on Sep 17, 2020

   Member

   More actions

   I can confirm that the crash still happens with v14.11.0. It seems to be fixed on the master branch but I can't immediately pinpoint the responsible commit.

   cc @nodejs/http2
3. zerobytes commented on Nov 29, 2021

   

   zerobytes

   on Nov 29, 2021

   Last edited by zerobytes

   More actions

   When we get a ERR_HTTP2_SESSION_ERROR, node still crashes.

   Happens at 17.0.0 version.

   Do we have any ideas on how to prevent that from happening?
   try/catching the calls won't work and it causes node to restart everytime.
   Should I open a new issue for that?
4. mcollina commented on Dec 4, 2021

   

   mcollina

   on Dec 4, 2021

   SponsorMember

   More actions

   @zerobytes it would be awesome to receive a PR to fix this bug.
5. RafaelGSS commented on Jan 4, 2022

   

   RafaelGSS

   on Jan 4, 2022

   Last edited by RafaelGSS

   Member

   More actions

   Hey! I was working on it, but lately, I'm not having time to work on it. So I'd like to share my findings about this issue to help anyone who wants to fix it.

   Well, after several debugging over Node Http2 + nghttp2 I have figured out that the following script is leaking memory (69 bytes in my machine)

   const http2 = require('http2');
   
   const server = http2.createServer((req, res) => {
     res.end()
   });
   
   server.listen(8080, () => {
     const clientSession = http2.connect('http://localhost:8080', {
       maxSendHeaderBlockLength: Number.MAX_SAFE_INTEGER,
       // maxSendHeaderBlockLength: 10,
     });
   
     clientSession.on('error', (error) => {
       console.log(error);
     })
   
     const stream = clientSession.request({
       'test-header': 'A'.repeat(87382)
     });
   
     stream.on('close', () => {
       console.log(`Stream closed with RST_STREAM code ${stream.rstCode}`);
       clientSession.close();
       server.close();
     });
   
     stream.on('error', (error) => {
       console.log(error);
     })
   
     stream.end();
   });

   And if you decrease the 'A'.repeat(87382) by 1 it works.

   What happens under the hood is that nghttp2 contains a header limit as you can see here https://github.com/nghttp2/nghttp2/blob/20079b4c2f688385ba9ecf723f958d0448894879/lib/nghttp2_hd.h#L45.

   When you run the above script using nghttp2 debug mode you will get the following results:

   using 87381

   inflatehd: decoded integer is 8
   inflatehd: 8 bytes read
   inflatehd: huffman encoded=1
   inflatehd: decoded integer is 65536
   inflatehd: valuelen=65536
   inflatehd: 16355 bytes read
   inflatehd: still 49181 bytes to go
   recv: proclen=16369
   recv: [IB_EXPECT_CONTINUATION]
   ...
   

   using 87382

   inflatehd: decoded integer is 8
   inflatehd: 8 bytes read
   inflatehd: huffman encoded=1
   ----> inflatehd: integer exceeded the maximum value 65536
   ----> inflatehd: error return -523
   Http2Session server (6) sending pending data
   stream: adjusting kept idle streams num_idle_streams=0, max=100
   send: frame preparation failed with The current session is closing
   Http2Session server (6) frame type 4 was not sent, code: -530
   ...
   

   However, it is not the real bug. What happens, in reality, is that the nghttp2_rcbuf_decref wasn't called enough and the memory of the nghttp2_hd_inflater->nv_value_keep is never free because the nghttp2_hd_inflater contains ref > 0 when the deleter of session returns (https://github.com/nodejs/node/blob/master/src/node_http2.cc#L534)

   Not sure if I could explain properly, but basically, when 'A'.repeat(X) and X is greater than 87381 (this number might make sense only in my machine - due to architectural reasons) the memory of hd_inflater is never free because the rcbuf->ref (that count the references of an object) is > 0.

   An obvious “fix” is to sync the maxHeaderBufferLength with nghttp2 header limit, but I’m not a fan of this approach.

   Also, is not clear to me if that is a bug inside nghttp2 or our current implementation. For someone who will take it in the coming days, check which functions are calling the nghttp2_rcbuf_incref for the hd_inflater and never freeing when the maximum header value is thrown.

   Note: it might be related to #28632 (comment)

   cc: @addaleax (I'm tagging you because you might have a quick solution on mind)
6. RafaelGSS commented on Feb 2, 2022

   

   RafaelGSS

   on Feb 2, 2022

   Member

   More actions

   @mcollina could you close this one?
7. mcollina

   closed this as completedon Feb 3, 2022