[venkata-stepsecurity]

Follow-up to #503.

Problem

go-libaudit's reassembler delivers events in sequence order: an event is delivered only when every earlier event in flight has completed (EOE or PROCTITLE record) or been given up. Records that are not part of an audited syscall, such as BPF program loads and unloads or ANOM_PROMISCUOUS, can arrive alone without a record that completes an event. Each one holds every later event until it times out (2 s), or until 50 events are in flight.

Handlers look up processes in /proc when an event is dispatched (parent process, container, executable). A delayed event can miss a short-lived process or its parent.

Measured on GitHub-hosted runners (ubuntu-24.04, amd64 and arm64), with the agent's audit rules and a 90 s workload (sudo, docker, git, curl, python):

reassembler fed with	syscall events delayed > 100 ms	max delay
every record (current int)	170 of 3,544 (amd64), 58 of 3,358 (arm64)	512 ms, 237 ms
only records of audited syscall events	0	1 ms

No syscall event was missing with the filter. The only records it dropped from syscall events were BPF and ANOM_PROMISCUOUS records logged before the SYSCALL record.

Change

• Pass a record to the reassembler only if it is a SYSCALL record, or belongs to an event whose SYSCALL record was already passed. Every record the agent uses (EXECVE, SOCKADDR, CWD, PATH, PROCTITLE) is logged after SYSCALL (kernel audit_log_exit). The set of open events is cleared when the reassembler delivers an event, so it is bounded by the events in flight.
• Records that arrive after their event was delivered (or given up) are dropped instead of starting a new event that never completes.
• Pushing a record, maintenance and the final flush take the same lock, so an event cannot be given up between a record being admitted and being pushed.

Testing

• New tests: records outside syscall events do not hold later events (unit and through receive with a fake netlink socket), records after delivery are dropped, completion on EOE alone, the open-event set is bounded, and a late record racing maintenance (forces the interleaving with a blocking callback).
• Full package with -race (Go 1.27.3 container, Test_addAuditRules skipped as it needs iptables): 48 pass, int 42. TestRun fails the same way on int in the container.
• Mutation check: pushing every record, not clearing delivered events, removing the lock, or bypassing the filter in receive each make at least one test fail.
• The same change was made and tested for the enterprise agent, including an A/B run on GitHub-hosted runners where detections were identical and the delayed detection was recovered.
• Int canary workflows (harden-runner installing this build, amd64 and arm64): the 6 workflows that run this agent passed; the agent logged commit=efd33da. The seventh uses the enterprise agent and fails on test branches regardless of the agent.
• Reviewed with Codex: the port matches the enterprise change, and replaying standalone records and the late record race against this branch dispatches in about 1 ms (2 s on int).

Notes

• If an audited syscall's own last records are lost (for example during a netlink overflow), later events can still wait until it is given up; the 50 event in-flight limit bounds this.
• [procmon] gaps in audit sequence numbers also counts the skipped events, so it is not a count of lost events.
• Touches the same receive loop as Skip parsing audit events that cannot be detections #505; whichever merges second needs a rebase.

[step-security-bot-int]

Please find StepSecurity AI-CodeWise code comments below.

Code Comments

procmon.go

• [High]Synchronize access to shared map syscallEvents consistently with a single lock
  The auditStream structure uses two separate mutexes: ops and mu. The syscallEvents map is protected only by mu, but it is accessed at the same time as ops mutex is held in push and ReassemblyComplete. This inconsistent lock usage can lead to race conditions or deadlocks. According to Go concurrency best practices (https://golang.org/doc/effective_go#concurrency), shared mutable state must be protected consistently with the same mutex to avoid data races. Refactor the code to protect all accesses to syscallEvents with a single mutex (preferably ops), or acquire both mutexes in a consistent order every time. For example, remove mu and use ops mutex exclusively when reading or writing syscallEvents.
• [High]Add error handling for the Reassembler Maintain call
  The maintain method calls r.Maintain() but returns its error directly with no logging or retry logic. Lack of error handling can cause silent failures and loss of audit event reconstructions. According to error handling best practices in Go (https://go.dev/doc/effective_go#errors), errors should at least be logged or managed appropriately. Add logging for errors returned by r.Maintain(). If maintain fails, implement retries or escalate errors suitably to avoid silent loss of audit data.
• [Medium]Check for nil pointer dereferences in ReassemblyComplete
  ReassemblyComplete assumes msgs[0] exists and accesses msgs[0].Sequence without intermediate nil or length checks beyond the initial len check. Additionally, msgs[0] itself might be nil. This can cause runtime panics. Defensive programming is recommended as per Go best practices (https://go.dev/doc/effective_go#commentary). In ReassemblyComplete, after checking len(msgs) > 0, also check if msgs[0] is non-nil before accessing its Sequence field.
• [Medium]Consider using sync.RWMutex for syscallEvents map if access is mostly read
  The syscallEvents map is only checked for existence in push and deleted in ReassemblyComplete, while being protected by a mutex. Since there are more reads than writes, using sync.RWMutex could improve concurrent read performance per Go race and synchronization best practices (https://golang.org/pkg/sync/#RWMutex). Replace mu sync.Mutex with mu sync.RWMutex and use RLock/RUnlock in push and Lock/Unlock in ReassemblyComplete.
• [Medium]Initialize syscallEvents map in constructor to avoid nil checks
  The syscallEvents map is lazily initialized inside push under mutex protection, which adds overhead and risks data race if other parts access it before push executes. To improve code clarity and reliability, maps should be initialized before use (https://golang.org/doc/effective_go#maps). Initialize syscallEvents as make(map[uint32]struct{}) when auditStream struct is created.
• [Low]Add context or cancellation support for maintain and flush methods
  The maintain and flush operations lock ops mutex, which could block indefinitely if called from a context that should be canceled. Best practices suggest designing long-running or potentially blocking operations with context support for graceful shutdown (https://blog.golang.org/context). Add context.Context parameters to maintain and flush methods and respect cancellation signals to avoid blocking that could stall the system.
• [Low]Add comments explaining the locking strategy and map usage in auditStream
  The code has multiple mutexes and a map to track syscall event sequences but lacks explicit documentation on their interactions. Clear documentation aids maintenance and reduces bugs (https://google.github.io/styleguide/go/). Add comments describing why both ops and mu exist, which protects what, and the lifecycle of syscallEvents.
• [Low]Use consistent naming conventions for mutex fields
  The struct uses mu and ops for mutex names. This is ambiguous and reduces clarity. Go style favors more descriptive names (https://golang.org/doc/effective_go#names). Rename mu to syscallEventsMu or eventsMu to clarify its purpose.
• [Low]Avoid deferred unlocks in tight loops or high-frequency functions for performance
  push unlocks mu and ops via defer. Frequent defer in performance-critical code can add overhead (https://dave.cheney.net/2014/12/05/dont-panic-and-routine-exit). In push, replace defer unlock with explicit unlock calls as late as possible to reduce defer overhead.
• [Low]Add unit tests covering concurrency scenarios for auditStream methods
  Concurrency bugs can be subtle. Tests that simulate concurrent calls to push, maintain, flush, and ReassemblyComplete ensure correctness (https://blog.golang.org/race-detector). Add concurrency tests in the test suite that call auditStream methods from multiple goroutines and validate no data races or deadlocks occur.

procmon_linux.go

• [High]Perform error handling immediately after message parse and continue loop to avoid invalid data processing
  Based on Go error handling best practices (Effective Go, Go Blog), error checks need to handle the error immediately before operating on the value. In the original code, if the message parsing failed, it still attempted to push to the reassembler. This can lead to undefined behavior or panics. Add 'continue' after logging the parse error to skip the current iteration and avoid using invalid message data.

Code fix:

if err != nil {
noteLoss(&p.invalidRecords, 1, fmt.Sprintf("dropped audit record with an invalid header: %v", err))
continue
}

• [Medium]Refactor Reassembler maintenance logic into auditStream to encapsulate related functionality
  According to Clean Code principles and Go idiomatic design, encapsulating related logic (like maintenance of reassembler inside auditStream) improves modularity and readability of code. This change follows the principle of grouping closely related functions together. Modify 'maintainReassembler' function signature to accept auditStream instance and delegate maintaining logic to it.

Code fix:

func maintainReassembler(s *auditStream, r *libaudit.Reassembler, done <-chan struct{}) {
// existing logic now calls s.maintain(r)
}

• [Medium]Call 'stream.flush' instead of 'reassembler.Close' in defer to ensure consistent state flush before shutdown
  Closing resources and flushing buffers properly prevents data loss (Effective Go). By calling 'stream.flush' instead of simply closing the reassembler, the code ensures final event processing and consistent cleanup. Replace 'reassembler.Close()' with 'stream.flush(reassembler)' in the deferred cleanup to properly flush audit stream before closure.

Code fix:

defer func() {
close(done)
maintaining.Wait()
stream.flush(reassembler)
}()

• [Low]Introduce a local variable 'stream' for auditStream to improve readability and separate concerns
  Following Go best practices on clarity (Effective Go), assigning 'auditStream' to a named variable clarifies the code's intent, separating creation from usage. Create a separate 'stream' variable before passing it to NewReassembler.

Code fix:

stream := &auditStream{monitor: p, dispatch: dispatch}
reassembler, err := libaudit.NewReassembler(reassemblerMaxInFlight, reassemblerTimeout, stream)

procmon_linux_test.go

• [High]Avoid hardcoding fixed timeout durations in tests
  Hardcoding timeout durations, like time.After(reassemblerTimeout / 2), can lead to flaky tests if the environment or code performance changes. Use configurable or adaptive timeouts to make tests more reliable. This aligns with guidance from 'Effective Go' and general testing best practices to avoid fragile tests. Refactor the test to accept a configurable timeout value or derive the timeout dynamically, possibly with a multiplier or a context with timeout, instead of relying on a fixed duration. For example, pass a timeout parameter or use a context.Context with a deadline.
• [Medium]Use context.Context for cancellation and timeouts in goroutines
  The goroutine in the test uses a channel and a hard stop to signal completion or error, but does not leverage Go's context package. Contexts provide a standard way to manage cancellation and deadlines, improving code clarity and preventing resource leaks. This is recommended by the Go blog and official Go documentation. Modify the receive method and the test to accept a context.Context to control cancellation and timeouts instead of relying on channel-based release signals and fixed timeouts.
• [Medium]Check error types more robustly when expecting syscall errors
  The test checks for the error using errors.Is(err, syscall.EBADF), which is good practice. However, ensure that any wrapped errors or additional context is properly handled by using errors.Is instead of direct equality. This follows the Go 1.13+ error wrapping best practices. Ensure that throughout the codebase, all relevant error returns are wrapped using fmt.Errorf and %w, enabling the test's use of errors.Is. No change needed here if wrapping is already done, but audit related code to confirm.
• [Low]Add comments to clarify the role of 'release' channel synchronization
  The use of the 'release' channel as a synchronization mechanism in the fakeNetlink steps can be unclear to readers, which might reduce maintainability. Clear comments improve code understanding and align with best practices as documented in effective commenting guidelines. Add inline comments explaining the purpose and behavior of the 'release' channel in controlling when certain messages are allowed to proceed.
• [Low]Use table-driven testing for similar test cases
  This test is specific, but if there are or will be multiple tests that verify event holding behavior under different conditions, using a table-driven approach improves scalability and reduces duplication, as per Go testing best practices. Refactor this test and related ones into a table-driven format with descriptive test cases iterated in a loop.
• [Low]Validate channel buffer capacities to prevent hidden deadlocks
  The dispatched channel is created with a buffer of size 1; verify buffer sizes sufficiently decouple producer/consumer to avoid blocking scenarios during dispatch. Analyze and increase channel buffer size if the test or production code needs to dispatch multiple events concurrently without blocking.
• [Low]Prefer descriptive test failure messages
  Failure messages like 'unexpected event %+v' can be enhanced by specifying expected vs actual values for quicker debugging. Change test failure calls to include expected and actual values distinctly, e.g. t.Fatalf("expected IPAddress '192.0.2.10', got %q", e.IPAddress).

procmon_test.go

• [High]Add error handling after calls to r.Maintain() and r.Close()
  According to Go best practices (Effective Go, Go Blog), errors returned from functions should always be checked and handled properly to avoid silent failures or unexpected behavior. In the provided tests, calls to r.Maintain() and r.Close() occasionally return errors which are not consistently checked. Add proper error checks for calls to r.Maintain() and r.Close(). For example: if err := r.Maintain(); err != nil { t.Fatalf("maintain: %v", err) } and similarly for r.Close() to ensure that test failures or unexpected states are detected early.
• [High]Ensure concurrency-related resources are cleaned up during test teardown
  The tests using goroutines and waiting mechanisms (channels and sync.Once) should ensure proper cleanup to prevent resource leaks or deadlocks, per Go concurrency guidelines (https://go.dev/doc/effective_go#concurrency). The test TestLateRecordWaitsForTheEventToBeGivenUp uses goroutines and channels but does not always guarantee all goroutines exit cleanly, for example when test fails early. Use t.Cleanup() consistently to close channels and wait for goroutines, and check for errors properly to prevent goroutine leaks. Also add timeout and explicit cancellation where appropriate to avoid hanging tests.
• [Medium]Add comments explaining the purpose of complex tests and concurrency mechanisms
  The code introduces concurrency management with sync.Once, channels, and non-trivial test scenarios. Per cleanup and maintainability best practices (Go Code Review Comments), complex logic should have explanatory comments for clarity to ease future maintenance. Add comments in tests like TestLateRecordWaitsForTheEventToBeGivenUp explaining the role of blockingStream, how releasing the channel affects deferred runs, and why pushing is deferred. This documentation helps future maintainers quickly understand the test's goals.
• [Medium]Avoid reusing variable names for unrelated objects in test loops
  In the loop in TestSyscallEventsAreBounded, the variable 'seq' and the function syscallRecord are used repeatedly without clear scoping or nil checks. Per Go style guidelines, variables used in loops should be scoped carefully to avoid capturing oversights, especially in closures. Use explicit variable scoping and avoid capturing outer variables in nested function literals (if any). Here it's safe, but adding a comment or making the loop body explicit reduces risk in future edits.
• [Low]Consistent formatting of error messages in tests
  Error messages in tests sometimes use different forms, e.g., t.Fatalf("expected the connect to be dispatched once, got %d", ...) vs t.Fatalf("expected the connect to be dispatched once, got %+v", ...). Consistency aids debugging and scanning through logs, a principle supported by Go testing practices and logging best practices. Standardize formatting of messages, using %+v for structs and %d for counts, and ensure messages are informative and follow a common style throughout the tests.
• [Low]Use time constants instead of numeric literals for durations
  Using explicit time.Duration values like 50*time.Millisecond instead of numeric literals encourages clarity and reduces magic numbers, as emphasized in Effective Go and many style guides. Replace all numeric durations in time.Sleep and timeout specifications with named constants or clearly indicate units, e.g., const reassemblerTestTimeout = 50 * time.Millisecond.
• [Low]Avoid unnecessary dereferencing and explicitly check nil where required
  Accessing fields like stream.delivered.Load() assumes non-nil pointers without explicit checks, which can cause panics in cases of test setup errors or future changes. Defensive programming is recommended (Google Go Style Guide). Add nil checks before accessing pointer fields in tests or provide deeper guarantees in test setup that pointers cannot be nil.
• [Low]Add timeout constants or configuration to avoid magic timeout values in tests
  Tests currently use hardcoded timeout values like 5 seconds. Moving these to constants improves maintainability and clarity, as recommended in common Go test patterns. Define constants like var testLongTimeout = 5 * time.Second and refer to them in the tests to make future adjustments easier and promote reuse.

Feedback

We appreciate your feedback in helping us improve the service! To provide feedback, please use emojis on this comment. If you find the comments helpful, give them a 👍. If they aren't useful, kindly express that with a 👎. If you have questions or detailed feedback, please create n GitHub issue in StepSecurity/AI-CodeWise.