Crash in HttpTunnel::consumer_reenable

[maskit]

Crash happened on 9.0.1.
In HttpTunnel::consumer_reenable, c->write_vio was null on this line.

NetVConnection *netvc = dynamic_cast<NetVConnection *>(c->write_vio->vc_server);

#0  0x0000556c505a7172 in HttpTunnel::consumer_reenable(HttpTunnelConsumer*) (this=0x7fa1fd715b00, c=0x7fa1fd715b48)
#1  0x0000556c505a6a32 in HttpTunnel::consumer_handler(int, HttpTunnelConsumer*) (this=0x7fa1fd715b00, event=3, c=0x7fa1fd715b48)
#2  0x0000556c505a3fa3 in HttpTunnel::main_handler(int, void*) (this=0x7fa1fd715b00, event=4194304, data=<optimized out>)
#3  0x0000556c5071bcee in handleEvent (this=<optimized out>, event=3, data=0x7fffffffffffffff)
#4  write_signal_and_update(int, UnixNetVConnection*) (event=3, vc=0x7fa1af626bc0)
#5  0x0000556c5071b297 in write_signal_done (event=3, nh=0x7fac1a211290, vc=0x7fa1af626bc0)
#6  write_signal_error (nh=0x7fac1a211290, vc=0x7fa1af626bc0, lerrno=0)
#7  write_to_net_io(NetHandler*, UnixNetVConnection*, EThread*) (nh=0x7fac1a211290, vc=0x7fa1af626bc0, thread=<optimized out>)
#8  0x0000556c5071430c in NetHandler::process_ready_list() (this=0x7fac1a211290)
#9  0x0000556c50714a12 in NetHandler::waitForActivity(long) (this=<optimized out>, timeout=<optimized out>)
#10 0x0000556c50714aed in non-virtual thunk to NetHandler::waitForActivity(long) ()
#11 0x0000556c5074b70f in EThread::execute_regular() (this=0x7fac1a20d1c0)
#12 0x0000556c5074b91a in EThread::execute() (this=0x7fac1a20d1c0)

[maskit]

The crash happens if a producer is throttled (flow controlled) and background fill starts.
Workaround is to disable flow control (proxy.config.http.flow_control.enabled).

Here is the detail:

consumer_handler calls HttpSM::tunnel_handler_ua here

trafficserver/proxy/http/HttpTunnel.cc

Lines 1317 to 1330 in e24c79a

	case VC_EVENT_WRITE_COMPLETE:
	case VC_EVENT_EOS:
	case VC_EVENT_ERROR:
	case VC_EVENT_ACTIVE_TIMEOUT:
	case VC_EVENT_INACTIVITY_TIMEOUT:
	ink_assert(c->alive);
	ink_assert(c->buffer_reader);
	c->alive = false;
	c->bytes_written = c->write_vio ? c->write_vio->ndone : 0;
	// Interesting tunnel event, call SM
	jump_point = c->vc_handler;
	(sm->*jump_point)(event, c);

And if background fill starts in HttpSM::tunnel_handler_ua, a consumer for client is closed here.

trafficserver/proxy/http/HttpSM.cc

Lines 3326 to 3329 in e24c79a

	// Even with the background fill, the client side should go down
	c->write_vio = nullptr;
	c->vc->do_io_close(EHTTP_ERROR);
	c->alive = false;

So the consumer c is no longer available at this point.

However, this block somehow calls consumer_reenable for the consumer that has just been closed, despite the comment.

trafficserver/proxy/http/HttpTunnel.cc

Lines 1367 to 1379 in e24c79a

	// Since we removed a consumer, it may now be
	// possible to put more stuff in the buffer
	// Note: we reenable only after calling back
	// the SM since the reenabling has the side effect
	// updating the buffer state for the VConnection
	// that is being reenabled
	if (p->alive && p->read_vio && p->read_buffer->write_avail() > 0) {
	if (p->is_throttled()) {
	this->consumer_reenable(c);
	} else {
	p->read_vio->reenable();
	}
	}

consumer_reenable uses the consumer here and ATS crashes, because write_vio is null.

trafficserver/proxy/http/HttpTunnel.cc

Lines 1272 to 1280 in e24c79a

	// We can stall for small thresholds on network sinks because this event happens
	// before the actual socket write. So we trap for the buffer becoming empty to
	// make sure we get an event to unthrottle after the write.
	if (HT_HTTP_CLIENT == c->vc_type) {
	NetVConnection *netvc = dynamic_cast<NetVConnection *>(c->write_vio->vc_server);
	if (netvc) { // really, this should always be true.
	netvc->trapWriteBufferEmpty();
	}
	}

The reenabling above won't happen if flow control is disabled, because throttling happens only if flow control is enabled.

[zwoop]

This is a new issue / regression between 8 and 9, right ? Actually, talking to VJ, it sounds like it's more of a bad interaction with the old setting and the two new ones?

[maskit]

This is a new issue / regression between 8 and 9, right ?

Yeah, it seems like so. The code clearing the write_vio in HttpSM was added by #6809, and it's not backported to 8.x.

I don't know about old settings and new settings, but the code here looks weird. I don't understand why we need to reeneble removed write_vio.

[github-actions]

This issue has been automatically marked as stale because it has not had recent activity. Marking it stale to flag it for further consideration by the community.