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" <[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; >>>> >>> >> >
