From cc1b5cda7073c68491e0ad9a5b4d08216ae07806 Mon Sep 17 00:00:00 2001
From: Keegan Carruthers-Smith <keegan.csmith@gmail.com>
Date: Thu, 5 Oct 2023 08:10:20 +0200
Subject: [PATCH] ctags: allow binary to be anything with validation (#652)

Previously we enforced the binary was called universal-ctags. However,
we let users override the name of the binary, so if they override it we
should use it. To prevent footguns, we now validate the binary was built
with the interactive feature.

In the case of scip-ctags we use the old validation of just checking the
name.

Additionally we fix a bug that was introduced where if symbols are
optional we continue if parsing fails.

Test Plan: indexed with and without universal-ctags. Ctags on my mbp
points to something from xcode which wouldn't work

  $ CTAGS_COMMAND=ctags go run ./cmd/zoekt-git-index -require_ctags .
  2023/10/04 17:08:12 indexGitRepo(/Users/keegan/src/github.com/sourcegraph/zoekt, delta=false): build.NewBuilder: ctags.NewParserMap: ctags binary is not universal-ctags or is not compiled with +interactive feature: bin=ctags
  exit status 1

  $ CTAGS_COMMAND=universal-ctags go run ./cmd/zoekt-git-index -require_ctags .
  2023/10/04 17:08:29 finished github.com%2Fsourcegraph%2Fzoekt_v16.00000.zoekt: 8657338 index bytes (overhead 2.9)

  $ CTAGS_COMMAND=ctags go run ./cmd/zoekt-git-index .
  2023/10/04 17:08:40 finished github.com%2Fsourcegraph%2Fzoekt_v16.00000.zoekt: 8538246 index bytes (overhead 2.9)
---
 build/ctags.go      |  4 ++++
 ctags/json.go       | 46 ++++++++++++++++++++++++++++++++++-----------
 ctags/json_test.go  |  2 +-
 ctags/parser_map.go |  2 +-
 4 files changed, 41 insertions(+), 13 deletions(-)

diff --git a/build/ctags.go b/build/ctags.go
index 2763c231..cd9a4e17 100644
--- a/build/ctags.go
+++ b/build/ctags.go
@@ -53,6 +53,10 @@ func ctagsAddSymbolsParserMap(todo []*zoekt.Document, languageMap ctags.Language
 		parser := parserMap[parserKind]
 		if parser == nil {
 			parser = parserMap[ctags.UniversalCTags]
+			if parser == nil {
+				// this happens if CTagsMustSucceed is not true and we didn't find universal-ctags
+				continue
+			}
 		}
 
 		es, err := parser.Parse(doc.Name, doc.Content)
diff --git a/ctags/json.go b/ctags/json.go
index c5f802ee..bdff60d6 100644
--- a/ctags/json.go
+++ b/ctags/json.go
@@ -15,9 +15,11 @@
 package ctags
 
 import (
+	"bytes"
 	"fmt"
 	"log"
 	"os"
+	"os/exec"
 	"strings"
 	"sync"
 	"time"
@@ -119,19 +121,41 @@ func (lp *lockedParser) close() {
 
 // NewParser creates a parser that is implemented by the given
 // universal-ctags binary. The parser is safe for concurrent use.
-func NewParser(bin string) (Parser, error) {
-	if strings.Contains(bin, "universal-ctags") || strings.Contains(bin, "scip-ctags") {
-		opts := goctags.Options{
-			Bin: bin,
+func NewParser(parserType CTagsParserType, bin string) (Parser, error) {
+	if err := checkBinary(parserType, bin); err != nil {
+		return nil, err
+	}
+
+	opts := goctags.Options{
+		Bin: bin,
+	}
+	if debug {
+		opts.Info = log.New(os.Stderr, "CTAGS INF: ", log.LstdFlags)
+		opts.Debug = log.New(os.Stderr, "CTAGS DBG: ", log.LstdFlags)
+	}
+	return &lockedParser{
+		opts: opts,
+	}, nil
+}
+
+// checkBinary does checks on bin to ensure we can correctly use the binary
+// for symbols. It is more user friendly to fail early in this case.
+func checkBinary(typ CTagsParserType, bin string) error {
+	switch typ {
+	case UniversalCTags:
+		helpOutput, err := exec.Command(bin, "--help").CombinedOutput()
+		if err != nil {
+			return fmt.Errorf("failed to check if %s is universal-ctags: %w\n--help output:\n%s", bin, err, string(helpOutput))
 		}
-		if debug {
-			opts.Info = log.New(os.Stderr, "CTAGS INF: ", log.LstdFlags)
-			opts.Debug = log.New(os.Stderr, "CTAGS DBG: ", log.LstdFlags)
+		if !bytes.Contains(helpOutput, []byte("+interactive")) {
+			return fmt.Errorf("ctags binary is not universal-ctags or is not compiled with +interactive feature: bin=%s", bin)
+		}
+
+	case ScipCTags:
+		if !strings.Contains(bin, "scip-ctags") {
+			return fmt.Errorf("only supports scip-ctags, not %s", bin)
 		}
-		return &lockedParser{
-			opts: opts,
-		}, nil
 	}
 
-	return nil, fmt.Errorf("only supports universal-ctags, not %s", bin)
+	return nil
 }
diff --git a/ctags/json_test.go b/ctags/json_test.go
index 67b4ff36..5a6304f4 100644
--- a/ctags/json_test.go
+++ b/ctags/json_test.go
@@ -27,7 +27,7 @@ func TestJSON(t *testing.T) {
 		t.Skip(err)
 	}
 
-	p, err := NewParser("universal-ctags")
+	p, err := NewParser(UniversalCTags, "universal-ctags")
 	if err != nil {
 		t.Fatal("newProcess", err)
 	}
diff --git a/ctags/parser_map.go b/ctags/parser_map.go
index 8f8d040a..b7756648 100644
--- a/ctags/parser_map.go
+++ b/ctags/parser_map.go
@@ -66,7 +66,7 @@ func NewParserMap(bins ParserBinMap, cTagsMustSucceed bool) (ParserMap, error) {
 	for _, parserType := range []CTagsParserType{UniversalCTags, ScipCTags} {
 		bin := bins[parserType]
 		if bin != "" {
-			parser, err := NewParser(bin)
+			parser, err := NewParser(parserType, bin)
 
 			if err != nil && cTagsMustSucceed {
 				return nil, fmt.Errorf("ctags.NewParserMap: %v", err)