Merge remote-tracking branch 'origin/master' into v24docs

gcla · Jul 4, 2022 · eab04cc · eab04cc
2 parents 57f90d0 d72d5ad
commit eab04cc
Show file tree

Hide file tree

Showing 8 changed files with 136 additions and 86 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -13,6  13,7 @@
 ### Changed
 
 - Now you can build and install termshark with one command: `go install github.com/gcla/termshark/v2/cmd/termshark`
 - Fixed a race condition that caused extcap captures (e.g. randpkt) to sporadically fail.
 - Dark-mode is now the default in the absence of a specific user-setting.
 - Fixed a bug that caused mouse-clicks within the hex view to not function correctly if the viewport was not
   at the top of the data to be displayed.

diff --git a/cmd/termshark/termshark.go b/cmd/termshark/termshark.go
@@ -5,6  5,7 @@
 package main
 
 import (
 	"bytes"
 	"fmt"
 	"os"
 	"os/exec"
@@ -720,6  721,7 @@ func cmain() int {
 	//======================================================================
 
 	ifaceExitCode := 0
 	stderr := &bytes.Buffer{}
 	var ifaceErr error
 
 	// This is deferred until after the app is Closed - otherwise messages written to stdout/stderr are
@@ -731,6  733,20 @@ func cmain() int {
 				fmt.Fprintf(os.Stderr, ": %v", ifaceErr)
 			}
 			fmt.Fprintf(os.Stderr, " (exit code %d)\n", ifaceExitCode)
 			if stderr.Len() != 0 {
 				// The default capture bin is termshark itself, with a special environment
 				// variable set that causes it to try dumpcap, then tshark, in that order (for
 				// efficiency of capture, but falling back to tshark for extcap interfaces).
 				// But telling the user the capture process is "termshark" is misleading.
 				cbin, err1 := filepath.Abs(filepath.FromSlash(termshark.CaptureBin()))
 				def, err2 := filepath.Abs("termshark")
 				if err1 == nil && err2 == nil && cbin == def {
 					cbin = "the capture process"
 				}
 
 				fmt.Fprintf(os.Stderr, "Standard error stream from %s:\n", cbin)
 				fmt.Fprintf(os.Stderr, "------\n%s\n------\n", stderr.String())
 			}
 			if runtime.GOOS == "linux" && os.Geteuid() != 0 {
 				fmt.Fprintf(os.Stderr, "You might need: sudo setcap cap_net_raw,cap_net_admin eip %s\n", termshark.PrivilegedBin())
 				fmt.Fprintf(os.Stderr, "Or try running with sudo or as root.\n")
@@ -931,10  947,11 @@ func cmain() int {
 		ifaceExitCode = 0
 		for _, psrc := range psrcs {
 			if psrc.IsInterface() {
-				if ifaceExitCode, ifaceErr = termshark.RunForExitCode(
 				if ifaceExitCode, ifaceErr = termshark.RunForStderr(
 					termshark.CaptureBin(),
 					[]string{"-i", psrc.Name(), "-a", "duration:1"},
 					append(os.Environ(), "TERMSHARK_CAPTURE_MODE=1"),
 					stderr,
 				); ifaceExitCode != 0 {
 					return 1
 				}

diff --git a/pcap/loader.go b/pcap/loader.go
@@ -1284,6  1284,93 @@ func (c *PdmlLoader) loadPcapSync(row int, visible bool, ps iPdmlLoaderEnv, cb i
 
 }
 
 // waitForFileData sets an inotify watch on filename, and returns when a WRITE
 // event is seen.  There is special logic for the case where the file is
 // removed; then the watcher is deleted and reinstated. This is to handle a
 // specific loading bug in termshark due to an optimized packet capture
 // process. To capture packets, termshark runs itself with a special env var
 // set.  It detects this at startup, then launches dumpcap as the first
 // capture method. If this fails (e.g. the source is an extcap), it launches
 // tshark instead. Dumpcap is more efficient, but tshark is needed for the
 // extcap sources. The problem is that termshark needs a heuristic for when
 // packets have actually been detected - this is so it can wait to launch the
 // UI (in case a password is needed at the terminal, I don't want to obscure
 // that with the UI). So termshark waits for a WRITE to the pcap generated by
 // the capture process, and then launches tail. BUT - if dumpcap fails, it
 // will delete (unlink) the capture file passed to it with the -w argument
 // before tshark starts. If we don't watch for WRITE, this triggers the
 // notifier; then tail starts; then tail fails because depending on timing,
 // tshark may not have started yet and so the tail target pcap does not exist.
 // The fix is to monitor for inotify REMOVE too, and if seen, recreate the
 // pcap file (empty), and restart the watcher. And importantly, don't let
 // the tail process start until the WRITE event is seen.
 func waitForFileData(ctx context.Context, filename string, errFn func(error)) {
 OuterLoop:
 	for {
 		// this set up is so that I can detect when there are actually packets to read (e.g
 		// maybe there's no traffic on the interface). When there's something to read, the
 		// rest of the procedure can spring into action. Why not spring into action right away?
 		// Because the tail command needs a file to exist to watch it with -f. Can I rely on
 		// tail -F across all supported platforms? (e.g. Windows)
 		watcher, err := fsnotify.NewWatcher()
 		if err != nil {
 			err = fmt.Errorf("Could not create FS watch: %v", err)
 			errFn(err)
 			return
 		}
 		defer watcher.Close()
 
 		file, err := os.OpenFile(filename, os.O_RDWR|os.O_CREATE, 0644)
 		if err != nil {
 			err = fmt.Errorf("Could not touch temporary pcap file %s: %v", filename, err)
 			errFn(err)
 		}
 		file.Close()
 
 		if err := watcher.Add(filename); err != nil {
 			err = fmt.Errorf("Could not set up watcher for %s: %v", filename, err)
 			errFn(err)
 			return
 		}
 
 		removeWatcher := func(file string) {
 			if watcher != nil {
 				watcher.Remove(file)
 				watcher = nil
 			}
 		}
 
 		// Make sure that no matter what happens from here on, the watcher is not leaked. But we'll remove
 		// it earlier under normal operation so that setting and removing watches with new loaders do not
 		// race.
 		defer removeWatcher(filename)
 
 	NotifyLoop:
 		for {
 			select {
 			case fe := <-watcher.Events:
 				if fe.Name == filename {
 					switch fe.Op {
 					case fsnotify.Remove:
 						removeWatcher(filename)
 						continue OuterLoop
 					default:
 						break NotifyLoop
 					}
 				}
 			case err := <-watcher.Errors:
 				err = fmt.Errorf("Unexpected watcher error for %s: %v", filename, err)
 				errFn(err)
 				return
 			case <-ctx.Done():
 				return
 			}
 		}
 
 		break OuterLoop
 	}
 }
 
 // loadPsmlSync starts tshark processes, and other processes, to generate PSML
 // data. There is coordination with the PDML loader via a channel,
 // startStage2Chan. If a filter is set, then we might need to read far more
@@ -1626,73  1713,14 @@ func (p *PsmlLoader) loadPsmlSync(iloader *InterfaceLoader, e iPsmlLoaderEnv, cb
 
 			p.tailCmd.SetStdout(fifoPipeWriter)
 
-			// this set up is so that I can detect when there are actually packets to read (e.g
-			// maybe there's no traffic on the interface). When there's something to read, the
-			// rest of the procedure can spring into action. Why not spring into action right away?
-			// Because the tail command needs a file to exist to watch it with -f. Can I rely on
-			// tail -F across all supported platforms? (e.g. Windows)
-			watcher, err := fsnotify.NewWatcher()
-			if err != nil {
-				err = fmt.Errorf("Could not create FS watch: %v", err)
-				HandleError(PsmlCode, app, err, cb)
-				intPsmlCancelFn()
-				p.tailCancelFn() // needed to end the goroutine, end if tailcmd has not started
-				return
-			}
-			defer watcher.Close()
-
-			file, err := os.OpenFile(e.InterfaceFile(), os.O_RDWR|os.O_CREATE, 0644)
-			if err != nil {
-				err = fmt.Errorf("Could not touch temporary pcap file %s: %v", e.InterfaceFile(), err)
-				HandleError(PsmlCode, app, err, cb)
-				intPsmlCancelFn()
-				p.tailCancelFn() // needed to end the goroutine, end if tailcmd has not started
-			}
-			file.Close()
-
-			if err := watcher.Add(e.InterfaceFile()); err != nil {
-				err = fmt.Errorf("Could not set up watcher for %s: %v", e.InterfaceFile(), err)
-				HandleError(PsmlCode, app, err, cb)
-				intPsmlCancelFn()
-				p.tailCancelFn() // needed to end the goroutine, end if tailcmd has not started
-				return
-			}
-
-			removeWatcher := func(file string) {
-				if watcher != nil {
-					watcher.Remove(file)
-					watcher = nil
-				}
-			}
-
-			// Make sure that no matter what happens from here on, the watcher is not leaked. But we'll remove
-			// it earlier under normal operation so that setting and removing watches with new loaders do not
-			// race.
-			defer removeWatcher(e.InterfaceFile())
-
-		Loop:
-			for {
-				select {
-				case fe := <-watcher.Events:
-					if fe.Name == e.InterfaceFile() {
-						break Loop
-					}
-				case err := <-watcher.Errors:
-					err = fmt.Errorf("Unexpected watcher error for %s: %v", e.InterfaceFile(), err)
 			waitForFileData(intPsmlCtx,
 				e.InterfaceFile(),
 				func(err error) {
 					HandleError(PsmlCode, app, err, cb)
 					intPsmlCancelFn()
 					p.tailCancelFn() // needed to end the goroutine, end if tailcmd has not started
-					return
-				case <-intPsmlCtx.Done():
-					return
-				}
-			}
-
-			// Remove early if possible - because then if we clear the pcap and restart, we won't
-			// race the termination of this function with the starting of a new instance of it, meaning
-			// the new call adds the same watcher (idempotent) but then the terminating instance removes
-			// it
-			removeWatcher(e.InterfaceFile())
 				},
 			)
 
 			log.Infof("Starting Tail command: %v", p.tailCmd)
 

diff --git a/system/dumpcapext.go b/system/dumpcapext.go
@@ -8,13  8,12 @@
 package system
 
 import (
 	"fmt"
 	"os"
 	"os/exec"
 	"regexp"
 	"strconv"
 	"syscall"
-
-	log "github.com/sirupsen/logrus"
 )
 
 //======================================================================
@@ -45,31  44,32 @@ func DumpcapExt(dumpcapBin string, tsharkBin string, args ...string) error {
 			if len(fdnum) == 2 {
 				fd, err := strconv.Atoi(fdnum[1])
 				if err != nil {
-					log.Warnf("Unexpected error parsing %s: %v", args[1], err)
 					fmt.Fprintf(os.Stderr, "Unexpected error parsing %s: %v\n", args[1], err)
 				} else {
 					err = Dup2(fd, 0)
 					if err != nil {
-						log.Warnf("Problem duplicating fd %d to 0: %v", fd, err)
-						log.Warnf("Will not try to replace argument %s to tshark", args[1])
 						fmt.Fprintf(os.Stderr, "Problem duplicating fd %d to 0: %v\n", fd, err)
 						fmt.Fprintf(os.Stderr, "Will not try to replace argument %s to tshark\n", args[1])
 					} else {
-						log.Infof("Replacing argument %s with - for tshark compatibility", args[1])
 						fmt.Fprintf(os.Stderr, "Replacing argument %s with - for tshark compatibility\n", args[1])
 						args[1] = "-"
 					}
 				}
 			}
 		}
 	}
 
 	fmt.Fprintf(os.Stderr, "Starting termshark's custom live capture procedure.\n")
 	dumpcapCmd := exec.Command(dumpcapBin, args...)
-	log.Infof("Starting dumpcap command %v", dumpcapCmd)
 	fmt.Fprintf(os.Stderr, "Trying dumpcap command %v\n", dumpcapCmd)
 	dumpcapCmd.Stdin = os.Stdin
 	dumpcapCmd.Stdout = os.Stdout
 	dumpcapCmd.Stderr = os.Stderr
 	if dumpcapCmd.Run() != nil {
 		var tshark string
 		tshark, err = exec.LookPath(tsharkBin)
 		if err == nil {
-			log.Infof("Retrying with dumpcap command %v", append([]string{tshark}, args...))
 			fmt.Fprintf(os.Stderr, "Retrying with capture command %v\n", append([]string{tshark}, args...))
 			err = syscall.Exec(tshark, append([]string{tshark}, args...), os.Environ())
 		}
 	}

diff --git a/system/dumpcapext_arm64.go b/system/dumpcapext_arm64.go
@@ -8,11  8,10 @@
 package system
 
 import (
 	"fmt"
 	"os"
 	"os/exec"
 	"syscall"
-
-	log "github.com/sirupsen/logrus"
 )
 
 //======================================================================
@@ -26,15  25,16 @@ func DumpcapExt(dumpcapBin string, tsharkBin string, args ...string) error {
 	var err error
 
 	dumpcapCmd := exec.Command(dumpcapBin, args...)
-	log.Infof("Starting dumpcap command %v", dumpcapCmd)
 	fmt.Fprintf(os.Stderr, "Starting termshark's custom live capture procedure.\n")
 	fmt.Fprintf(os.Stderr, "Trying dumpcap command %v\n", dumpcapCmd)
 	dumpcapCmd.Stdin = os.Stdin
 	dumpcapCmd.Stdout = os.Stdout
 	dumpcapCmd.Stderr = os.Stderr
 	if dumpcapCmd.Run() != nil {
 		var tshark string
 		tshark, err = exec.LookPath(tsharkBin)
 		if err == nil {
-			log.Infof("Retrying with dumpcap command %v", append([]string{tshark}, args...))
 			fmt.Fprintf(os.Stderr, "Retrying with dumpcap command %v\n", append([]string{tshark}, args...))
 			err = syscall.Exec(tshark, append([]string{tshark}, args...), os.Environ())
 		}
 	}

diff --git a/system/dumpcapext_darwin.go b/system/dumpcapext_darwin.go
@@ -5,11  5,10 @@
 package system
 
 import (
 	"fmt"
 	"os"
 	"os/exec"
 	"syscall"
-
-	log "github.com/sirupsen/logrus"
 )
 
 //======================================================================
@@ -23,15  22,16 @@ func DumpcapExt(dumpcapBin string, tsharkBin string, args ...string) error {
 	var err error
 
 	dumpcapCmd := exec.Command(dumpcapBin, args...)
-	log.Infof("Starting dumpcap command %v", dumpcapCmd)
 	fmt.Fprintf(os.Stderr, "Starting termshark's custom live capture procedure.\n")
 	fmt.Fprintf(os.Stderr, "Trying dumpcap command %v\n", dumpcapCmd)
 	dumpcapCmd.Stdin = os.Stdin
 	dumpcapCmd.Stdout = os.Stdout
 	dumpcapCmd.Stderr = os.Stderr
 	if dumpcapCmd.Run() != nil {
 		var tshark string
 		tshark, err = exec.LookPath(tsharkBin)
 		if err == nil {
-			log.Infof("Retrying with dumpcap command %v", append([]string{tshark}, args...))
 			fmt.Fprintf(os.Stderr, "Retrying with dumpcap command %v\n", append([]string{tshark}, args...))
 			err = syscall.Exec(tshark, append([]string{tshark}, args...), os.Environ())
 		}
 	}

diff --git a/system/dumpcapext_windows.go b/system/dumpcapext_windows.go
@@ -5,10  5,9 @@
 package system
 
 import (
 	"fmt"
 	"os"
 	"os/exec"
-
-	log "github.com/sirupsen/logrus"
 )
 
 //======================================================================
@@ -20,7  19,8 @@ import (
 // tshark supports extcap interfaces.
 func DumpcapExt(dumpcapBin string, tsharkBin string, args ...string) error {
 	dumpcapCmd := exec.Command(dumpcapBin, args...)
-	log.Infof("Starting dumpcap command %v", dumpcapCmd)
 	fmt.Fprintf(os.Stderr, "Starting termshark's custom live capture procedure.\n")
 	fmt.Fprintf(os.Stderr, "Trying dumpcap command %v\n", dumpcapCmd)
 	dumpcapCmd.Stdin = os.Stdin
 	dumpcapCmd.Stdout = os.Stdout
 	dumpcapCmd.Stderr = os.Stderr
@@ -29,7  29,7 @@ func DumpcapExt(dumpcapBin string, tsharkBin string, args ...string) error {
 	}
 
 	tsharkCmd := exec.Command(tsharkBin, args...)
-	log.Infof("Retrying with dumpcap command %v", tsharkCmd)
 	fmt.Fprintf(os.Stderr, "Retrying with dumpcap command %v\n", tsharkCmd)
 	tsharkCmd.Stdin = os.Stdin
 	tsharkCmd.Stdout = os.Stdout
 	tsharkCmd.Stderr = os.Stderr

diff --git a/utils.go b/utils.go
@@ -224,14  224,18 @@ func TSharkPath() (string, *gowid.KeyValueError) {
 }
 
 func RunForExitCode(prog string, args []string, env []string) (int, error) {
 	return RunForStderr(prog, args, env, ioutil.Discard)
 }
 
 func RunForStderr(prog string, args []string, env []string, stderr io.Writer) (int, error) {
 	var err error
 	exitCode := -1 // default bad
 	cmd := exec.Command(prog, args...)
 	if env != nil {
 		cmd.Env = env
 	}
 	cmd.Stdout = ioutil.Discard
-	cmd.Stderr = ioutil.Discard
 	cmd.Stderr = stderr
 	err = cmd.Run()
 	if err != nil {
 		if exerr, ok := err.(*exec.ExitError); ok {