-
Notifications
You must be signed in to change notification settings - Fork 621
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: sanitize pprof references #3218
Conversation
f40f3d7
to
642a49f
Compare
ca29a82
to
412c359
Compare
@@ -33,51 +35,6 @@ func Test_Merge_Self(t *testing.T) { | |||
testhelper.EqualProto(t, p.Profile, m.Profile()) | |||
} | |||
|
|||
func Test_Merge_ZeroReferences(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to remove the test as it's no longer valid: invalid elements are discarded at normalization/merge
// ensureHasMapping ensures all locations have at least a mapping. | ||
func (p *Profile) ensureHasMapping() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now we're doing this at reference sanitisation
pkg/pprof/merge.go
Outdated
@@ -36,7 +36,7 @@ func (m *ProfileMerge) merge(p *profilev1.Profile, clone bool) error { | |||
if p == nil || len(p.StringTable) < 2 { | |||
return nil | |||
} | |||
ConvertIDsToIndices(p) | |||
sanitizeReferences(p) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only reason we do it here is that Merge
might be called without Normalize
. It is a fairly expensive operation, but it's better than a failure. Ideally, normalization and sanity checks should be performed at parsing
Co-authored-by: Christian Simon <simon@swine.de>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally LGTM.
A few things that a simple fuzzing session #3222 uncovered (not originating from this PR):
--- a/pkg/pprof/merge.go
+++ b/pkg/pprof/merge.go
@@ -196,6 +196,9 @@ func combineHeaders(a, b *profilev1.Profile) error {
// returns nil if the profiles are compatible; otherwise an error with
// details on the incompatibility.
func compatible(a, b *profilev1.Profile) error {
+ if a == nil || b == nil {
+ return fmt.Errorf("a nil profile was specified")
+ }
if !equalValueType(a.PeriodType, b.PeriodType) {
return fmt.Errorf("incompatible period types %v and %v", a.PeriodType, b.PeriodType)
}
@@ -213,6 +216,12 @@ func compatible(a, b *profilev1.Profile) error {
// equalValueType returns true if the two value types are semantically
// equal. It ignores the internal fields used during encode/decode.
func equalValueType(st1, st2 *profilev1.ValueType) bool {
+ if st1 == nil && st2 == nil {
+ return true
+ }
+ if st1 == nil || st2 == nil {
+ return false
+ }
return st1.Type == st2.Type && st1.Unit == st2.Unit
}
@@ -242,11 +251,13 @@ func RewriteStrings(p *profilev1.Profile, n []uint32) {
}
p.DropFrames = int64(n[p.DropFrames])
p.KeepFrames = int64(n[p.KeepFrames])
- if p.PeriodType.Type != 0 {
- p.PeriodType.Type = int64(n[p.PeriodType.Type])
- }
- if p.PeriodType.Unit != 0 {
- p.PeriodType.Unit = int64(n[p.PeriodType.Unit])
+ if p.PeriodType != nil {
+ if p.PeriodType.Type != 0 {
+ p.PeriodType.Type = int64(n[p.PeriodType.Type])
+ }
+ if p.PeriodType.Unit != 0 {
+ p.PeriodType.Unit = int64(n[p.PeriodType.Unit])
+ }
}
for i, x := range p.Comment {
p.Comment[i] = int64(n[x])
I see that we actually do not check if an element is nil (e.g. nil sample) at all – to be added in this PR |
Resolves #3203