From acb11adf2e10f0dd14d4878ccd03b12105a99580 Mon Sep 17 00:00:00 2001
From: William Findlay <will@isovalent.com>
Date: Wed, 1 Nov 2023 15:14:29 -0400
Subject: [PATCH] server: make a copy of exit messages when filtering fields

[upstream commit 6b34552331b4da87ad7c0739ab60c7189fec20a5]

We encountered another segfault similar to the one fixed in #1432. This time, it's related
to the pid field in exit events. Fix this by making a copy of the exit event before
filtering. Also optimize copying logic to only perform the copy when absolutely necessary.

Signed-off-by: William Findlay <will@isovalent.com>
---
 pkg/filters/fields.go | 51 ++++++++++++++++++++++++++++++++++---------
 pkg/server/server.go  | 13 +++++------
 2 files changed, 47 insertions(+), 17 deletions(-)

diff --git a/pkg/filters/fields.go b/pkg/filters/fields.go
index c9f6fd4b003..06cd1eb08e0 100644
--- a/pkg/filters/fields.go
+++ b/pkg/filters/fields.go
@@ -81,15 +81,48 @@ type FieldFilter struct {
 	fields         fmutils.NestedMask
 	action         tetragon.FieldFilterAction
 	invertEventSet bool
+	needsCopy      map[tetragon.EventType]struct{}
 }
 
 // NewFieldFilter constructs a new FieldFilter from a set of fields.
 func NewFieldFilter(eventSet []tetragon.EventType, fields []string, action tetragon.FieldFilterAction, invertEventSet bool) *FieldFilter {
+	// We only need to copy exec and exit events when they are explicitly filtering out
+	// the PID. This is because we need the PID to not be nil when accessing the event
+	// later on from the eventcache. See additional comments below for details.
+	var maybeNeedsCopy bool
+	if action == tetragon.FieldFilterAction_EXCLUDE {
+		for _, field := range fields {
+			if strings.HasPrefix(field, "process") {
+				maybeNeedsCopy = true
+				break
+			}
+		}
+	} else if action == tetragon.FieldFilterAction_INCLUDE {
+		// For inclusion, it's the opposite situation from the above. If the process.pid
+		// field is NOT present, it will be trimmed. So assume we need a copy unless we
+		// see process.pid.
+		maybeNeedsCopy = true
+		for _, field := range fields {
+			if field == "process.pid" {
+				maybeNeedsCopy = false
+				break
+			}
+		}
+	}
+
+	needsCopy := make(map[tetragon.EventType]struct{})
+	if maybeNeedsCopy {
+		for _, t := range eventSet {
+			needsCopy[t] = struct{}{}
+		}
+	}
+
 	return &FieldFilter{
 		eventSet:       eventSet,
 		fields:         fmutils.NestedMaskFromPaths(fields),
 		action:         action,
 		invertEventSet: invertEventSet,
+		needsCopy:      needsCopy,
 	}
 }
 
@@ -105,12 +138,10 @@ func NewExcludeFieldFilter(eventSet []tetragon.EventType, fields []string, inver
 
 // FieldFilterFromProto constructs a new FieldFilter from a Tetragon API field filter.
 func FieldFilterFromProto(filter *tetragon.FieldFilter) *FieldFilter {
-	var fields fmutils.NestedMask
+	var fields []string
 
 	if filter.Fields != nil {
-		fields = fmutils.NestedMaskFromPaths(filter.Fields.Paths)
-	} else {
-		fields = make(fmutils.NestedMask)
+		fields = filter.Fields.Paths
 	}
 
 	invert := false
@@ -118,12 +149,7 @@ func FieldFilterFromProto(filter *tetragon.FieldFilter) *FieldFilter {
 		invert = filter.InvertEventSet.Value
 	}
 
-	return &FieldFilter{
-		eventSet:       filter.EventSet,
-		fields:         fields,
-		action:         filter.Action,
-		invertEventSet: invert,
-	}
+	return NewFieldFilter(filter.EventSet, fields, filter.Action, invert)
 }
 
 // FieldFiltersFromGetEventsRequest returns a list of EventFieldFilter for
@@ -141,6 +167,11 @@ func FieldFiltersFromGetEventsRequest(request *tetragon.GetEventsRequest) []*Fie
 	return filters
 }
 
+func (f *FieldFilter) NeedsCopy(ev *tetragon.GetEventsResponse) bool {
+	_, ok := f.needsCopy[ev.EventType()]
+	return ok
+}
+
 // Filter filters the fields in the GetEventsResponse, keeping fields specified in the
 // inclusion filter and discarding fields specified in the exclusion filter. Exclusion
 // takes precedence over inclusion and an empty filter set will keep all remaining fields.
diff --git a/pkg/server/server.go b/pkg/server/server.go
index 3df14b94e81..0fd1d0120e1 100644
--- a/pkg/server/server.go
+++ b/pkg/server/server.go
@@ -164,14 +164,13 @@ func (s *Server) GetEventsWG(request *tetragon.GetEventsRequest, server tetragon
 			// Filter the GetEventsResponse fields
 			filters := filters.FieldFiltersFromGetEventsRequest(request)
 			filterEvent := event
-			if len(filters) > 0 && filterEvent.GetProcessExec() != nil { // this is an exec event and we have fieldFilters
-				// We need a copy of the exec event as modifing the original message
-				// can cause issues in the process cache (we keep a copy of that message there).
-				filterEvent = proto.Clone(event).(*tetragon.GetEventsResponse)
-			}
+
 			for _, filter := range filters {
-				// we need not to change res
-				// maybe only for exec events
+				// We need a copy of the exec or exit event as modifing the original message
+				// can cause issues in the process cache (we keep a copy of that message there).
+				if filter.NeedsCopy(filterEvent) && filterEvent == event {
+					filterEvent = proto.Clone(event).(*tetragon.GetEventsResponse)
+				}
 				filter.Filter(filterEvent)
 			}