You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@trafficserver.apache.org by James Peach <jp...@apache.org> on 2014/10/03 17:28:37 UTC
Re: git commit: [TS-3112] - Add null pointer check for contp to prevent core dump after handleEvent(TS_FETCH_EVENT_EXT_HEAD_DONE)
On Oct 3, 2014, at 6:29 AM, sudheerv@apache.org wrote:
> Repository: trafficserver
> Updated Branches:
> refs/heads/master 33f651c90 -> d1b3dc66b
>
>
> [TS-3112] - Add null pointer check for contp to prevent core dump after handleEvent(TS_FETCH_EVENT_EXT_HEAD_DONE)
>
>
> Project: http://git-wip-us.apache.org/repos/asf/trafficserver/repo
> Commit: http://git-wip-us.apache.org/repos/asf/trafficserver/commit/d1b3dc66
> Tree: http://git-wip-us.apache.org/repos/asf/trafficserver/tree/d1b3dc66
> Diff: http://git-wip-us.apache.org/repos/asf/trafficserver/diff/d1b3dc66
>
> Branch: refs/heads/master
> Commit: d1b3dc66b5725879949350890ab014cf235cae64
> Parents: 33f651c
> Author: Sudheer Vinukonda <su...@yahoo-inc.com>
> Authored: Fri Oct 3 13:29:03 2014 +0000
> Committer: Sudheer Vinukonda <su...@yahoo-inc.com>
> Committed: Fri Oct 3 13:29:03 2014 +0000
>
> ----------------------------------------------------------------------
> proxy/FetchSM.cc | 3 +++
> 1 file changed, 3 insertions(+)
> ----------------------------------------------------------------------
>
>
> http://git-wip-us.apache.org/repos/asf/trafficserver/blob/d1b3dc66/proxy/FetchSM.cc
> ----------------------------------------------------------------------
> diff --git a/proxy/FetchSM.cc b/proxy/FetchSM.cc
> index d7b187a..4a79db4 100644
> --- a/proxy/FetchSM.cc
> +++ b/proxy/FetchSM.cc
> @@ -249,6 +249,9 @@ FetchSM::InvokePluginExt(int fetch_event)
> has_sent_header = true;
> }
>
> + if (!contp)
> + goto out;
> +
There's a check for contp being NULL just 10 lines above here ... how can it become NULL now?
> if (!has_body()) {
> contp->handleEvent(TS_FETCH_EVENT_EXT_BODY_DONE, this);
> goto out;
>
Re: git commit: [TS-3112] - Add null pointer check for contp to prevent core dump after handleEvent(TS_FETCH_EVENT_EXT_HEAD_DONE)
Posted by James Peach <jp...@apache.org>.
On Oct 3, 2014, at 8:28 AM, James Peach <jp...@apache.org> wrote:
> On Oct 3, 2014, at 6:29 AM, sudheerv@apache.org wrote:
>
>> Repository: trafficserver
>> Updated Branches:
>> refs/heads/master 33f651c90 -> d1b3dc66b
>>
>>
>> [TS-3112] - Add null pointer check for contp to prevent core dump after handleEvent(TS_FETCH_EVENT_EXT_HEAD_DONE)
>>
>>
>> Project: http://git-wip-us.apache.org/repos/asf/trafficserver/repo
>> Commit: http://git-wip-us.apache.org/repos/asf/trafficserver/commit/d1b3dc66
>> Tree: http://git-wip-us.apache.org/repos/asf/trafficserver/tree/d1b3dc66
>> Diff: http://git-wip-us.apache.org/repos/asf/trafficserver/diff/d1b3dc66
>>
>> Branch: refs/heads/master
>> Commit: d1b3dc66b5725879949350890ab014cf235cae64
>> Parents: 33f651c
>> Author: Sudheer Vinukonda <su...@yahoo-inc.com>
>> Authored: Fri Oct 3 13:29:03 2014 +0000
>> Committer: Sudheer Vinukonda <su...@yahoo-inc.com>
>> Committed: Fri Oct 3 13:29:03 2014 +0000
>>
>> ----------------------------------------------------------------------
>> proxy/FetchSM.cc | 3 +++
>> 1 file changed, 3 insertions(+)
>> ----------------------------------------------------------------------
>>
>>
>> http://git-wip-us.apache.org/repos/asf/trafficserver/blob/d1b3dc66/proxy/FetchSM.cc
>> ----------------------------------------------------------------------
>> diff --git a/proxy/FetchSM.cc b/proxy/FetchSM.cc
>> index d7b187a..4a79db4 100644
>> --- a/proxy/FetchSM.cc
>> +++ b/proxy/FetchSM.cc
>> @@ -249,6 +249,9 @@ FetchSM::InvokePluginExt(int fetch_event)
>> has_sent_header = true;
>> }
>>
>> + if (!contp)
>> + goto out;
>> +
>
> There's a check for contp being NULL just 10 lines above here ... how can it become NULL now?
Also, InvokePluginExt weirdly mixes sending plugin events and de-chunking ... if there's no continuation, don't you still need to de-chunk? Any why is there even an InvokePlugin and InkvokePluginExt?
J
Re: git commit: [TS-3112] - Add null pointer check for contp to prevent core dump after handleEvent(TS_FETCH_EVENT_EXT_HEAD_DONE)
Posted by James Peach <jp...@apache.org>.
On Oct 3, 2014, at 8:28 AM, James Peach <jp...@apache.org> wrote:
> On Oct 3, 2014, at 6:29 AM, sudheerv@apache.org wrote:
>
>> Repository: trafficserver
>> Updated Branches:
>> refs/heads/master 33f651c90 -> d1b3dc66b
>>
>>
>> [TS-3112] - Add null pointer check for contp to prevent core dump after handleEvent(TS_FETCH_EVENT_EXT_HEAD_DONE)
>>
>>
>> Project: http://git-wip-us.apache.org/repos/asf/trafficserver/repo
>> Commit: http://git-wip-us.apache.org/repos/asf/trafficserver/commit/d1b3dc66
>> Tree: http://git-wip-us.apache.org/repos/asf/trafficserver/tree/d1b3dc66
>> Diff: http://git-wip-us.apache.org/repos/asf/trafficserver/diff/d1b3dc66
>>
>> Branch: refs/heads/master
>> Commit: d1b3dc66b5725879949350890ab014cf235cae64
>> Parents: 33f651c
>> Author: Sudheer Vinukonda <su...@yahoo-inc.com>
>> Authored: Fri Oct 3 13:29:03 2014 +0000
>> Committer: Sudheer Vinukonda <su...@yahoo-inc.com>
>> Committed: Fri Oct 3 13:29:03 2014 +0000
>>
>> ----------------------------------------------------------------------
>> proxy/FetchSM.cc | 3 +++
>> 1 file changed, 3 insertions(+)
>> ----------------------------------------------------------------------
>>
>>
>> http://git-wip-us.apache.org/repos/asf/trafficserver/blob/d1b3dc66/proxy/FetchSM.cc
>> ----------------------------------------------------------------------
>> diff --git a/proxy/FetchSM.cc b/proxy/FetchSM.cc
>> index d7b187a..4a79db4 100644
>> --- a/proxy/FetchSM.cc
>> +++ b/proxy/FetchSM.cc
>> @@ -249,6 +249,9 @@ FetchSM::InvokePluginExt(int fetch_event)
>> has_sent_header = true;
>> }
>>
>> + if (!contp)
>> + goto out;
>> +
>
> There's a check for contp being NULL just 10 lines above here ... how can it become NULL now?
Also, InvokePluginExt weirdly mixes sending plugin events and de-chunking ... if there's no continuation, don't you still need to de-chunk? Any why is there even an InvokePlugin and InkvokePluginExt?
J
Re: git commit: [TS-3112] - Add null pointer check for contp to
prevent core dump after handleEvent(TS_FETCH_EVENT_EXT_HEAD_DONE)
Posted by Sudheer Vinukonda <su...@yahoo-inc.com.INVALID>.
Umm, not sure - I suppose we will need a check there as well.
Thanks,
Sudheer
On 10/3/14, 9:30 AM, "James Peach" <jp...@apache.org> wrote:
>
>On Oct 3, 2014, at 9:01 AM, Sudheer Vinukonda
><su...@yahoo-inc.com.INVALID> wrote:
>
>> Yeah, true - But, I scanned through the rest of the function and only
>> found this place as missing the check. The rest of branches all use
>>“goto
>> out” conveniently which checks “contp” again.
>
>Is this loop guaranteed to only execute once?
>
> do {
> if (chunked_handler.state == ChunkedHandler::CHUNK_FLOW_CONTROL) {
> chunked_handler.state = ChunkedHandler::CHUNK_READ_SIZE_START;
> }
>
> event = dechunk_body();
> if (!event) {
> read_vio->reenable();
> goto out;
> }
>
> contp->handleEvent(event, this);
> } while (chunked_handler.state == ChunkedHandler::CHUNK_FLOW_CONTROL);
>
>
>>
>> Thanks,
>>
>> Sudheer
>>
>> On 10/3/14, 8:57 AM, "James Peach" <ja...@me.com> wrote:
>>
>>> On Oct 3, 2014, at 8:35 AM, Sudheer Vinukonda
>>> <su...@yahoo-inc.com.INVALID> wrote:
>>>
>>>> Hi James,
>>>>
>>>> handleEvent() effectively calls the plugin (or in this case, SPDY
>>>>layer)
>>>> which may call TSFetchDestroy in error conditions. TSFetchDestroy sets
>>>> contp to NULL, but, doesn¹t destroy FetchSM yet, since, it¹s in a
>>>>tight
>>>> loop protected by ³recursion² counter. When handleEvent returns,
>>>> recursion
>>>> is decremented and contp is already null, so, FetchSM gets destroyed.
>>>
>>> Ah. In that case, there's a number of other places in InvokePluginExt
>>> where that can happen ...
>>>
>>>>
>>>> Thanks,
>>>>
>>>> Sudheer
>>>>
>>>> On 10/3/14, 8:28 AM, "James Peach" <jp...@apache.org> wrote:
>>>>
>>>>> On Oct 3, 2014, at 6:29 AM, sudheerv@apache.org wrote:
>>>>>
>>>>>> Repository: trafficserver
>>>>>> Updated Branches:
>>>>>> refs/heads/master 33f651c90 -> d1b3dc66b
>>>>>>
>>>>>>
>>>>>> [TS-3112] - Add null pointer check for contp to prevent core dump
>>>>>> after
>>>>>> handleEvent(TS_FETCH_EVENT_EXT_HEAD_DONE)
>>>>>>
>>>>>>
>>>>>> Project: http://git-wip-us.apache.org/repos/asf/trafficserver/repo
>>>>>> Commit:
>>>>>> http://git-wip-us.apache.org/repos/asf/trafficserver/commit/d1b3dc66
>>>>>> Tree:
>>>>>> http://git-wip-us.apache.org/repos/asf/trafficserver/tree/d1b3dc66
>>>>>> Diff:
>>>>>> http://git-wip-us.apache.org/repos/asf/trafficserver/diff/d1b3dc66
>>>>>>
>>>>>> Branch: refs/heads/master
>>>>>> Commit: d1b3dc66b5725879949350890ab014cf235cae64
>>>>>> Parents: 33f651c
>>>>>> Author: Sudheer Vinukonda <su...@yahoo-inc.com>
>>>>>> Authored: Fri Oct 3 13:29:03 2014 +0000
>>>>>> Committer: Sudheer Vinukonda <su...@yahoo-inc.com>
>>>>>> Committed: Fri Oct 3 13:29:03 2014 +0000
>>>>>>
>>>>>>
>>>>>>---------------------------------------------------------------------
>>>>>>-
>>>>>> proxy/FetchSM.cc | 3 +++
>>>>>> 1 file changed, 3 insertions(+)
>>>>>>
>>>>>>---------------------------------------------------------------------
>>>>>>-
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>http://git-wip-us.apache.org/repos/asf/trafficserver/blob/d1b3dc66/pr
>>>>>>ox
>>>>>> y/
>>>>>> FetchSM.cc
>>>>>>
>>>>>>---------------------------------------------------------------------
>>>>>>-
>>>>>> diff --git a/proxy/FetchSM.cc b/proxy/FetchSM.cc
>>>>>> index d7b187a..4a79db4 100644
>>>>>> --- a/proxy/FetchSM.cc
>>>>>> +++ b/proxy/FetchSM.cc
>>>>>> @@ -249,6 +249,9 @@ FetchSM::InvokePluginExt(int fetch_event)
>>>>>> has_sent_header = true;
>>>>>> }
>>>>>>
>>>>>> + if (!contp)
>>>>>> + goto out;
>>>>>> +
>>>>>
>>>>> There's a check for contp being NULL just 10 lines above here ... how
>>>>> can
>>>>> it become NULL now?
>>>>>
>>>>>
>>>>>> if (!has_body()) {
>>>>>> contp->handleEvent(TS_FETCH_EVENT_EXT_BODY_DONE, this);
>>>>>> goto out;
>>>>>>
>>>>>
>>>>
>>>
>>
>
Re: git commit: [TS-3112] - Add null pointer check for contp to prevent core dump after handleEvent(TS_FETCH_EVENT_EXT_HEAD_DONE)
Posted by James Peach <jp...@apache.org>.
On Oct 3, 2014, at 9:01 AM, Sudheer Vinukonda <su...@yahoo-inc.com.INVALID> wrote:
> Yeah, true - But, I scanned through the rest of the function and only
> found this place as missing the check. The rest of branches all use “goto
> out” conveniently which checks “contp” again.
Is this loop guaranteed to only execute once?
do {
if (chunked_handler.state == ChunkedHandler::CHUNK_FLOW_CONTROL) {
chunked_handler.state = ChunkedHandler::CHUNK_READ_SIZE_START;
}
event = dechunk_body();
if (!event) {
read_vio->reenable();
goto out;
}
contp->handleEvent(event, this);
} while (chunked_handler.state == ChunkedHandler::CHUNK_FLOW_CONTROL);
>
> Thanks,
>
> Sudheer
>
> On 10/3/14, 8:57 AM, "James Peach" <ja...@me.com> wrote:
>
>> On Oct 3, 2014, at 8:35 AM, Sudheer Vinukonda
>> <su...@yahoo-inc.com.INVALID> wrote:
>>
>>> Hi James,
>>>
>>> handleEvent() effectively calls the plugin (or in this case, SPDY layer)
>>> which may call TSFetchDestroy in error conditions. TSFetchDestroy sets
>>> contp to NULL, but, doesn¹t destroy FetchSM yet, since, it¹s in a tight
>>> loop protected by ³recursion² counter. When handleEvent returns,
>>> recursion
>>> is decremented and contp is already null, so, FetchSM gets destroyed.
>>
>> Ah. In that case, there's a number of other places in InvokePluginExt
>> where that can happen ...
>>
>>>
>>> Thanks,
>>>
>>> Sudheer
>>>
>>> On 10/3/14, 8:28 AM, "James Peach" <jp...@apache.org> wrote:
>>>
>>>> On Oct 3, 2014, at 6:29 AM, sudheerv@apache.org wrote:
>>>>
>>>>> Repository: trafficserver
>>>>> Updated Branches:
>>>>> refs/heads/master 33f651c90 -> d1b3dc66b
>>>>>
>>>>>
>>>>> [TS-3112] - Add null pointer check for contp to prevent core dump
>>>>> after
>>>>> handleEvent(TS_FETCH_EVENT_EXT_HEAD_DONE)
>>>>>
>>>>>
>>>>> Project: http://git-wip-us.apache.org/repos/asf/trafficserver/repo
>>>>> Commit:
>>>>> http://git-wip-us.apache.org/repos/asf/trafficserver/commit/d1b3dc66
>>>>> Tree:
>>>>> http://git-wip-us.apache.org/repos/asf/trafficserver/tree/d1b3dc66
>>>>> Diff:
>>>>> http://git-wip-us.apache.org/repos/asf/trafficserver/diff/d1b3dc66
>>>>>
>>>>> Branch: refs/heads/master
>>>>> Commit: d1b3dc66b5725879949350890ab014cf235cae64
>>>>> Parents: 33f651c
>>>>> Author: Sudheer Vinukonda <su...@yahoo-inc.com>
>>>>> Authored: Fri Oct 3 13:29:03 2014 +0000
>>>>> Committer: Sudheer Vinukonda <su...@yahoo-inc.com>
>>>>> Committed: Fri Oct 3 13:29:03 2014 +0000
>>>>>
>>>>> ----------------------------------------------------------------------
>>>>> proxy/FetchSM.cc | 3 +++
>>>>> 1 file changed, 3 insertions(+)
>>>>> ----------------------------------------------------------------------
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> http://git-wip-us.apache.org/repos/asf/trafficserver/blob/d1b3dc66/prox
>>>>> y/
>>>>> FetchSM.cc
>>>>> ----------------------------------------------------------------------
>>>>> diff --git a/proxy/FetchSM.cc b/proxy/FetchSM.cc
>>>>> index d7b187a..4a79db4 100644
>>>>> --- a/proxy/FetchSM.cc
>>>>> +++ b/proxy/FetchSM.cc
>>>>> @@ -249,6 +249,9 @@ FetchSM::InvokePluginExt(int fetch_event)
>>>>> has_sent_header = true;
>>>>> }
>>>>>
>>>>> + if (!contp)
>>>>> + goto out;
>>>>> +
>>>>
>>>> There's a check for contp being NULL just 10 lines above here ... how
>>>> can
>>>> it become NULL now?
>>>>
>>>>
>>>>> if (!has_body()) {
>>>>> contp->handleEvent(TS_FETCH_EVENT_EXT_BODY_DONE, this);
>>>>> goto out;
>>>>>
>>>>
>>>
>>
>
Re: git commit: [TS-3112] - Add null pointer check for contp to prevent core dump after handleEvent(TS_FETCH_EVENT_EXT_HEAD_DONE)
Posted by James Peach <jp...@apache.org>.
On Oct 3, 2014, at 9:01 AM, Sudheer Vinukonda <su...@yahoo-inc.com.INVALID> wrote:
> Yeah, true - But, I scanned through the rest of the function and only
> found this place as missing the check. The rest of branches all use “goto
> out” conveniently which checks “contp” again.
Is this loop guaranteed to only execute once?
do {
if (chunked_handler.state == ChunkedHandler::CHUNK_FLOW_CONTROL) {
chunked_handler.state = ChunkedHandler::CHUNK_READ_SIZE_START;
}
event = dechunk_body();
if (!event) {
read_vio->reenable();
goto out;
}
contp->handleEvent(event, this);
} while (chunked_handler.state == ChunkedHandler::CHUNK_FLOW_CONTROL);
>
> Thanks,
>
> Sudheer
>
> On 10/3/14, 8:57 AM, "James Peach" <ja...@me.com> wrote:
>
>> On Oct 3, 2014, at 8:35 AM, Sudheer Vinukonda
>> <su...@yahoo-inc.com.INVALID> wrote:
>>
>>> Hi James,
>>>
>>> handleEvent() effectively calls the plugin (or in this case, SPDY layer)
>>> which may call TSFetchDestroy in error conditions. TSFetchDestroy sets
>>> contp to NULL, but, doesn¹t destroy FetchSM yet, since, it¹s in a tight
>>> loop protected by ³recursion² counter. When handleEvent returns,
>>> recursion
>>> is decremented and contp is already null, so, FetchSM gets destroyed.
>>
>> Ah. In that case, there's a number of other places in InvokePluginExt
>> where that can happen ...
>>
>>>
>>> Thanks,
>>>
>>> Sudheer
>>>
>>> On 10/3/14, 8:28 AM, "James Peach" <jp...@apache.org> wrote:
>>>
>>>> On Oct 3, 2014, at 6:29 AM, sudheerv@apache.org wrote:
>>>>
>>>>> Repository: trafficserver
>>>>> Updated Branches:
>>>>> refs/heads/master 33f651c90 -> d1b3dc66b
>>>>>
>>>>>
>>>>> [TS-3112] - Add null pointer check for contp to prevent core dump
>>>>> after
>>>>> handleEvent(TS_FETCH_EVENT_EXT_HEAD_DONE)
>>>>>
>>>>>
>>>>> Project: http://git-wip-us.apache.org/repos/asf/trafficserver/repo
>>>>> Commit:
>>>>> http://git-wip-us.apache.org/repos/asf/trafficserver/commit/d1b3dc66
>>>>> Tree:
>>>>> http://git-wip-us.apache.org/repos/asf/trafficserver/tree/d1b3dc66
>>>>> Diff:
>>>>> http://git-wip-us.apache.org/repos/asf/trafficserver/diff/d1b3dc66
>>>>>
>>>>> Branch: refs/heads/master
>>>>> Commit: d1b3dc66b5725879949350890ab014cf235cae64
>>>>> Parents: 33f651c
>>>>> Author: Sudheer Vinukonda <su...@yahoo-inc.com>
>>>>> Authored: Fri Oct 3 13:29:03 2014 +0000
>>>>> Committer: Sudheer Vinukonda <su...@yahoo-inc.com>
>>>>> Committed: Fri Oct 3 13:29:03 2014 +0000
>>>>>
>>>>> ----------------------------------------------------------------------
>>>>> proxy/FetchSM.cc | 3 +++
>>>>> 1 file changed, 3 insertions(+)
>>>>> ----------------------------------------------------------------------
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> http://git-wip-us.apache.org/repos/asf/trafficserver/blob/d1b3dc66/prox
>>>>> y/
>>>>> FetchSM.cc
>>>>> ----------------------------------------------------------------------
>>>>> diff --git a/proxy/FetchSM.cc b/proxy/FetchSM.cc
>>>>> index d7b187a..4a79db4 100644
>>>>> --- a/proxy/FetchSM.cc
>>>>> +++ b/proxy/FetchSM.cc
>>>>> @@ -249,6 +249,9 @@ FetchSM::InvokePluginExt(int fetch_event)
>>>>> has_sent_header = true;
>>>>> }
>>>>>
>>>>> + if (!contp)
>>>>> + goto out;
>>>>> +
>>>>
>>>> There's a check for contp being NULL just 10 lines above here ... how
>>>> can
>>>> it become NULL now?
>>>>
>>>>
>>>>> if (!has_body()) {
>>>>> contp->handleEvent(TS_FETCH_EVENT_EXT_BODY_DONE, this);
>>>>> goto out;
>>>>>
>>>>
>>>
>>
>
Re: git commit: [TS-3112] - Add null pointer check for contp to
prevent core dump after handleEvent(TS_FETCH_EVENT_EXT_HEAD_DONE)
Posted by Sudheer Vinukonda <su...@yahoo-inc.com.INVALID>.
Yeah, true - But, I scanned through the rest of the function and only
found this place as missing the check. The rest of branches all use “goto
out” conveniently which checks “contp” again.
Thanks,
Sudheer
On 10/3/14, 8:57 AM, "James Peach" <ja...@me.com> wrote:
>On Oct 3, 2014, at 8:35 AM, Sudheer Vinukonda
><su...@yahoo-inc.com.INVALID> wrote:
>
>> Hi James,
>>
>> handleEvent() effectively calls the plugin (or in this case, SPDY layer)
>> which may call TSFetchDestroy in error conditions. TSFetchDestroy sets
>> contp to NULL, but, doesn¹t destroy FetchSM yet, since, it¹s in a tight
>> loop protected by ³recursion² counter. When handleEvent returns,
>>recursion
>> is decremented and contp is already null, so, FetchSM gets destroyed.
>
>Ah. In that case, there's a number of other places in InvokePluginExt
>where that can happen ...
>
>>
>> Thanks,
>>
>> Sudheer
>>
>> On 10/3/14, 8:28 AM, "James Peach" <jp...@apache.org> wrote:
>>
>>> On Oct 3, 2014, at 6:29 AM, sudheerv@apache.org wrote:
>>>
>>>> Repository: trafficserver
>>>> Updated Branches:
>>>> refs/heads/master 33f651c90 -> d1b3dc66b
>>>>
>>>>
>>>> [TS-3112] - Add null pointer check for contp to prevent core dump
>>>>after
>>>> handleEvent(TS_FETCH_EVENT_EXT_HEAD_DONE)
>>>>
>>>>
>>>> Project: http://git-wip-us.apache.org/repos/asf/trafficserver/repo
>>>> Commit:
>>>> http://git-wip-us.apache.org/repos/asf/trafficserver/commit/d1b3dc66
>>>> Tree:
>>>>http://git-wip-us.apache.org/repos/asf/trafficserver/tree/d1b3dc66
>>>> Diff:
>>>>http://git-wip-us.apache.org/repos/asf/trafficserver/diff/d1b3dc66
>>>>
>>>> Branch: refs/heads/master
>>>> Commit: d1b3dc66b5725879949350890ab014cf235cae64
>>>> Parents: 33f651c
>>>> Author: Sudheer Vinukonda <su...@yahoo-inc.com>
>>>> Authored: Fri Oct 3 13:29:03 2014 +0000
>>>> Committer: Sudheer Vinukonda <su...@yahoo-inc.com>
>>>> Committed: Fri Oct 3 13:29:03 2014 +0000
>>>>
>>>> ----------------------------------------------------------------------
>>>> proxy/FetchSM.cc | 3 +++
>>>> 1 file changed, 3 insertions(+)
>>>> ----------------------------------------------------------------------
>>>>
>>>>
>>>>
>>>>
>>>>http://git-wip-us.apache.org/repos/asf/trafficserver/blob/d1b3dc66/prox
>>>>y/
>>>> FetchSM.cc
>>>> ----------------------------------------------------------------------
>>>> diff --git a/proxy/FetchSM.cc b/proxy/FetchSM.cc
>>>> index d7b187a..4a79db4 100644
>>>> --- a/proxy/FetchSM.cc
>>>> +++ b/proxy/FetchSM.cc
>>>> @@ -249,6 +249,9 @@ FetchSM::InvokePluginExt(int fetch_event)
>>>> has_sent_header = true;
>>>> }
>>>>
>>>> + if (!contp)
>>>> + goto out;
>>>> +
>>>
>>> There's a check for contp being NULL just 10 lines above here ... how
>>>can
>>> it become NULL now?
>>>
>>>
>>>> if (!has_body()) {
>>>> contp->handleEvent(TS_FETCH_EVENT_EXT_BODY_DONE, this);
>>>> goto out;
>>>>
>>>
>>
>
Re: git commit: [TS-3112] - Add null pointer check for contp to
prevent core dump after handleEvent(TS_FETCH_EVENT_EXT_HEAD_DONE)
Posted by James Peach <ja...@me.com>.
On Oct 3, 2014, at 8:35 AM, Sudheer Vinukonda <su...@yahoo-inc.com.INVALID> wrote:
> Hi James,
>
> handleEvent() effectively calls the plugin (or in this case, SPDY layer)
> which may call TSFetchDestroy in error conditions. TSFetchDestroy sets
> contp to NULL, but, doesn¹t destroy FetchSM yet, since, it¹s in a tight
> loop protected by ³recursion² counter. When handleEvent returns, recursion
> is decremented and contp is already null, so, FetchSM gets destroyed.
Ah. In that case, there's a number of other places in InvokePluginExt where that can happen ...
>
> Thanks,
>
> Sudheer
>
> On 10/3/14, 8:28 AM, "James Peach" <jp...@apache.org> wrote:
>
>> On Oct 3, 2014, at 6:29 AM, sudheerv@apache.org wrote:
>>
>>> Repository: trafficserver
>>> Updated Branches:
>>> refs/heads/master 33f651c90 -> d1b3dc66b
>>>
>>>
>>> [TS-3112] - Add null pointer check for contp to prevent core dump after
>>> handleEvent(TS_FETCH_EVENT_EXT_HEAD_DONE)
>>>
>>>
>>> Project: http://git-wip-us.apache.org/repos/asf/trafficserver/repo
>>> Commit:
>>> http://git-wip-us.apache.org/repos/asf/trafficserver/commit/d1b3dc66
>>> Tree: http://git-wip-us.apache.org/repos/asf/trafficserver/tree/d1b3dc66
>>> Diff: http://git-wip-us.apache.org/repos/asf/trafficserver/diff/d1b3dc66
>>>
>>> Branch: refs/heads/master
>>> Commit: d1b3dc66b5725879949350890ab014cf235cae64
>>> Parents: 33f651c
>>> Author: Sudheer Vinukonda <su...@yahoo-inc.com>
>>> Authored: Fri Oct 3 13:29:03 2014 +0000
>>> Committer: Sudheer Vinukonda <su...@yahoo-inc.com>
>>> Committed: Fri Oct 3 13:29:03 2014 +0000
>>>
>>> ----------------------------------------------------------------------
>>> proxy/FetchSM.cc | 3 +++
>>> 1 file changed, 3 insertions(+)
>>> ----------------------------------------------------------------------
>>>
>>>
>>>
>>> http://git-wip-us.apache.org/repos/asf/trafficserver/blob/d1b3dc66/proxy/
>>> FetchSM.cc
>>> ----------------------------------------------------------------------
>>> diff --git a/proxy/FetchSM.cc b/proxy/FetchSM.cc
>>> index d7b187a..4a79db4 100644
>>> --- a/proxy/FetchSM.cc
>>> +++ b/proxy/FetchSM.cc
>>> @@ -249,6 +249,9 @@ FetchSM::InvokePluginExt(int fetch_event)
>>> has_sent_header = true;
>>> }
>>>
>>> + if (!contp)
>>> + goto out;
>>> +
>>
>> There's a check for contp being NULL just 10 lines above here ... how can
>> it become NULL now?
>>
>>
>>> if (!has_body()) {
>>> contp->handleEvent(TS_FETCH_EVENT_EXT_BODY_DONE, this);
>>> goto out;
>>>
>>
>
Re: git commit: [TS-3112] - Add null pointer check for contp to
prevent core dump after handleEvent(TS_FETCH_EVENT_EXT_HEAD_DONE)
Posted by Sudheer Vinukonda <su...@yahoo-inc.com.INVALID>.
Hi James,
handleEvent() effectively calls the plugin (or in this case, SPDY layer)
which may call TSFetchDestroy in error conditions. TSFetchDestroy sets
contp to NULL, but, doesn¹t destroy FetchSM yet, since, it¹s in a tight
loop protected by ³recursion² counter. When handleEvent returns, recursion
is decremented and contp is already null, so, FetchSM gets destroyed.
Thanks,
Sudheer
On 10/3/14, 8:28 AM, "James Peach" <jp...@apache.org> wrote:
>On Oct 3, 2014, at 6:29 AM, sudheerv@apache.org wrote:
>
>> Repository: trafficserver
>> Updated Branches:
>> refs/heads/master 33f651c90 -> d1b3dc66b
>>
>>
>> [TS-3112] - Add null pointer check for contp to prevent core dump after
>>handleEvent(TS_FETCH_EVENT_EXT_HEAD_DONE)
>>
>>
>> Project: http://git-wip-us.apache.org/repos/asf/trafficserver/repo
>> Commit:
>>http://git-wip-us.apache.org/repos/asf/trafficserver/commit/d1b3dc66
>> Tree: http://git-wip-us.apache.org/repos/asf/trafficserver/tree/d1b3dc66
>> Diff: http://git-wip-us.apache.org/repos/asf/trafficserver/diff/d1b3dc66
>>
>> Branch: refs/heads/master
>> Commit: d1b3dc66b5725879949350890ab014cf235cae64
>> Parents: 33f651c
>> Author: Sudheer Vinukonda <su...@yahoo-inc.com>
>> Authored: Fri Oct 3 13:29:03 2014 +0000
>> Committer: Sudheer Vinukonda <su...@yahoo-inc.com>
>> Committed: Fri Oct 3 13:29:03 2014 +0000
>>
>> ----------------------------------------------------------------------
>> proxy/FetchSM.cc | 3 +++
>> 1 file changed, 3 insertions(+)
>> ----------------------------------------------------------------------
>>
>>
>>
>>http://git-wip-us.apache.org/repos/asf/trafficserver/blob/d1b3dc66/proxy/
>>FetchSM.cc
>> ----------------------------------------------------------------------
>> diff --git a/proxy/FetchSM.cc b/proxy/FetchSM.cc
>> index d7b187a..4a79db4 100644
>> --- a/proxy/FetchSM.cc
>> +++ b/proxy/FetchSM.cc
>> @@ -249,6 +249,9 @@ FetchSM::InvokePluginExt(int fetch_event)
>> has_sent_header = true;
>> }
>>
>> + if (!contp)
>> + goto out;
>> +
>
>There's a check for contp being NULL just 10 lines above here ... how can
>it become NULL now?
>
>
>> if (!has_body()) {
>> contp->handleEvent(TS_FETCH_EVENT_EXT_BODY_DONE, this);
>> goto out;
>>
>