On Oct 3, 2014, at 9:01 AM, Sudheer Vinukonda <[email protected]>
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" <[email protected]> wrote:
>
>> On Oct 3, 2014, at 8:35 AM, Sudheer Vinukonda
>> <[email protected]> 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" <[email protected]> wrote:
>>>
>>>> On Oct 3, 2014, at 6:29 AM, [email protected] 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 <[email protected]>
>>>>> Authored: Fri Oct 3 13:29:03 2014 +0000
>>>>> Committer: Sudheer Vinukonda <[email protected]>
>>>>> 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;
>>>>>
>>>>
>>>
>>
>
