You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@trafficserver.apache.org by shinrich <gi...@git.apache.org> on 2016/05/23 14:37:54 UTC
[GitHub] trafficserver pull request: TS-4469: TS-3612 restructuring issues ...
GitHub user shinrich opened a pull request:
https://github.com/apache/trafficserver/pull/657
TS-4469: TS-3612 restructuring issues causing crashes in plugins.
A series of changes that fixed some of the use after free cases seen in Yahoo.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/shinrich/trafficserver ts-4469
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/trafficserver/pull/657.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #657
----
commit 3794120868da0026386a4631bc0c494e0a3071aa
Author: Susan Hinrichs <sh...@ieee.org>
Date: 2016-05-23T14:33:07Z
TS-4469: TS-3612 restructuring issues causing crashes in plugins.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4469: TS-3612 restructuring issues ...
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/657#discussion_r64237117
--- Diff: proxy/http2/Http2Stream.cc ---
@@ -241,17 +243,22 @@ Http2Stream::do_io_close(int /* flags */)
{
current_reader = NULL; // SM on the way out
if (!sent_delete) {
+ SCOPED_MUTEX_LOCK(lock, this->mutex, this_ethread());
--- End diff --
I don't know this code at all, but it seems a little scary to hold this mutex for so long. Particularly since it's still being held when we reenable SM ? And it's also held across the calls to a number of other methods, which hopefully are not trying to lock anything, but it seems like we could easily open up the possibilities of deadlocks here.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4469: TS-3612 restructuring issues ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/trafficserver/pull/657
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4469: TS-3612 restructuring issues ...
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/657#discussion_r64236451
--- Diff: proxy/http/HttpTunnel.cc ---
@@ -1311,7 +1311,8 @@ HttpTunnel::consumer_reenable(HttpTunnelConsumer *c)
if (is_debug_tag_set("http_tunnel"))
Debug("http_tunnel", "Unthrottle %p %" PRId64 " / %" PRId64, p, backlog, p->backlog());
srcp->unthrottle();
- srcp->read_vio->reenable();
+ if (srcp->read_vio)
--- End diff --
Extremely nitpicky, and I don't feel super strongly, but I think we've been trying to avoid the if()'s without {} going forward.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4469: TS-3612 restructuring issues ...
Posted by shinrich <gi...@git.apache.org>.
Github user shinrich commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/657#discussion_r64239248
--- Diff: proxy/http2/Http2Stream.cc ---
@@ -241,17 +243,22 @@ Http2Stream::do_io_close(int /* flags */)
{
current_reader = NULL; // SM on the way out
if (!sent_delete) {
+ SCOPED_MUTEX_LOCK(lock, this->mutex, this_ethread());
--- End diff --
The reenable is concerning. Agreed. That should happen very rarely, but would be a bear when it happens. I was mostly concerned with holding the lock while doing state changes to the Stream object (changing closed, sent_delete and parent). I'll restructure to drop the lock before going into reenable.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---