strip struct tags when hashing structs for type identity

This was a long standing TODO, and a user finally ran into it.
The fix isn't terribly straightforward, but I'm pretty happy with it.

Add a test case where the same struct field is identical
with no tag, with the "tagged1" json tag, and again with "tagged2".
While here, we add a test case for a regular named field too.

Fixes #801.

hash.go

@@ -224,6 +224,9 @@ func entryOffKey() uint32 {
 }
 
 func hashWithPackage(pkg *listedPackage, name string) string {
+	// If the user provided us with an obfuscation seed,
+	// we use that with the package import path directly..
+	// Otherwise, we use GarbleActionID as a fallback salt.
 	if !flagSeed.present() {
 		return hashWithCustomSalt(pkg.GarbleActionID[:], name)
 	}
@@ -233,16 +236,72 @@ func hashWithPackage(pkg *listedPackage, name string) string {
 	return hashWithCustomSalt([]byte(pkg.ImportPath+"|"), name)
 }
 
-func hashWithStruct(strct *types.Struct, fieldName string) string {
+// stripStructTags takes the bytes produced by [types.WriteType]
-	// TODO: We should probably strip field tags here.
+// and removes any struct tags in-place, such as rewriting
-	// Do we need to do anything else to make a
+//
-	// struct type "canonical"?
+//	struct{Foo int; Bar string "json:\"bar\""}
-	fieldsSalt := []byte(strct.String())
+//
+// into
+//
+//	struct{Foo int; Bar string}
+//
+// Note that, unlike most Go source, WriteType uses double quotes for tags.
+//
+// Reusing WriteType does require a second pass over its output here,
+// which we could save by implementing our own modified version of WriteType.
+// However, that would be a significant amount of code to maintain.
+func stripStructTags(p []byte) []byte {
+	i := 0
+	for i < len(p) {
+		b := p[i]
+		start := i - 1 // a struct tag is preceded by a space
+		i++
+		if b != '"' {
+			continue
+		}
+		// Find the closing double quote, skipping over escaped characters.
+		// Note that we should probably iterate over runes and not bytes,
+		// but this byte implementation is probably good enough in practice.
+		for {
+			b = p[i]
+			i++
+			if b == '\\' {
+				i++
+			} else if b == '"' {
+				break
+			}
+		}
+		end := i
+		// Remove the bytes between start and end,
+		// and reset i to start, since we just shortened p.
+		p = append(p[:start], p[end:]...)
+		i = start
+	}
+	return p
+}
+
+var typeIdentityBuf bytes.Buffer
+
+// hashWithStruct is separate from hashWithPackage since Go
+// allows converting between struct types across packages.
+// Hashing struct field names differently between packages would break that.
+//
+// We hash field names with the identity struct type as a salt
+// so that the same field name used in different struct types is obfuscated differently.
+// Note that "identity" means omitting struct tags since conversions ignore them.
+func hashWithStruct(strct *types.Struct, field *types.Var) string {
+	typeIdentityBuf.Reset()
+	types.WriteType(&typeIdentityBuf, strct, nil)
+	salt := stripStructTags(typeIdentityBuf.Bytes())
+
+	// If the user provided us with an obfuscation seed,
+	// we only use the identity struct type as a salt.
+	// Otherwise, we add garble's own inputs to the salt as a fallback.
 	if !flagSeed.present() {
-		withGarbleHash := addGarbleToHash(fieldsSalt)
+		withGarbleHash := addGarbleToHash(salt)
-		fieldsSalt = withGarbleHash[:]
+		salt = withGarbleHash[:]
 	}
-	return hashWithCustomSalt(fieldsSalt, fieldName)
+	return hashWithCustomSalt(salt, field.Name())
 }
 
 // minHashLength and maxHashLength define the range for the number of base64

main.go

@@ -2001,7 +2001,7 @@ func (tf *transformer) transformGoFile(file *ast.File) *ast.File {
 			if strct == nil {
 				panic("could not find struct for field " + name)
 			}
-			node.Name = hashWithStruct(strct, name)
+			node.Name = hashWithStruct(strct, obj)
 			if flagDebug { // TODO(mvdan): remove once https://go.dev/issue/53465 if fixed
 				log.Printf("%s %q hashed with struct fields to %q", debugName, name, node.Name)
 			}

reverse.go

@@ -106,7 +106,7 @@ One can reverse a captured panic stack trace as follows:
 						if strct == nil {
 							panic("could not find struct for field " + name.Name)
 						}
-						replaces = append(replaces, hashWithStruct(strct, name.Name), name.Name)
+						replaces = append(replaces, hashWithStruct(strct, obj), name.Name)
 					}
 
 				case *ast.CallExpr:

testdata/script/implement.txtar

@@ -48,7 +48,9 @@ type StructUnnamed = struct {
 	Bar struct {
 		Nested *[]string
 	}
+	Named lib3.Named
 	lib3.StructEmbed
+	Tagged string // no field tag
 }
 
 var _ = lib1.Struct1(lib2.Struct2{})
@@ -69,7 +71,9 @@ type Struct1 struct {
 	Bar struct {
 		Nested *[]string
 	}
+	Named lib3.Named
 	lib3.StructEmbed
+	Tagged string `json:"tagged1"`
 }
 
 -- lib2/lib2.go --
@@ -82,11 +86,15 @@ type Struct2 struct {
 	Bar struct {
 		Nested *[]string
 	}
+	Named lib3.Named
 	lib3.StructEmbed
+	Tagged string `json:"tagged2"`
 }
 -- lib3/lib3.go --
 package lib3
 
+type Named int
+
 type StructEmbed struct {
 	Baz any
 }