Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

multi-thread lsquic server crashed because header stream was released before connection closed(Q043). #201

Closed
jazune opened this issue Dec 3, 2020 · 5 comments
Labels

Comments

@jazune
Copy link

jazune commented Dec 3, 2020

I got a crash in maybe_conn_flush_headers_stream function because the header stream was abnormal released when I carried out my stress test on my multi-thread server.(lsquc version is 2.20.0 with multi-thread safe patch)

I found the header stream was released (with AddressSanitizer compile) in service_streams function because (stream->sm_qflags & SMQF_FREE_STREAM) was true.

Since maybe_conn_flush_headers_stream will be executed in every full_conn_ci_tick, Should we not release header stream before connection closed?

It seems header stream flag SMQF_FREE_STREAM was set but conn was not closed, and after the next full_conn_ci_tick, it crashed.

@jazune jazune closed this as completed Dec 4, 2020
@dtikhonov
Copy link
Contributor

@jazune, did this turn out not to be an lsquic bug?

@jazune
Copy link
Author

jazune commented Dec 7, 2020

It seems lsquic bug.
I use AddressSanitizer to analyse, and I found the header stream is allocated in lsquic, released in lsquic, and reused also in lsquic.
In fact, application will not use header stream directly.

I fix this by:

headers_on_close (lsquic_stream_t *stream, struct lsquic_stream_ctx *ctx)
{
struct headers_stream *hs = (struct headers_stream *) ctx;.
LSQ_DEBUG("stream closed");
++ hs->hs_stream = NULL;

}

@@ -3099,6 +3099,8 @@ maybe_conn_flush_headers_stream (struct full_conn *conn)
if (conn->fc_flags & FC_HTTP)
{
stream = lsquic_headers_stream_get_stream(conn->fc_pub.u.gquic.hs);
++ if (stream == NULL)
++ return;
if (lsquic_stream_has_data_to_flush(stream))
(void) lsquic_stream_flush(stream);
}

But I still don't know why full_conn_ci_tick was executed when header stream was closed

@jazune jazune reopened this Dec 7, 2020
@dtikhonov
Copy link
Contributor

It does not happen in single-threaded code that a connection is ticked after the headers stream is destroyed. I incline to think that it is more likely that the bug is in your multi-threaded logic.

@dtikhonov
Copy link
Contributor

We have been able to reproduce this issue. Fix is forthcoming.

litespeedtech pushed a commit that referenced this issue Jan 8, 2021
- [BUGFIX] gQUIC: do not destroy critical streams when connection is
  closed.  See issue #201.
- [BUGFIX] Drop #if LSQUIC_CONN_STATS from lsquic.h.  See issue #211.
- [BUGFIX] Challenge cancellation when path validation fails.
- [BUGFIX] Do not send FIN if RST is scheduled to be sent on a stream.
- [BUGFIX] gQUIC's is_tickable() when connection is closing.
- [BUGFIX] Q050 processing of GOAWAY frames.
@litespeedtech
Copy link
Owner

Fixed in 2.27.3 -- closing.

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

No branches or pull requests

3 participants