improve "reverse" even further (#289)

Fix up a few TODOs, and simplify the way we handle comments.

We now add whitespace around inline /*line*/ directives, to ensure we
don't break programs. A test case is added too.

We now add line directives to call sites, not function declarations,
since those are what actually shows up in stack traces.
It's unclear if we care about any other lines inside functions at all.
This also fixes reversing with -literals, since that feature adds a
significant amount of code which shuffles line numbers around.

Finally, we extend the tests with types, methods, and anonymous
functions, and we make all of them work well.

Updates #5.

main.go

@@ -286,7 +286,8 @@ func mainErr(args []string) error {
 	// possibly differentiate between the two versions of a package.
 	// The same happens with "foo/bar_test", whose ImportPath is actually
 	// "foo/bar_test [foo/bar.test]".
-	// We'll likely file this as an upstream bug to fix in Go 1.17.
+	//
+	// TODO(mvdan): remove once https://github.com/golang/go/issues/44963 is fixed
 	//
 	// Until then, here's our workaround: since this edge case only happens
 	// for the compiler, check if any "_test.go" files are being compiled.
@@ -582,7 +583,7 @@ func transformCompile(args []string) ([]string, error) {
 	// extra calls to 'go list'.
 	// Since this is a rare edge case and only occurs for a few std
 	// packages, do the extra 'go list' calls for now.
-	// TODO(mvdan): report this upstream and investigate further.
+	// TODO(mvdan): remove once https://github.com/golang/go/issues/44630 is fixed
 	if curPkg.Standard && len(curPkg.ImportMap) > 0 {
 		origImporter = importer.Default()
 	}

position.go

@@ -63,21 +63,20 @@ func printFile(file1 *ast.File) ([]byte, error) {
 	addComment := func(offset int, text string) {
 		toAdd = append(toAdd, commentToAdd{offset, text})
 	}
+
+	// Make sure the entire file gets a zero filename by default,
+	// in case we miss any positions below.
 	addComment(0, "/*line :1*/")
-	for _, group := range file2.Comments {
-		for _, comment := range group.List {
-			if isDirective(comment.Text) {
-				// TODO(mvdan): merge with the zeroing below
-				pos := fset.Position(comment.Pos())
-				addComment(pos.Offset, comment.Text)
-			}
-		}
-	}
-	// Remove all existing comments by making them whitespace.
+
+	// Remove any comments by making them whitespace.
+	// Keep directives, as they affect the build.
 	// This is superior to removing the comments before printing,
 	// because then the final source would have different line numbers.
 	for _, group := range file2.Comments {
 		for _, comment := range group.List {
+			if isDirective(comment.Text) {
+				continue
+			}
 			start := fset.Position(comment.Pos()).Offset
 			end := fset.Position(comment.End()).Offset
 			for i := start; i < end; i++ {
@@ -86,21 +85,36 @@ func printFile(file1 *ast.File) ([]byte, error) {
 		}
 	}
 
-	for i, decl := range file2.Decls {
+	var origCallExprs []*ast.CallExpr
+	ast.Inspect(file1, func(node ast.Node) bool {
+		if node, ok := node.(*ast.CallExpr); ok {
+			origCallExprs = append(origCallExprs, node)
+		}
+		return true
+	})
+
+	i := 0
+	ast.Inspect(file2, func(node ast.Node) bool {
+		node, ok := node.(*ast.CallExpr)
+		if !ok {
+			return true
+		}
+		origNode := origCallExprs[i]
+		i++
 		newName := ""
 		if !opts.Tiny {
-			decl := file1.Decls[i]
-			origPos := fmt.Sprintf("%s:%d", filename, fset.Position(decl.Pos()).Offset)
+			origPos := fmt.Sprintf("%s:%d", filename, fset.Position(origNode.Pos()).Offset)
 			newName = hashWith(curPkg.GarbleActionID, origPos) + ".go"
 			// log.Printf("%q hashed with %x to %q", origPos, curPkg.GarbleActionID, newName)
 		}
 		newPos := fmt.Sprintf("%s:1", newName)
-		pos := fset.Position(decl.Pos())
+		pos := fset.Position(node.Pos())
 
-		// We use the /*text*/ form, since we can use multiple of them
+		// We use the "/*text*/" form, since we can use multiple of them
 		// on a single line, and they don't require extra newlines.
 		addComment(pos.Offset, "/*line "+newPos+"*/")
-	}
+		return true
+	})
 
 	// We add comments in order.
 	sort.Slice(toAdd, func(i, j int) bool {
@@ -111,12 +125,19 @@ func printFile(file1 *ast.File) ([]byte, error) {
 	var buf2 bytes.Buffer
 	for _, comment := range toAdd {
 		buf2.Write(src[copied:comment.offset])
+		copied = comment.offset
+
+		// Make sure there is whitespace at either side of a comment.
+		// Otherwise, we could change the syntax of the program.
+		// Inserting "/*text*/" in "a/b" // must be "a/ /*text*/ b",
+		// as "a//*text*/b" is tokenized as a "//" comment.
+		buf2.WriteByte(' ')
 		buf2.WriteString(comment.text)
 		if strings.HasPrefix(comment.text, "//") {
 			buf2.WriteByte('\n')
+		} else {
+			buf2.WriteByte(' ')
 		}
-
-		copied = comment.offset
 	}
 	buf2.Write(src[copied:])
 	return buf2.Bytes(), nil

reverse.go

@@ -62,30 +62,34 @@ func commandReverse(args []string) error {
 			if err != nil {
 				return err
 			}
-			for _, decl := range file.Decls {
+			ast.Inspect(file, func(node ast.Node) bool {
+				switch node := node.(type) {
+
+				// Replace names.
+				// TODO: do var names ever show up in output?
+				case *ast.FuncDecl:
+					addReplace(node.Name.Name)
+				case *ast.TypeSpec:
+					addReplace(node.Name.Name)
+
+				case *ast.CallExpr:
+					// continues below
+				default:
+					return true
+				}
+
 				// Reverse position information.
-				pos := fset.Position(decl.Pos())
+				pos := fset.Position(node.Pos())
 				origPos := fmt.Sprintf("%s:%d", goFile, pos.Offset)
-				newFilename := hashWith(lpkg.GarbleActionID, origPos) + ".go"
+				newPos := hashWith(lpkg.GarbleActionID, origPos) + ".go:1"
 
-				// For each line number within this function,
-				// replace the obfuscated position with its original form.
-				// Remember that the "/*line" directive starts at 1.
-				numLines := fset.Position(decl.End()).Line - pos.Line
-				for i := 0; i < numLines; i++ {
-					replaces = append(replaces,
-						fmt.Sprintf("%s:%d", newFilename, 1+i),
-						fmt.Sprintf("%s/%s:%d", lpkg.ImportPath, goFile, pos.Line+i),
-					)
-				}
+				replaces = append(replaces,
+					newPos,
+					fmt.Sprintf("%s/%s:%d", lpkg.ImportPath, goFile, pos.Line),
+				)
 
-				// Reverse the name itself.
-				// TODO: Probably do type names too. What else?
-				switch decl := decl.(type) {
-				case *ast.FuncDecl:
-					addReplace(decl.Name.Name)
-				}
-			}
+				return true
+			})
 		}
 	}
 	repl := strings.NewReplacer(replaces...)

testdata/scripts/literals.txt

@@ -46,8 +46,9 @@ grep '^\s+\w+ := \[\.{3}\](byte|uint16|uint32|uint64)\{[0-9\s,]+\}$' debug1/test
 # Split obfuscator. Detect decryptKey ^= i * counter
 grep '^\s+\w+ \^= \w+ \* \w+$' debug1/test/main/extra_literals.go
 
-# XorShuffle obfuscator. Detect data = append(data, x (^|-|+) y...)
-grep '^\s+\w+ = append\(\w+,(\s+\w+\[\d+\][\^\-+]\w+\[\d+\],?)+\)$' debug1/test/main/extra_literals.go
+# XorShuffle obfuscator. Detect data = append(data, x (^|-|+) y...).
+# Note that the line obfuscator adds an inline comment before the call.
+grep '^\s+\w+ = .*\bappend\(\w+,(\s+\w+\[\d+\][\^\-+]\w+\[\d+\],?)+\)$' debug1/test/main/extra_literals.go
 
 # XorSeed obfuscator. Detect type decFunc func(byte) decFunc
 grep '^\s+type \w+ func\(byte\) \w+$' debug1/test/main/extra_literals.go

testdata/scripts/position.txt

@@ -2,7 +2,9 @@ env GOPRIVATE=test/main
 
 garble build
 exec ./main
-! stdout 'main.go|other_file_name|is sorted'
+! stdout 'main\.go|other_file_name|is sorted'
+
+! binsubstr main$exe 'main.go' 'other_file_name'
 
 [short] stop # no need to verify this with -short
 
@@ -55,6 +57,12 @@ func main() {
 	if sort.IsSorted(sort.StringSlice(varPositions)) {
 		fmt.Println("varPositions is sorted")
 	}
+
+	// Adding "/*text*/" comments here is tricky,
+	// as we don't want to turn "a/b" into "a//*text*/b".
+	// We need "a/ /*text*/b" to preserve the syntax.
+	// The nested expression is needed to prevent spaces.
+	fmt.Printf("%v\n", 10*float64(3.0)/float64(4.0))
 }
 -- other_file_name.go --
 package main

testdata/scripts/reverse.txt

@@ -19,13 +19,25 @@ stdin main.stderr
 garble reverse
 cmp stdout reverse.stdout
 
-# TODO: test with -literals too, as it modifies the source.
-
 # Ensure that the reversed output matches the non-garbled output.
 go build -trimpath
 exec ./main
 cmp stderr reverse.stdout
 
+# Ensure that we can still reverse with -literals.
+garble -literals build
+exec ./main
+cp stderr main-literals.stderr
+
+stdin main-literals.stderr
+garble -literals reverse
+cmp stdout reverse.stdout
+
+# Reversing a -literals output without the flag should fail.
+stdin main-literals.stderr
+garble reverse
+cmp stdout main-literals.stderr
+
 -- go.mod --
 module test/main
 
@@ -44,9 +56,13 @@ func main() {
 }
 
 func unexportedMainFunc() {
-	if err := lib.ExportedLibFunc(os.Stderr); err != nil {
-		panic(err)
+	anonFunc := func() {
+		lt := lib.ExportedLibType{}
+		if err := lt.ExportedLibMethod(os.Stderr); err != nil {
+			panic(err)
+		}
 	}
+	anonFunc()
 }
 -- lib/lib.go --
 package lib
@@ -57,7 +73,13 @@ import (
 	"runtime/debug"
 )
 
-func ExportedLibFunc(w io.Writer) error {
+type ExportedLibType struct{}
+
+func (*ExportedLibType) ExportedLibMethod(w io.Writer) error {
+	return printStackTrace(w)
+}
+
+func printStackTrace(w io.Writer) error {
 	// Panic outputs include "0xNN" pointers and offsets which change
 	// between platforms.
 	// Strip them out here, to have portable static stdout files.
@@ -74,9 +96,13 @@ func ExportedLibFunc(w io.Writer) error {
 goroutine 1 [running]:
 runtime/debug.Stack(0x??, 0x??, 0x??)
 	runtime/debug/stack.go:24 +0x??
-test/main/lib.ExportedLibFunc(0x??, 0x??, 0x??, 0x??)
-	test/main/lib/lib.go:17 +0x??
-main.unexportedMainFunc(...)
-	test/main/main.go:14
+test/main/lib.printStackTrace(0x??, 0x??, 0x??, 0x??)
+	test/main/lib/lib.go:23 +0x??
+test/main/lib.(*ExportedLibType).ExportedLibMethod(...)
+	test/main/lib/lib.go:12
+main.unexportedMainFunc.func1()
+	test/main/main.go:16 +0x??
+main.unexportedMainFunc()
+	test/main/main.go:20 +0x??
 main.main()
 	test/main/main.go:10 +0x??