You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by GitBox <gi...@apache.org> on 2021/03/09 03:00:45 UTC
[GitHub] [trafficserver] shinrich opened a new pull request #7586: Fix crash in open_close_h2
shinrich opened a new pull request #7586:
URL: https://github.com/apache/trafficserver/pull/7586
Found this while running autest against 9.0.x. About half the time I'd get a crash from open_close_h2. A later version of the crash is below. During debugging I added an assert to verify that the read_vio.cont was the the Http2 session. In the origin crash, the read_vio.cont was the Http2ClientSession, so after reading the request header, the READ_COMPLETE was sent to the Http2ClientSession which caused it to try and read another frame, even though there was no more data in the buffer. This caused it to interpret a frame of a bogus type.
```
(gdb) bt
#0 0x00002acaf252e3d7 in raise () from /lib64/libc.so.6
#1 0x00002acaf252fac8 in abort () from /lib64/libc.so.6
#2 0x00002acaefaa40c8 in ink_abort (message_format=0x2acaefb11e78 "%s:%d: failed assertion `%s`") at ink_error.cc:99
#3 0x00002acaefa9fd11 in _ink_assert (expression=0xa27515 "read_vio.cont != _proxy_ssn", file=0xa27499 "Http2Stream.cc", line=168) at ink_assert.cc:37
#4 0x00000000007f0629 in Http2Stream::send_request (this=0x2acb02a78b80, cstate=...) at Http2Stream.cc:168
#5 0x00000000007e0a9b in rcv_headers_frame (cstate=..., frame=...) at Http2ConnectionState.cc:390
#6 0x00000000007e538e in Http2ConnectionState::main_event_handler (this=0x2acb14bb0798, event=2253, edata=0x2acafc308f60) at Http2ConnectionState.cc:1063
#7 0x000000000066b737 in Continuation::handleEvent (this=0x2acb14bb0798, event=2253, data=0x2acafc308f60) at /home/shinrich/vtrafficserver9/iocore/eventsystem/I_Continuation.h:167
#8 0x00000000007d6906 in send_connection_event (cont=0x2acb14bb0798, event=2253, edata=0x2acafc308f60) at Http2ClientSession.cc:65
#9 0x00000000007da8b4 in Http2ClientSession::do_complete_frame_read (this=0x2acb14bb0490) at Http2ClientSession.cc:570
#10 0x00000000007daf2d in Http2ClientSession::state_process_frame_read (this=0x2acb14bb0490, event=100, vio=0x2acb1b228ab0, inside_frame=false) at Http2ClientSession.cc:628
#11 0x00000000007d9ca8 in Http2ClientSession::state_start_frame_read (this=0x2acb14bb0490, event=100, edata=0x2acb1b228ab0) at Http2ClientSession.cc:482
#12 0x00000000007d8859 in Http2ClientSession::main_event_handler (this=0x2acb14bb0490, event=100, edata=0x2acb1b228ab0) at Http2ClientSession.cc:352
#13 0x000000000066b737 in Continuation::handleEvent (this=0x2acb14bb0490, event=100, data=0x2acb1b228ab0) at /home/shinrich/vtrafficserver9/iocore/eventsystem/I_Continuation.h:167
#14 0x00000000007d9a29 in Http2ClientSession::state_read_connection_preface (this=0x2acb14bb0490, event=100, edata=0x2acb1b228ab0) at Http2ClientSession.cc:462
#15 0x00000000007d8859 in Http2ClientSession::main_event_handler (this=0x2acb14bb0490, event=100, edata=0x2acb1b228ab0) at Http2ClientSession.cc:352
#16 0x000000000066b737 in Continuation::handleEvent (this=0x2acb14bb0490, event=100, data=0x2acb1b228ab0) at /home/shinrich/vtrafficserver9/iocore/eventsystem/I_Continuation.h:167
#17 0x000000000099b159 in read_signal_and_update (event=100, vc=0x2acb1b2288d0) at UnixNetVConnection.cc:83
#18 0x000000000099efa6 in UnixNetVConnection::readSignalAndUpdate (this=0x2acb1b2288d0, event=100) at UnixNetVConnection.cc:1042
#19 0x000000000095b997 in SSLNetVConnection::net_read_io (this=0x2acb1b2288d0, nh=0x2acaf6eb4a20, lthread=0x2acaf6eb0980) at SSLNetVConnection.cc:670
#20 0x00000000009904a0 in NetHandler::process_ready_list (this=0x2acaf6eb4a20) at UnixNet.cc:416
#21 0x0000000000990dc3 in NetHandler::waitForActivity (this=0x2acaf6eb4a20, timeout=60000000) at UnixNet.cc:551
#22 0x00000000009d4915 in EThread::execute_regular (this=0x2acaf6eb0980) at UnixEThread.cc:271
#23 0x00000000009d4b17 in EThread::execute (this=0x2acaf6eb0980) at UnixEThread.cc:332
#24 0x00000000009d3345 in spawn_thread_internal (a=0x2acaf33d9f80) at Thread.cc:92
#25 0x00002acaf18bfea5 in start_thread () from /lib64/libpthread.so.0
#26 0x00002acaf25f69fd in clone () from /lib64/libc.so.6
```
The problem is that the HttpSM::state_add_to_list() was calling a do_io_read and passing in the continuation associated with the netvc (the Http2ClientSession in this case). That is never the correct thing for a Http2 session. This logic was introduced in PR #7096
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] masaori335 merged pull request #7586: Fix crash in open_close_h2
Posted by GitBox <gi...@apache.org>.
masaori335 merged pull request #7586:
URL: https://github.com/apache/trafficserver/pull/7586
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] masaori335 commented on a change in pull request #7586: Fix crash in open_close_h2
Posted by GitBox <gi...@apache.org>.
masaori335 commented on a change in pull request #7586:
URL: https://github.com/apache/trafficserver/pull/7586#discussion_r638447115
##########
File path: proxy/http/HttpSM.cc
##########
@@ -465,11 +465,8 @@ HttpSM::state_add_to_list(int event, void * /* data ATS_UNUSED */)
if (ua_entry->read_vio) {
// Seems like ua_entry->read_vio->disable(); should work, but that was
// not sufficient to stop the state machine from processing IO events until the
- // TXN_START hooks had completed
- // Preserve the current read cont and mutex
- NetVConnection *netvc = ((ProxyTransaction *)ua_entry->vc)->get_netvc();
- ink_assert(netvc != nullptr);
- ua_entry->read_vio = ua_entry->vc->do_io_read(netvc->read_vio_cont(), 0, nullptr);
+ // TXN_START hooks had completed. Just set the number of bytes to read to 0
+ ua_entry->read_vio = ua_entry->vc->do_io_read(this, 0, nullptr);
Review comment:
I got the same idea while digging another issue.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] zwoop commented on pull request #7586: Fix crash in open_close_h2
Posted by GitBox <gi...@apache.org>.
zwoop commented on pull request #7586:
URL: https://github.com/apache/trafficserver/pull/7586#issuecomment-848869576
Cherry-picked to v9.1.x branch.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] zwoop commented on pull request #7586: Fix crash in open_close_h2
Posted by GitBox <gi...@apache.org>.
zwoop commented on pull request #7586:
URL: https://github.com/apache/trafficserver/pull/7586#issuecomment-848868211
@masaori335 is this a candidate for 9.0.x as well ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] masaori335 commented on a change in pull request #7586: Fix crash in open_close_h2
Posted by GitBox <gi...@apache.org>.
masaori335 commented on a change in pull request #7586:
URL: https://github.com/apache/trafficserver/pull/7586#discussion_r638453227
##########
File path: proxy/http2/Http2Stream.cc
##########
@@ -259,13 +259,16 @@ Http2Stream::send_request(Http2ConnectionState &cstate)
return;
}
- if (this->recv_end_stream) {
- this->read_vio.nbytes = bufindex;
- this->signal_read_event(VC_EVENT_READ_COMPLETE);
- } else {
- // End of header but not end of stream, must have some body frames coming
- this->has_body = true;
- this->signal_read_event(VC_EVENT_READ_READY);
+ // Is the _sm ready to process the header?
+ if (this->read_vio.nbytes > 0) {
Review comment:
NVM. Retry on the HTTP/2 side is not required. My understanding is
- If HttpSM can't get hook continuation lock in above, it schedule `EVENT_INTERVAL`.
- When HttpSM handles `EVENT_INTERVAL`, it'll read request in this read_vio.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shinrich commented on pull request #7586: Fix crash in open_close_h2
Posted by GitBox <gi...@apache.org>.
shinrich commented on pull request #7586:
URL: https://github.com/apache/trafficserver/pull/7586#issuecomment-793972288
[approve ci autest]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] masaori335 commented on pull request #7586: Fix crash in open_close_h2
Posted by GitBox <gi...@apache.org>.
masaori335 commented on pull request #7586:
URL: https://github.com/apache/trafficserver/pull/7586#issuecomment-849167599
> This logic was introduced in PR #7096
#7278 is the PR, I think, it's backported to 9.0.x, so we need this to 9.0.x too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] masaori335 commented on a change in pull request #7586: Fix crash in open_close_h2
Posted by GitBox <gi...@apache.org>.
masaori335 commented on a change in pull request #7586:
URL: https://github.com/apache/trafficserver/pull/7586#discussion_r638448691
##########
File path: proxy/http2/Http2Stream.cc
##########
@@ -259,13 +259,16 @@ Http2Stream::send_request(Http2ConnectionState &cstate)
return;
}
- if (this->recv_end_stream) {
- this->read_vio.nbytes = bufindex;
- this->signal_read_event(VC_EVENT_READ_COMPLETE);
- } else {
- // End of header but not end of stream, must have some body frames coming
- this->has_body = true;
- this->signal_read_event(VC_EVENT_READ_READY);
+ // Is the _sm ready to process the header?
+ if (this->read_vio.nbytes > 0) {
Review comment:
When `read_vio.nbytes == 0`, ATS retry thie `Http2Stream::send_request()` call by somehow?
##########
File path: proxy/http2/Http2Stream.cc
##########
@@ -259,13 +259,16 @@ Http2Stream::send_request(Http2ConnectionState &cstate)
return;
}
- if (this->recv_end_stream) {
- this->read_vio.nbytes = bufindex;
- this->signal_read_event(VC_EVENT_READ_COMPLETE);
- } else {
- // End of header but not end of stream, must have some body frames coming
- this->has_body = true;
- this->signal_read_event(VC_EVENT_READ_READY);
+ // Is the _sm ready to process the header?
+ if (this->read_vio.nbytes > 0) {
Review comment:
When `read_vio.nbytes == 0`, ATS retry this `Http2Stream::send_request()` call by somehow?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] masaori335 commented on pull request #7586: Fix crash in open_close_h2
Posted by GitBox <gi...@apache.org>.
masaori335 commented on pull request #7586:
URL: https://github.com/apache/trafficserver/pull/7586#issuecomment-849210211
This conflicts against 9.0.x because of #7499, which is backported to 9.1.x only. IMO, #7499 is not necessary and too big for 9.0.x.
I'll open backport PR for this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org