Skip to content
This repository has been archived by the owner on Aug 11, 2020. It is now read-only.

QuicStreamStats::final_size assertion crash #383

Open
splitice opened this issue May 7, 2020 · 9 comments
Open

QuicStreamStats::final_size assertion crash #383

splitice opened this issue May 7, 2020 · 9 comments

Comments

@splitice
Copy link

splitice commented May 7, 2020

  • Version: d2ed3a8
  • Platform: Debian x64 (server), Debian ARM 32bit (client)
  • Subsystem: QUIC server

What steps will reproduce the bug?

Still working this out

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

Reproduction Rate: 100%

Still working this out

What do you see instead?

Server Side crash:

node[20171]: ../src/quic/node_quic_stream-inl.h:38:void node::quic::QuicStream::set_final_size(uint64_t): Assertion `(GetStat(&QuicStreamStats::final_size)) == (0)' failed.
 1: 0x55c259d6b7b0 node::Abort() [node]
 2: 0x55c259d6b833  [node]
 3: 0x55c259eccaac  [node]
 4: 0x55c259ed290e node::quic::QuicSession::OnStreamReset(ngtcp2_conn*, long, unsigned long, unsigned long, void*, void*) [node]
 5: 0x55c25ac93876  [node]
 6: 0x55c25ac984cf ngtcp2_conn_read_pkt [node]
 7: 0x55c259ecba2c  [node]
 8: 0x55c259ecf275 node::quic::QuicSession::Receive(long, unsigned char const*, node::SocketAddress const&, node::SocketAddress const&, unsigned int) [node]
 9: 0x55c259ee060b node::quic::QuicSocket::OnReceive(long, node::AllocatedBuffer, node::SocketAddress const&, node::SocketAddress const&, unsigned int) [node]
10: 0x55c259ed922a node::quic::QuicEndpoint::OnRecv(long, uv_buf_t const&, sockaddr const*, unsigned int) [node]
11: 0x55c25a744017  [node]
12: 0x55c25a746cd0  [node]
13: 0x55c25a73438b uv_run [node]
14: 0x55c259daddae node::NodeMainInstance::Run() [node]
15: 0x55c259d3a254 node::Start(int, char**) [node]
16: 0x7f475c3fa2e1 __libc_start_main [/lib/x86_64-linux-gnu/libc.so.6]
17: 0x55c259cd113a _start [node]

Additional information

Found when experimenting with porting existing software to QUIC.

@jasnell
Copy link
Member

jasnell commented May 7, 2020

Ok, good to know! I have this check in there for now largely as a debugging aid as I suspected this could happen but hadn't yet been able to trigger the case. The final size should only ever be set once so once we identify the paths where it would be set multiple times we need to add the necessary guards.

@splitice
Copy link
Author

splitice commented May 7, 2020

@jasnell I've been playing with GDB OnStreamReset is only called once.

I think I found the cause, calling .destroy() on the stream rather than .close() on the client side. This is the result of my porting from a different protocol, still no reason it should crash the server component (DoS).

Might I suggest that some of the assertions be ported to stream errors in the future? Servers should not be prone to crashing from incorrect user supplied data.

@splitice
Copy link
Author

splitice commented May 7, 2020

Hmm,

Doesnt happen 100% of the time without .destroy() however I still managed to get it to happen. This time it was the client side that hit the assertion.

I'll test a build without that assertion and see what happens.

@jasnell
Copy link
Member

jasnell commented May 7, 2020

Fantastic. Ok. Are you able to provide a test case that at least triggers it some of the time? I can refine it down from there. And yes, some of the asserts will convert into catchable errors.

@splitice
Copy link
Author

splitice commented May 7, 2020

@jasnell trying to do that, not having much luck getting a simple test to actually do it.

@jasnell
Copy link
Member

jasnell commented May 7, 2020

Ok. I'm hoping to have some time to look at it tomorrow. I'll let you know what I find

@splitice
Copy link
Author

@jasnell any luck?

@jasnell
Copy link
Member

jasnell commented May 12, 2020

Not yet. Ended up running out of time the other day. Thank you for the reminder, I'll make sure I take a look tomorrow

@splitice
Copy link
Author

Might also be relevant. I dumped out the session level stats and they don't add up.

"bidiCount": "765",
"selfInitiatedCount": "994",
"uniCount": "0",
"peerInitiatedCount": "536"

Only opening bidirectional streams. If the counters were working wouldnt selfInitiatedCount+peerInitiatedCount = bidiCount ?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants