Skip to content
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

Possible bug in Test_EventFilters when a case has many cmdEvent and each cmdEvent has different number of expectEvent. #4255

Open
canoriz opened this issue Aug 18, 2024 · 1 comment · May be fixed by #4305
Assignees
Labels
Milestone

Comments

@canoriz
Copy link

canoriz commented Aug 18, 2024

Description

This is not an existing bug but a possible bug. I am developing a feature (a new kind of event), when adding a test for my feature, I encountered a bug in integration-tests. My test case looks like this, after running ping, tracee should emit events.SchedProcessExec and events.MyDevelopingEvent.

		{
			name: "xxx",
			policyFiles: []testutils.PolicyFileWithID{
				{
					Id: 1,
					PolicyFile: v1beta1.PolicyFile{
						Metadata: v1beta1.Metadata{
							Name: "xxx",
						},
						Spec: k8s.PolicySpec{
							Scope:          []string{"global"},
							DefaultActions: []string{"log"},
							Rules: []k8s.Rule{
								{
									Event:   "sched_process_exec",
									Filters: []string{"comm=ping"},
									...
								},
								{
									Event:   "sched_process_exit",
									Filters: []string{"comm=who"},
								},
							},
						},
					},
				},
			},
			cmdEvents: []cmdEvents{
				newCmdEvents(
					"who",
					0,
					1*time.Second,
					[]trace.Event{
						expectEvent(anyHost, "who", testutils.CPUForTests, anyPID, 0, events.SchedProcessExit, orPolNames("xxx"), orPolIDs(1)),
					},
					[]string{},
					false,
				),
				newCmdEvents(
					"ping -c1 0.0.0.0",
					0,
					1*time.Second,
					[]trace.Event{
						expectEvent(anyHost, "ping", testutils.CPUForTests, anyPID, 0, events.SchedProcessExec, orPolNames("xxx"), orPolIDs(1)),
						expectEvent(anyHost, "ping", testutils.CPUForTests, anyPID, 0, events.MyDevelopingEvent, orPolNames("xxx"), orPolIDs(1)),
					},
					[]string{},
					true,
				),
			},
			useSyscaller: false,
			coolDown:     0,
			test:         ExpectAllInOrderSequentially,
		},

Line 2482 calculates index of actEvtsCopy, the expression cmdIdx*len(cmd.expectedEvents)+evtIdx will produce wrong index when cmd in cmdEvents have different number of expectedEvents. Take my test case as an example, when comparing output of runCmd("ping"), the correct index should be 1 and 2, but the expression gives 2 and 3 because len(cmd.expectedEvents) of "ping" is 2 and len(cmd.expectedEvent) of "who" is 1.

for cmdIdx, cmd := range cmdEvents {
syscallsInSets := []string{}
checkSets := len(cmd.sets) > 0
if checkSets {
syscallsInSets = getAllSyscallsInSets(cmd.sets)
}
// compare the expected events with the actual events in the same order
for evtIdx, expEvt := range cmd.expectedEvents {
actEvt := actEvtsCopy[cmdIdx*len(cmd.expectedEvents)+evtIdx]

Output of tracee version:

I think main branch also have this possible bug. I give version v0.21.0 here.

v0.21.0

Output of uname -a:

Linux 6.9.9-amd64 #1 SMP PREEMPT_DYNAMIC Debian 6.9.9-1 (2024-07-13) x86_64 GNU/Linux

Additional details

I suggest changing code to below.

	actEvtIdx := 0
	for _, cmd := range cmdEvents {
		syscallsInSets := []string{}
		checkSets := len(cmd.sets) > 0
		if checkSets {
			syscallsInSets = getAllSyscallsInSets(cmd.sets)
		}

		// compare the expected events with the actual events in the same order
		for _, expEvt := range cmd.expectedEvents {
			// runCmds ensures at least same number of events were received in actual,
			// hence no out of range panic here
			actEvt := actEvtsCopy[actEvtIdx]
			actEvtIdx++
@geyslan
Copy link
Member

geyslan commented Aug 21, 2024

@canoriz thanks, I'm going to check your suggestion.

@geyslan geyslan self-assigned this Aug 23, 2024
@geyslan geyslan added this to the v0.23.0 milestone Aug 23, 2024
geyslan added a commit to geyslan/tracee that referenced this issue Sep 17, 2024
ExpectAllInOrderSequentially might try to access an index out of range
depending on the number of events that are being checked.

More about in issue aquasecurity#4255.
@geyslan geyslan linked a pull request Sep 17, 2024 that will close this issue
geyslan added a commit to geyslan/tracee that referenced this issue Sep 25, 2024
ExpectAllInOrderSequentially might try to access an index out of range
depending on the number of events that are being checked.

More about in issue aquasecurity#4255.
geyslan added a commit to geyslan/tracee that referenced this issue Sep 25, 2024
ExpectAllInOrderSequentially might try to access an index out of range
depending on the number of events that are being checked.

More about in issue aquasecurity#4255.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants