You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@trafficserver.apache.org by canselcik <gi...@git.apache.org> on 2015/11/05 04:52:41 UTC
[GitHub] trafficserver pull request: Dereferencing a NULL pointer in SpdyCl...
GitHub user canselcik opened a pull request:
https://github.com/apache/trafficserver/pull/323
Dereferencing a NULL pointer in SpdyClientSession::clear()
- At `SpdyClientSession.cc:28`, `static ClassAllocator<SpdyClientSession> spdyClientSessionAllocator` creates an instance of `SpdyClientSession` using the default constructor.
- From that point on, `spdyClientSessionAllocator.alloc()` essentially calls memcpy on this prototype as an optimization to return new instances.
- The regular usage of `SpdyClientSession` ensures that `SpdyClientSession::init()` would be called before its destructor is invoked. This init function sets the value of the `mutex` pointer inside `SpdyClientSession` from its initial value of `NULL`.
- When `ClassAllocator` is being freed, the destructor on its `SpdyClientSession` prototype is called. However, the `mutex` inside the prototype is `NULL` and dereferencing it to get to `this->mutex->thread_holding` causes a SEGFAULT.
@bgaff
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/canselcik/trafficserver spdy-sigsegv
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/trafficserver/pull/323.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 #323
----
commit 24c5510c7f89dd7d3aedeba99e5f6f96e01945d5
Author: Can Selcik <se...@illinois.edu>
Date: 2015-11-05T03:43:24Z
Dereferencing a NULL pointer in SpdyClientSession::clear()
- At `SpdyClientSession.cc:28`, `static ClassAllocator<SpdyClientSession> spdyClientSessionAllocator` creates an instance of `SpdyClientSession` using the default constructor.
- From that point on, `spdyClientSessionAllocator.alloc()` essentially calls memcpy on this prototype as an optimization to return new instances.
- The regular usage of `SpdyClientSession` ensures that `SpdyClientSession::init()` would be called before its destructor is invoked. This init function sets the value of the `mutex` pointer inside `SpdyClientSession` from its initial value of `NULL`.
- When `ClassAllocator` is being freed, the destructor on its `SpdyClientSession` prototype is called. However, the `mutex` inside the prototype is `NULL` and dereferencing it to get to `this->mutex->thread_holding` causes a SEGFAULT.
----
---
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: Dereferencing a NULL pointer in SpdyCl...
Posted by canselcik <gi...@git.apache.org>.
Github user canselcik commented on the pull request:
https://github.com/apache/trafficserver/pull/323#issuecomment-153974633
How about `ClassAllocator<SpdyRequest> spdyRequestAllocator`, though? If the right fix is to remove `~SpdyClientSession()`, then should we also remove `~SpdyRequest()`?
`~SpdyRequest()` seems to be calling `SpdyRequest::clear()` so the same behavior is observed there. The only difference is that it already has the `if(!spdy_sm) return;` check at the top.
---
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: Dereferencing a NULL pointer in SpdyCl...
Posted by bgaff <gi...@git.apache.org>.
Github user bgaff commented on the pull request:
https://github.com/apache/trafficserver/pull/323#issuecomment-153950509
@jpeach the only way to deal with that would be a mutex that protects initialization and destruction. The problem is that these objects are constructed via static initialization and so the mutex itself isn't guaranteed to be initialized in time. I believe this is just a variant of the static initialization fiasco problem.
---
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: Dereferencing a NULL pointer in SpdyCl...
Posted by canselcik <gi...@git.apache.org>.
Github user canselcik commented on the pull request:
https://github.com/apache/trafficserver/pull/323#issuecomment-153950655
@jpeach `ClassAllocator<SpdyClientSession>`, when initialized, creates an instance of `SpdyClientSession` and puts it aside. At this point, for this "prototype", `SpdyClientSession::mutex` is `NULL`.
It uses the default constructor to create this instance, and then sets this instance aside to make duplicates of in the future when asked to.
Normally, whenever `SpdyClientSession` is used, `SpdyClientSession::init()` is called immediately, but ClassAllocator doesn't perform such a specific operation as it should be as general as possible.
---
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: Dereferencing a NULL pointer in SpdyCl...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/323#issuecomment-153949955
How do we end up freeing a partially-initialized ```SpdyClientSession```? It looks like this does not fix the root cause of the problem.
---
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: Dereferencing a NULL pointer in SpdyCl...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/323#issuecomment-154125097
Yup, ```SpdyRequest``` has the same issue. I have no objection to improving the way ```ClassAllocator``` works in a separate ticket. One thing you could explore is whether ```ClassAllocator``` should call the constructor and destructors manually, which would remove the need for the init/destroy pattern.
---
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: Dereferencing a NULL pointer in SpdyCl...
Posted by bgaff <gi...@git.apache.org>.
Github user bgaff commented on the pull request:
https://github.com/apache/trafficserver/pull/323#issuecomment-153956235
@jpeach , given that we follow an init() / clear() pattern anyway, what are your thoughts on that actually: just remove the call to clear() from the destructor.
---
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: Dereferencing a NULL pointer in SpdyCl...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/323#issuecomment-153951633
Yeh that's the way these object usually work. ```SpdyClientSession``` is a bit different from the other client session objects because last time I cleaned this up I didn't have a way to build and test SPDY. But in general we don't code the destroy to work for partially-initialized objects. It sounds that there is a path where a partially-initialized ```SpdyClientSession``` can be destroyed. I didn't see that in the quick look I took though :-/
---
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: Dereferencing a NULL pointer in SpdyCl...
Posted by bgaff <gi...@git.apache.org>.
Github user bgaff commented on the pull request:
https://github.com/apache/trafficserver/pull/323#issuecomment-153948633
I don't see an issue w/ this, I'll merge it if no one else has issues.
---
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: Dereferencing a NULL pointer in SpdyCl...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/trafficserver/pull/323
---
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: Dereferencing a NULL pointer in SpdyCl...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/323#issuecomment-153956478
Ah I see now. Then probably the right fix is to remove ```~SpdyClientSession()```. I don't think any other proxy-allocated object have destructors, since that can't really work. While you are at it, could you look at removing ```SpdyClientSession::clear()``` and just implementing ```SpdyClientSession::destroy()```?
It looks like someone had a similar problem in ```SpdyRequest::clear()``` ...
---
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: Dereferencing a NULL pointer in SpdyCl...
Posted by bgaff <gi...@git.apache.org>.
Github user bgaff commented on the pull request:
https://github.com/apache/trafficserver/pull/323#issuecomment-153955267
these objects should _never_ go out of scope until the process dies (they live on a freelist), so isn't it enough just to do nothing in the destructor?
---
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: Dereferencing a NULL pointer in SpdyCl...
Posted by canselcik <gi...@git.apache.org>.
Github user canselcik commented on the pull request:
https://github.com/apache/trafficserver/pull/323#issuecomment-154213497
Sounds good. So why don't we move forward with this PR as is, and add the
```
void SpdyClientSession::clear()
{
if (!mutex)
return; // this object wasn't initialized.
```
to match
```
void SpdyRequest::clear()
{
if (!spdy_sm)
return; // this object wasn't initialized.
```
As a separate PR, we can discuss the possibility of improving the way `ClassAllocator` works. As we discussed, we can switch `ClassAllocator::proto` to a pointer to assure that the dtor on the prototype is never called.
---
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: Dereferencing a NULL pointer in SpdyCl...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/323#issuecomment-154250586
I agree that this change doesn't make anything worse :) Would you mind making a followup change to remove the destructors in the SPDY code?
---
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: Dereferencing a NULL pointer in SpdyCl...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/323#issuecomment-153950616
But the only way this could happen is if the session was released before ```init``` was called, right?
---
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: Dereferencing a NULL pointer in SpdyCl...
Posted by bgaff <gi...@git.apache.org>.
Github user bgaff commented on the pull request:
https://github.com/apache/trafficserver/pull/323#issuecomment-153956757
Cool, sounds like we're on the same page @jpeach.
@canselcik, you also cool w/ this proposal?
---
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: Dereferencing a NULL pointer in SpdyCl...
Posted by canselcik <gi...@git.apache.org>.
Github user canselcik commented on the pull request:
https://github.com/apache/trafficserver/pull/323#issuecomment-154269760
Thanks for the merge, and sounds good, I will look into removing the destructors in the SPDY code and improvements to `ClassAllocator` later today.
---
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: Dereferencing a NULL pointer in SpdyCl...
Posted by canselcik <gi...@git.apache.org>.
Github user canselcik commented on the pull request:
https://github.com/apache/trafficserver/pull/323#issuecomment-153954924
Upon exit `static ClassAllocator<SpdyClientSession> spdyClientSessionAllocator` goes out of scope and its destructor is called, which goes on to call the destructor on the `SpdyClientSession` prototype.
---
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.
---