You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by "bneradt (via GitHub)" <gi...@apache.org> on 2025/10/14 16:44:56 UTC
[PR] Fix of use-after-free tunnel_handler_100_continue_ua [trafficserver]
bneradt opened a new pull request, #12561:
URL: https://github.com/apache/trafficserver/pull/12561
From a recent instrumented core taking traffic in production:
```
(gdb) p write_state.vio
$22 = {
cont = 0x0,
cont_handler_name = 0x9046f2 "&HttpTunnel::main_handler",
nbytes = 9223372036854775807,
ndone = 0,
op = 2,
_disabled = false,
buffer = {
mbuf = 0x0,
entry = 0x0
},
vc_server = 0x7faa48983920,
mutex = {
m_ptr = 0x7fa7bfa24a00
}
}
```
Notice that:
* cont is nullptr
* nbytes is 9223372036854775807 (INT64_MAX)
These are the default value of do_io_write when no values are passed to it. This doesn't happen in a lot of places, but it does currently in tunnel_handler_100_continue_ua:
```cpp
c->vc->do_io_write();
```
This changes the call to the following more typical way of canceling a vc write operation:
```cpp
c->vc->do_io_write(nullptr, 0, nullptr);
```
This way when a PluginVC::process_timeout event is processed, the ntodo will be 0 and thus the non-existent handler will not be called (see PluginVC.cc):
```cpp
} else if (write_state.vio.op == VIO::WRITE && !write_state.shutdown && write_state.vio.ntodo() > 0) {
```
--
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.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix of use-after-free tunnel_handler_100_continue_ua [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on PR #12561:
URL: https://github.com/apache/trafficserver/pull/12561#issuecomment-3457995640
> The issue is to avoid losing events for live objects. In this particular case this is fine. In the default case it looks like cont is null anyway. I guess if "this" had been deleted, would it cause a crash in this path? Since you've run with this a lot, we should probably go ahead with your fix. It isn't going to cause more object event loss since it was already passing null.
Thank you @shinrich for the thoughts. I wasn't aware of the other pattern of passing this instead of nullptr when stopping a VC. That's important to keep in mind.
After thinking about this some, I think I will move forward with this patch as is. We've had this in production now for a few weeks and it resolves the crash. So it's at least an improvement in that regard. And I think you're right that since the tunnel is torn down in this code, we probably don't want the HttpSM to handle timeout events from it later. I'm concerned it could inappropriately tear down the next tunnel created after the 100 Continue transaction is processed.
--
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.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix of use-after-free tunnel_handler_100_continue_ua [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt merged PR #12561:
URL: https://github.com/apache/trafficserver/pull/12561
--
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.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix of use-after-free tunnel_handler_100_continue_ua [trafficserver]
Posted by "shinrich (via GitHub)" <gi...@apache.org>.
shinrich commented on PR #12561:
URL: https://github.com/apache/trafficserver/pull/12561#issuecomment-3453465231
The issue is to avoid losing events for live objects. In this particular case this is fine. In the default case it looks like cont is null anyway. I guess if "this" had been deleted, would it cause a crash in this path?
Since you've run with this a lot, we should probably go ahead with your fix. It isn't going to cause more object event loss since it was already passing null.
--
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.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix of use-after-free tunnel_handler_100_continue_ua [trafficserver]
Posted by "cmcfarlen (via GitHub)" <gi...@apache.org>.
cmcfarlen commented on PR #12561:
URL: https://github.com/apache/trafficserver/pull/12561#issuecomment-3513469333
Cherry-picked to 10.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.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix of use-after-free tunnel_handler_100_continue_ua [trafficserver]
Posted by "shinrich (via GitHub)" <gi...@apache.org>.
shinrich commented on code in PR #12561:
URL: https://github.com/apache/trafficserver/pull/12561#discussion_r2446295076
##########
src/proxy/http/HttpSM.cc:
##########
@@ -3425,9 +3425,9 @@ HttpSM::tunnel_handler_100_continue_ua(int event, HttpTunnelConsumer *c)
_ua.get_entry()->in_tunnel = false;
c->write_success = true;
- // remove the buffer reader from the consumer's vc
+ // Disable any write operation in case there are timeout events.
if (c->vc != nullptr) {
- c->vc->do_io_write();
+ c->vc->do_io_write(nullptr, 0, nullptr);
Review Comment:
You may want to pass in "this" as the first argument. I've had problems before with state machines getting lost because a terminal event is delivered to a VIO with no continuation to deliver the terminal event to.
--
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.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org