From 43216e2b97c71fba29d11945a5800e01f81e2d64 Mon Sep 17 00:00:00 2001 From: Graham Clark Date: Sun, 11 Apr 2021 23:53:52 -0400 Subject: [PATCH] First attempt to fix loader bug that means termshark doesn't quit This is an attempt to solve issue #121. I think my mistake is as follows: - I start the goroutine that runs the pcap process - after starting the process, I send its pid via a channel to another goroutine that controls process life-cycles - however that goroutine has closed down because the user issued a cancel The process lifetime goroutine should not shut down if it is guaranteed the pcap running goroutine is going to send its pid, because then nothing will listen for it. I've adjusted the logic so now that goroutine will definitely wait for the pid - unless it receives a pid==0 which indicates there was a problem starting the process. --- capinfo/loader.go | 2 +- convs/loader.go | 2 +- pcap/loader.go | 19 ++++++++++++++----- streams/loader.go | 4 ++-- 4 files changed, 18 insertions(+), 9 deletions(-) diff --git a/capinfo/loader.go b/capinfo/loader.go index 120e1d26..144e0bcb 100644 --- a/capinfo/loader.go +++ b/capinfo/loader.go @@ -146,7 +146,7 @@ func (c *Loader) loadCapinfoAsync(pcapf string, app gowid.IApp, cb ICapinfoCallb } } - if state == pcap.Terminated || (cancelledChan == nil && state == pcap.NotStarted) { + if state == pcap.Terminated || (procChan == nil && state == pcap.NotStarted) { break loop } } diff --git a/convs/loader.go b/convs/loader.go index 02d2214b..d6cdbfa0 100644 --- a/convs/loader.go +++ b/convs/loader.go @@ -158,7 +158,7 @@ func (c *Loader) loadConvAsync(pcapf string, convs []string, filter string, abs } } - if state == pcap.Terminated || (cancelledChan == nil && state == pcap.NotStarted) { + if state == pcap.Terminated || (procChan == nil && state == pcap.NotStarted) { break loop } } diff --git a/pcap/loader.go b/pcap/loader.go index bfc23fe3..2a11b79f 100644 --- a/pcap/loader.go +++ b/pcap/loader.go @@ -1015,8 +1015,17 @@ func (c *PdmlLoader) loadPcapSync(row int, visible bool, ps iPdmlLoaderEnv, cb i // a message means the proc has started // closed means it won't be started // if closed, then pdmlCmd == nil - if (pdmlState == Terminated || (pdmlCancelledChan == nil && pdmlState == NotStarted)) && - (pcapState == Terminated || (pcapCancelledChan == nil && pcapState == NotStarted)) { + // 04/11/21: I can't take a shortcut here and condition on Terminated || (cancelledChan == nil && NotStarted) + // See the pcap or pdml goroutines below. I block at the beginning, checking on the stage2 cancellation. + // If I get past that point, and there are no errors in the process invocation, I am guaranteed to start both + // the pdml and pcap processes. If there are errors, I am guaranteed to close the pcapPidChan with a defer. + // If I take a shortcut and end this goroutine via a stage2 cancellation before waiting for the pcap pid, + // then I'll block in that goroutine, trying to send to the pcapPidChan, but with nothing here to receive + // the value. In the pcap process goroutine, if I get past the stage2 cancellation check, then I need to + // have something to receive the pid - this goroutine. It needs to stay alive until it gets the pid, or a + // zero. + if (pdmlState == Terminated || (pdmlPidChan == nil && c.PdmlPid == 0)) && + (pcapState == Terminated || (pcapPidChan == nil && c.PcapPid == 0)) { // nothing to select on so break break loop } @@ -1464,7 +1473,7 @@ func (p *PsmlLoader) loadPsmlSync(iloader *InterfaceLoader, e iPsmlLoaderEnv, cb } } - if state == Terminated || (intCancelledChan == nil && state == NotStarted) { + if state == Terminated || (pidChan == nil && state == NotStarted) { break loop } } @@ -1590,7 +1599,7 @@ func (p *PsmlLoader) loadPsmlSync(iloader *InterfaceLoader, e iPsmlLoaderEnv, cb // successfully started then died/kill, OR // was never started, won't be started, and cancelled - if state == Terminated || (cancelledChan == nil && state == NotStarted) { + if state == Terminated || (pidChan == nil && state == NotStarted) { break loop } } @@ -2082,7 +2091,7 @@ func (i *InterfaceLoader) loadIfacesSync(e iIfaceLoaderEnv, cb interface{}, app // a message means the proc has started // closed means it won't be started // if closed, then pdmlCmd == nil - if state == Terminated || (cancelledChan == nil && state == NotStarted) { + if state == Terminated || (pidChan == nil && state == NotStarted) { // nothing to select on so break break loop } diff --git a/streams/loader.go b/streams/loader.go index 59169b8a..243fe494 100644 --- a/streams/loader.go +++ b/streams/loader.go @@ -179,7 +179,7 @@ func (c *Loader) loadStreamReassemblyAsync(pcapf string, proto string, idx int, } } - if state == pcap.Terminated || (cancelled == nil && state == pcap.NotStarted) { + if state == pcap.Terminated || (procChan == nil && state == pcap.NotStarted) { break loop } } @@ -300,7 +300,7 @@ func (c *Loader) startStreamIndexerAsync(pcapf string, proto string, idx int, ap } - if state == pcap.Terminated || (cancelledChan == nil && state == pcap.NotStarted) { + if state == pcap.Terminated || (procChan == nil && state == pcap.NotStarted) { break loop }