Umm, not sure - I suppose we will need a check there as well. Thanks,
Sudheer On 10/3/14, 9:30 AM, "James Peach" <[email protected]> wrote: > >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/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; >>>>>> >>>>> >>>> >>> >> >
