From 3f42bc8cad9964a041847d66152a91a67b88ad42 Mon Sep 17 00:00:00 2001 From: Sung Yoon Whang Date: Tue, 21 Jul 2026 20:38:52 -0700 Subject: [PATCH] fix(targethasher): preserve query inputs Clone set-like protobuf slices before sorting and filter external URL attributes on a copied rule so hashing never mutates caller-owned query results. Add Rapid properties that permute target, dependency, attribute, and value ordering while asserting stable semantic hashes and byte-for-byte input immutability. --- core/targethasher/BUILD.bazel | 3 + core/targethasher/graph.go | 23 +-- core/targethasher/graph_rapid_test.go | 230 ++++++++++++++++++++++++++ core/targethasher/sourcehasher.go | 33 ++-- 4 files changed, 263 insertions(+), 26 deletions(-) create mode 100644 core/targethasher/graph_rapid_test.go diff --git a/core/targethasher/BUILD.bazel b/core/targethasher/BUILD.bazel index d32d0fb6..ea505f89 100644 --- a/core/targethasher/BUILD.bazel +++ b/core/targethasher/BUILD.bazel @@ -19,6 +19,7 @@ go_library( go_test( name = "targethasher_test", srcs = [ + "graph_rapid_test.go", "graph_test.go", "mock_sourcehasher_test.go", "sourcehasher_test.go", @@ -34,5 +35,7 @@ go_test( "@com_github_google_go_cmp//cmp/cmpopts", "@com_github_stretchr_testify//assert", "@com_github_stretchr_testify//require", + "@net_pgregory_rapid//:rapid", + "@org_golang_google_protobuf//proto", ], ) diff --git a/core/targethasher/graph.go b/core/targethasher/graph.go index 03daffce..72c49c87 100644 --- a/core/targethasher/graph.go +++ b/core/targethasher/graph.go @@ -20,6 +20,7 @@ import ( "path/filepath" "regexp" "runtime" + "slices" "sort" "strings" @@ -157,12 +158,12 @@ var ( integrityAttr = "integrity" ) -func removeURLAttrs(t *buildpb.Target) { - if t.Rule == nil { - return +func removeURLAttrs(rule *buildpb.Rule) *buildpb.Rule { + if rule == nil { + return nil } - oldAttrs := t.GetRule().GetAttribute() + oldAttrs := rule.GetAttribute() var willCheckContent bool for _, attr := range oldAttrs { @@ -173,7 +174,7 @@ func removeURLAttrs(t *buildpb.Target) { } // no-op if sha256 is not present if !willCheckContent { - return + return rule } newAttrs := make([]*buildpb.Attribute, 0, len(oldAttrs)) @@ -182,30 +183,32 @@ func removeURLAttrs(t *buildpb.Target) { newAttrs = append(newAttrs, attr) } } - t.Rule.Attribute = newAttrs + copy := *rule + copy.Attribute = newAttrs + return © } func toTarget(t *buildpb.Target) (*Target, error) { switch *t.Type { case buildpb.Target_RULE: targetName := t.Rule.GetName() - deps := t.Rule.GetRuleInput() + deps := slices.Clone(t.Rule.GetRuleInput()) // sorting dependencies of rules, just like we do when calculating hashes for these rules. sort.Strings(deps) h := newHash() // TODO: remove this and handle external targets in the same way as internal targets if strings.HasPrefix(targetName, externalWorkspaceRulePrefix) { // if this is an external target, remove unwanted attributes from the rule, e.g. url and urls - removeURLAttrs(t) + rule := removeURLAttrs(t.Rule) // Workspace rule usually representing external repository or an HTTP file. // It is only used to hash external source files, so store it in a separate map. - HashRuleCommon(t.Rule, h) + HashRuleCommon(rule, h) return &Target{ Name: targetName, RuleType: ExternalRuleType, Deps: deps, - Rule: t.Rule, + Rule: rule, HashWithoutDeps: h.Sum(nil), External: true, }, nil diff --git a/core/targethasher/graph_rapid_test.go b/core/targethasher/graph_rapid_test.go new file mode 100644 index 00000000..67590da2 --- /dev/null +++ b/core/targethasher/graph_rapid_test.go @@ -0,0 +1,230 @@ +// Copyright (c) 2026 Uber Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package targethasher + +import ( + "fmt" + "slices" + "sort" + "strings" + "testing" + + buildpb "github.com/bazelbuild/buildtools/build_proto" + "github.com/google/go-cmp/cmp" + "google.golang.org/protobuf/proto" + "pgregory.net/rapid" +) + +func TestFromProto_queryOrderInvarianceAndInputImmutability(t *testing.T) { + t.Run("OrderInvariance", func(t *testing.T) { + rapid.Check(t, func(t *rapid.T) { + inputs, knownSourceHashes := drawPermutedQueryResults(t) + results := make([]normalizedHashResult, len(inputs)) + for i, input := range inputs { + result, err := FromProto(t.Context(), input, "/workspace", HashConfig{ + KnownSourceHashes: knownSourceHashes, + }) + if err != nil { + t.Fatalf("test defect: generated acyclic query variant %d failed: %v", i, err) + } + results[i] = normalizeHashResult(result) + } + + for i := 1; i < len(results); i++ { + if diff := cmp.Diff(results[0], results[i]); diff != "" { + t.Fatalf( + "order instability between variants 0 and %d (-variant 0 +variant %d):\n%s", + i, + i, + diff, + ) + } + } + }) + }) + + t.Run("InputImmutability", func(t *testing.T) { + rapid.Check(t, func(t *rapid.T) { + inputs, knownSourceHashes := drawPermutedQueryResults(t) + var failures []string + for i, input := range inputs { + before := proto.Clone(input).(*buildpb.QueryResult) + _, err := FromProto(t.Context(), input, "/workspace", HashConfig{ + KnownSourceHashes: knownSourceHashes, + }) + if err != nil { + t.Fatalf("test defect: generated acyclic query variant %d failed: %v", i, err) + } + if !proto.Equal(before, input) { + failures = append(failures, fmt.Sprintf( + "input mutation in variant %d (-before +after):\n%s", + i, + cmp.Diff(before.String(), input.String()), + )) + } + } + if len(failures) > 0 { + t.Fatal(strings.Join(failures, "\n")) + } + }) + }) +} + +func drawPermutedQueryResults(t *rapid.T) ([]*buildpb.QueryResult, map[string][]byte) { + query, knownSourceHashes := drawAcyclicQueryResult(t) + inputs := []*buildpb.QueryResult{ + proto.Clone(query).(*buildpb.QueryResult), + proto.Clone(query).(*buildpb.QueryResult), + proto.Clone(query).(*buildpb.QueryResult), + } + permuteQueryResult(t, inputs[1], "first") + permuteQueryResult(t, inputs[2], "second") + return inputs, knownSourceHashes +} + +func drawAcyclicQueryResult(t *rapid.T) (*buildpb.QueryResult, map[string][]byte) { + targetCount := rapid.IntRange(1, 8).Draw(t, "targetCount") + sourceCount := rapid.IntRange(1, min(targetCount, 3)).Draw(t, "sourceCount") + + query := &buildpb.QueryResult{Target: make([]*buildpb.Target, 0, targetCount)} + knownSourceHashes := make(map[string][]byte, sourceCount) + targetNames := make([]string, 0, targetCount) + for i := range sourceCount { + name := fmt.Sprintf("//pkg:source_%d.txt", i) + path := fmt.Sprintf("pkg/source_%d.txt", i) + targetNames = append(targetNames, name) + knownSourceHashes[path] = []byte(fmt.Sprintf("source-hash-%d", i)) + query.Target = append(query.Target, &buildpb.Target{ + Type: buildpb.Target_SOURCE_FILE.Enum(), + SourceFile: &buildpb.SourceFile{ + Name: StringPtr(name), + Location: StringPtr("/workspace/" + path), + }, + }) + } + + for i := sourceCount; i < targetCount; i++ { + name := fmt.Sprintf("//pkg:rule_%d", i) + var inputs []string + for dependency, dependencyName := range targetNames { + if rapid.Bool().Draw(t, fmt.Sprintf("rule-%d-dependency-%d", i, dependency)) { + inputs = append(inputs, dependencyName) + } + } + tags := rapid.SliceOfNDistinct( + rapid.SampledFrom([]string{"manual", "small", "test", "local"}), + 0, + 4, + rapid.ID[string], + ).Draw(t, fmt.Sprintf("rule-%d-tags", i)) + priorities := rapid.SliceOfNDistinct( + rapid.Int32Range(-3, 3), + 0, + 4, + rapid.ID[int32], + ).Draw(t, fmt.Sprintf("rule-%d-priorities", i)) + query.Target = append(query.Target, &buildpb.Target{ + Type: buildpb.Target_RULE.Enum(), + Rule: &buildpb.Rule{ + Name: StringPtr(name), + RuleClass: StringPtr("generated_rule"), + RuleInput: inputs, + Attribute: []*buildpb.Attribute{ + { + Name: StringPtr("description"), + Type: buildpb.Attribute_STRING.Enum(), + StringValue: StringPtr(rapid.StringMatching(`[a-z]{0,8}`).Draw(t, fmt.Sprintf("rule-%d-description", i))), + }, + { + Name: StringPtr("tags"), + Type: buildpb.Attribute_STRING_LIST.Enum(), + StringListValue: tags, + }, + { + Name: StringPtr("priorities"), + Type: buildpb.Attribute_INTEGER_LIST.Enum(), + IntListValue: priorities, + }, + }, + }, + }) + targetNames = append(targetNames, name) + } + return query, knownSourceHashes +} + +func permuteQueryResult(t *rapid.T, query *buildpb.QueryResult, variant string) { + query.Target = rapid.Permutation(query.Target).Draw(t, variant+"-targets") + for i, target := range query.Target { + if target.Rule == nil { + continue + } + target.Rule.RuleInput = rapid.Permutation(target.Rule.RuleInput).Draw(t, fmt.Sprintf("%s-rule-%d-inputs", variant, i)) + target.Rule.Attribute = rapid.Permutation(target.Rule.Attribute).Draw(t, fmt.Sprintf("%s-rule-%d-attributes", variant, i)) + for j, attribute := range target.Rule.Attribute { + attribute.StringListValue = rapid.Permutation(attribute.StringListValue).Draw(t, fmt.Sprintf("%s-rule-%d-attribute-%d-strings", variant, i, j)) + attribute.IntListValue = rapid.Permutation(attribute.IntListValue).Draw(t, fmt.Sprintf("%s-rule-%d-attribute-%d-ints", variant, i, j)) + } + } +} + +type normalizedHashResult struct { + TargetNames []string + Targets []normalizedHashTarget + Warnings []string +} + +type normalizedHashTarget struct { + Name string + Hash string + HashWithoutDeps string + RuleType string + Deps []string + Tags []string + Root bool + External bool +} + +func normalizeHashResult(result Result) normalizedHashResult { + normalized := normalizedHashResult{ + TargetNames: slices.Clone(result.TargetNames), + Targets: make([]normalizedHashTarget, 0, len(result.Targets)), + Warnings: make([]string, 0, len(result.Warnings)), + } + for _, target := range result.Targets { + deps := slices.Clone(target.Deps) + tags := slices.Clone(target.Tags) + sort.Strings(deps) + sort.Strings(tags) + normalized.Targets = append(normalized.Targets, normalizedHashTarget{ + Name: target.Name, + Hash: string(target.Hash), + HashWithoutDeps: string(target.HashWithoutDeps), + RuleType: target.RuleType, + Deps: deps, + Tags: tags, + Root: target.Root, + External: target.External, + }) + } + for name := range result.Warnings { + normalized.Warnings = append(normalized.Warnings, name) + } + sort.Slice(normalized.Targets, func(i, j int) bool { + return normalized.Targets[i].Name < normalized.Targets[j].Name + }) + sort.Strings(normalized.Warnings) + return normalized +} diff --git a/core/targethasher/sourcehasher.go b/core/targethasher/sourcehasher.go index d75e3322..aef04f9e 100644 --- a/core/targethasher/sourcehasher.go +++ b/core/targethasher/sourcehasher.go @@ -22,6 +22,7 @@ import ( "io/fs" "os" "path/filepath" + "slices" "sort" "strconv" "strings" @@ -176,7 +177,7 @@ func HashRuleCommon(r *buildpb.Rule, h hash.Hash) { // don't hash location, as it machine local paths // Attribute []*Attribute // Before hashing, sort to guarantee consistency - attributes := r.GetAttribute() + attributes := slices.Clone(r.GetAttribute()) sort.Slice(attributes, func(i, j int) bool { return attributes[i].GetName() < attributes[j].GetName() }) @@ -185,21 +186,21 @@ func HashRuleCommon(r *buildpb.Rule, h hash.Hash) { } // RuleInput []string // Before hashing, sort to guarantee consistency - ruleInputs := r.GetRuleInput() + ruleInputs := slices.Clone(r.GetRuleInput()) sort.Strings(ruleInputs) for _, ri := range ruleInputs { io.WriteString(h, ri) } // RuleOutput []string // Before hashing, sort to guarantee consistency - ruleOutputs := r.GetRuleOutput() + ruleOutputs := slices.Clone(r.GetRuleOutput()) sort.Strings(ruleOutputs) for _, ro := range ruleOutputs { io.WriteString(h, ro) } // DefaultSetting []string // Before hashing, sort to guarantee consistency - defaultSettings := r.GetDefaultSetting() + defaultSettings := slices.Clone(r.GetDefaultSetting()) sort.Strings(defaultSettings) for _, d := range defaultSettings { io.WriteString(h, d) @@ -247,7 +248,7 @@ func hashAttributes(h hash.Hash, a *buildpb.Attribute) { } // StringListValue []string // Before hashing, sort to guarantee consistency - stringListValue := a.GetStringListValue() + stringListValue := slices.Clone(a.GetStringListValue()) sort.Strings(stringListValue) for _, s := range stringListValue { io.WriteString(h, s) @@ -255,7 +256,7 @@ func hashAttributes(h hash.Hash, a *buildpb.Attribute) { // License *License // StringDictValue []*StringDictEntry // Before hashing, sort to guarantee consistency - stringDictValue := a.GetStringDictValue() + stringDictValue := slices.Clone(a.GetStringDictValue()) sort.Slice(stringDictValue, func(i, j int) bool { return stringDictValue[i].GetKey() < stringDictValue[j].GetKey() }) @@ -265,7 +266,7 @@ func hashAttributes(h hash.Hash, a *buildpb.Attribute) { } // FilesetListValue []*FilesetEntry // Before hashing, sort to guarantee consistency - filesetListValue := a.GetFilesetListValue() + filesetListValue := slices.Clone(a.GetFilesetListValue()) sort.Slice(filesetListValue, func(i, j int) bool { return filesetListValue[i].GetSource() < filesetListValue[j].GetSource() }) @@ -280,14 +281,14 @@ func hashAttributes(h hash.Hash, a *buildpb.Attribute) { } // File []string // Before hashing, sort to guarantee consistency - files := f.GetFile() + files := slices.Clone(f.GetFile()) sort.Strings(files) for _, file := range files { io.WriteString(h, file) } // Exclude []string // Before hashing, sort to guarantee consistency - excludedFiles := f.GetExclude() + excludedFiles := slices.Clone(f.GetExclude()) sort.Strings(excludedFiles) for _, file := range excludedFiles { io.WriteString(h, file) @@ -299,14 +300,14 @@ func hashAttributes(h hash.Hash, a *buildpb.Attribute) { } // LabelListDictValue []*LabelListDictEntry // Before hashing, sort to guarantee consistency - labelListDictValue := a.GetLabelListDictValue() + labelListDictValue := slices.Clone(a.GetLabelListDictValue()) sort.Slice(labelListDictValue, func(i, j int) bool { return labelListDictValue[i].GetKey() < labelListDictValue[j].GetKey() }) for _, ll := range labelListDictValue { io.WriteString(h, ll.GetKey()) // Before hashing, sort to guarantee consistency - llv := ll.GetValue() + llv := slices.Clone(ll.GetValue()) sort.Strings(llv) for _, v := range llv { io.WriteString(h, v) @@ -314,14 +315,14 @@ func hashAttributes(h hash.Hash, a *buildpb.Attribute) { } // StringListDictValue []*StringListDictEntry // Before hashing, sort to guarantee consistency - stringListDictValue := a.GetStringListDictValue() + stringListDictValue := slices.Clone(a.GetStringListDictValue()) sort.Slice(stringListDictValue, func(i, j int) bool { return stringListDictValue[i].GetKey() < stringListDictValue[j].GetKey() }) for _, sl := range stringListDictValue { io.WriteString(h, sl.GetKey()) // Before hashing, sort to guarantee consistency - slv := sl.GetValue() + slv := slices.Clone(sl.GetValue()) sort.Strings(slv) for _, v := range slv { io.WriteString(h, v) @@ -329,7 +330,7 @@ func hashAttributes(h hash.Hash, a *buildpb.Attribute) { } // IntListValue []int32 // Before hashing, sort to guarantee consistency - intListValue := a.GetIntListValue() + intListValue := slices.Clone(a.GetIntListValue()) sort.Slice(intListValue, func(i, j int) bool { return intListValue[i] < intListValue[j] }) @@ -338,7 +339,7 @@ func hashAttributes(h hash.Hash, a *buildpb.Attribute) { } // LabelDictUnaryValue []*LabelDictUnaryEntry // Before hashing, sort to guarantee consistency - labelDictUnaryValue := a.GetLabelDictUnaryValue() + labelDictUnaryValue := slices.Clone(a.GetLabelDictUnaryValue()) sort.Slice(labelDictUnaryValue, func(i, j int) bool { return labelDictUnaryValue[i].GetKey() < labelDictUnaryValue[j].GetKey() }) @@ -348,7 +349,7 @@ func hashAttributes(h hash.Hash, a *buildpb.Attribute) { } // LabelKeyedStringDictValue []*LabelKeyedStringDictEntry // Before hashing, sort to guarantee consistency - labelKeyedStringDictValue := a.GetLabelKeyedStringDictValue() + labelKeyedStringDictValue := slices.Clone(a.GetLabelKeyedStringDictValue()) sort.Slice(labelKeyedStringDictValue, func(i, j int) bool { return labelKeyedStringDictValue[i].GetKey() < labelKeyedStringDictValue[j].GetKey() })