[venkata-stepsecurity]

Fixes the agent memory growth reported in step-security/harden-runner#709.

Problem

ProcessMonitor.Events gets an entry for every audit sequence, and nothing ever removes one.

The audit rules include -S sendto -S sendmsg for every process. send() on a connected socket is audited as sendto with no SOCKADDR record, so these events never become ready and stay in the map forever. On a workload where short-lived processes stream results over localhost TCP, 99% of the retained entries were sendto events with no address, and the agent grew by about 1.4 GiB per minute until the runner ran out of memory.

A second problem showed up under the same load: receive returns on the first error. When the kernel's netlink socket overflows (ENOBUFS, "no buffer space available"), the loop exits and audit monitoring stops for the rest of the job, while the agent keeps running. MonitorProcesses discarded the error, so nothing was logged.

Changes

• handleRecord (the loop body, moved out so it can be tested) removes an event when its EOE record arrives. The kernel ends every multi-record event with EOE, after all of its records, so dispatch is unchanged.
• Events that never get an EOE, such as single-record events, are dropped once 8192 records arrive without touching them. The count is logged. This also bounds the map, since at most 8192 events can have been touched in the last 8192 records. PrepareEvent refreshes the event before sweeping and returns its own pointer, so the event being prepared is never dropped.
• On ENOBUFS, the loss is counted, logged on 1, 2, 4, 8, ... occurrences, and receiving continues. Other errors still stop the loop, and MonitorProcesses now logs why.

Testing

• New tests use real audit record text: sendto without an address is removed on EOE; connect, addressed sendto, execve and file-write events are each dispatched exactly once with their data; idle events are dropped, active events survive a sweep, and a late record during a sweep is handled; the receive loop keeps reading after ENOBUFS, using a fake netlink client. Each part of the fix was mutation-checked against the tests.
• go test -race ./... on Linux with Go 1.27.3: all pass except Test_addAuditRules, which needs root and also fails on int.
• End to end on GitHub-hosted ubuntu-latest, same workload, int and this branch built the same way:

	int	this branch
Agent RSS	grew to 5.0 GiB, workload stopped at low memory	43 to 49 MiB, flat for 8 minutes
Probe detections (github.com, example.com at start, middle, end)	6 of 6	6 of 6
Source overwrite detections	2	2

Not covered

Agent CPU stays near 100% on this workload, because every send() is still audited and parsed. Reducing the sendto/sendmsg audit volume is a separate change.

[step-security-bot-int]

Please find StepSecurity AI-CodeWise code comments below.

Code Comments

procmon_test.go

• [High]Avoid fatal test termination with t.Fatalf inside helper functions to allow better test granularity
  The parseAuditRecord helper calls t.Fatalf on error which immediately stops the test. Per Go testing best practices (https://golang.org/pkg/testing/#T), helper functions should return errors instead of calling fatal directly to allow tests to determine how to handle failures and report multiple errors. Change parseAuditRecord to return (*auparse.AuditMessage, error) and let the caller decide how to handle errors. For example:

func parseAuditRecord(t *testing.T, msgType auparse.AuditMessageType, text string) (*auparse.AuditMessage, error) {
t.Helper()
message, err := auparse.Parse(msgType, text)
if err != nil {
return nil, fmt.Errorf("parse %v: %w", msgType, err)
}
return message, nil
}

Then callers can do:
msg, err := parseAuditRecord(t, msgType, text)
if err != nil {
t.Fatalf("error parsing audit record: %v", err)
}

• [High]Check for and handle errors consistently instead of assuming success
  Currently the test helpers and functions call auparse.Parse without comprehensive error handling beyond t.Fatalf. While this is common in tests, more explicit error wrapping and checks improve debugging and reliability. According to Go error handling best practices (https://blog.golang.org/error-handling-and-go), errors should be wrapped or handled meaningfully. Return errors from parseAuditRecord as noted above. For production code, check and handle errors explicitly rather than implicitly assuming success or panicking.
• [Medium]Avoid excessive map growth to prevent memory bloat in production code
  The PrepareEvent method uses a sweep that avoids unbounded growth of p.Events, which is good. However, testTestPrepareEventDropsIdleEvents creates events up to 5 * pendingEventWindow, potentially large. In real usage, if pendingEventWindow or events grow much bigger, memory usage could grow unexpectedly. As per memory management best practices (https://golang.org/doc/effective_go.html#maps), consider additional caps or eviction policies. Ensure the sweep logic puts a strong cap on map size, possibly configurable based on runtime metrics. In tests, constrain special large loops to reasonable sizes.
• [Medium]Use table-driven tests uniformly with descriptive sub-test names
  The code uses table driven tests in TestHandleRecordDispatchesDetectionsOnce but other tests like TestHandleRecordRetiresSendWithoutAddressOnEOE do not follow this pattern. According to Go testing best practices (https://dave.cheney.net/2019/05/07/prefer-table-driven-tests), table-driven tests with rich descriptions improve readability and ease adding cases. Refactor single-case tests to table driven style with sub-tests using t.Run. For example, convert TestHandleRecordRetiresSendWithoutAddressOnEOE to a table test, even if it currently has one case.
• [Low]Avoid relying on string literals for syscall names
  Syscall names like "sendto" or "connect" are used as string literals in several places to validate events. According to software design best practices (https://blog.golang.org/constants), use named constants for such repeated string values to prevent typos and ease maintenance. Define constants for syscall names at the top of the file, for example:

const (
syscallSendto = "sendto"
syscallConnect = "connect"
)

and replace string literals with those constants.

• [Low]Check reference equality carefully when using copied structs
  The feedRecords helper copies dispatched events by value then appends pointers to these copies to the slice. This is correct to avoid aliasing, but for large structs copying may be inefficient. According to Go pointer usage best practices (https://golang.org/doc/effective_go.html#pointers_vs_values), assess whether copying large structs frequently is needed or if using pointers with immutability would be better. Evaluate the size of Event struct; if large, consider alternatives like returning pointers safely or implementing a Clone method to explicitly copy event data. Otherwise, document the copy logic clearly.
• [Low]Avoid repeated string formatting in test failure messages
  Formatting events with %+v in error messages is very helpful but repeated usage on large structs may slow tests. As per Go testing optimization docs (https://golang.org/pkg/testing/), limiting formatting expensive operations only on failures is advisable. Current usage is acceptable for test clarity but consider caching formatted strings or reducing output verbosity if tests become slow.
• [Low]Group imports consistently separating standard and third party packages
  The import block mixes standard library and external imports without grouping. Per Go import conventions (https://golang.org/doc/go1.11#modules), group standard library imports, then a blank line, then third party imports for better readability. Restructure imports like:

import (
"errors"
"fmt"
"os"
"syscall"
"testing"

"github.com/elastic/go-libaudit/v2/auparse"

)

• [Low]Add comments explaining complex test scenarios and magic numbers
  Tests like TestPrepareEventDropsIdleEvents use magic numbers (e.g. 5 * pendingEventWindow) without comments clarifying the significance. According to testing best practices (https://martinfowler.com/bliki/NewCode.html), comments improve test maintainability. Add descriptive comments on why these counts and windows are set as they are, so future maintainers understand the testing logic.

procmon.go

• [High]Protect access to shared state with appropriate locking
  The dropIdleEventsLocked method modifies the ProcessMonitor.Events map and fields under the assumption of being called with p.mutex locked. However, PrepareEvent calls dropIdleEventsLocked without ensuring the mutex is locked. This could lead to race conditions and data corruption. The Go memory model and best practices recommend guarding concurrent access to shared mutable state with proper synchronization (https://golang.org/doc/go1.9#race). Ensure that dropIdleEventsLocked is always called with the mutex locked by either acquiring the lock before calling it in PrepareEvent or by renaming the method and having it acquire and release the lock itself. Preferred is to acquire the lock in PrepareEvent before calling dropIdleEventsLocked as the function name indicates locked context.
• [High]Use consistent and clear lock acquisition and release patterns
  PrepareEvent locks the mutex at the start but performs significant logic including calls that are not immediately obvious to maintain the lock. Holding the lock across complex operations such as event preparation and map growth / scanning for droppable events can increase contention and reduce concurrency. According to Go concurrency best practices (https://blog.golang.org/go-maps-in-action), locks should be held for the minimal necessary scope. Refactor PrepareEvent to acquire the lock only when accessing or mutating shared state like p.Events and release early. Further, consider splitting event initialization and mutation into smaller critical sections. Also, move dropIdleEventsLocked invocation to within a locked section and limit scope during its call.
• [Medium]Avoid fragile parsing of timestamps using a fixed layout string
  The timestamp parsing in PrepareEvent uses a fixed layout string: '2006-01-02 15:04:05.999999999 +0000 UTC'. This string is precise and will fail silently if the input format changes slightly (e.g., timezone format or fractional seconds precision). Robust timestamp handling is critical in event correlation and security monitoring (https://pkg.go.dev/time#Parse). Use time.RFC3339Nano or a more tolerant parse approach. For example, try parsing with time.RFC3339Nano, and on failure, fall back to time.Now().UTC() or implement a parser that normalizes input timestamps before parsing.
• [Medium]Improve error handling by logging errors rather than ignoring them silently
  In PrepareEvent when parsing argc or timestamp, failures just lead to fallback values or a log line via WriteLog. Some errors e.g., timestamp parse errors are silently recovered with current time, which can mask upstream data issues important for audit completeness and forensic analysis. According to secure coding standards (OWASP), error handling should be explicit and informative. Enhance error logging semantics to include event sequence or additional context. Consider returning errors up the call chain or incrementing monitoring metrics to track degraded event fidelity.
• [Medium]Use idiomatic Go for type assertions and existence checks
  Many instances check for key existence and then repeatedly do conversions like fmt.Sprintf("%v", eventMap[...]) or value.([]string). This risks panics if type assertions fail, and redundant lookups increase CPU cost. Go best practices recommend a single assertion with ok-idiom and clear error or fallback handling (https://go.dev/doc/effective_go#type_switch). Use typed variable assertions with the comma-ok idiom. For example: if tags, ok := value.([]string); ok { ... } else { handle error or ignore } to avoid panics. Cache lookup results into variables to prevent multiple map lookups.
• [Low]Add comments explaining the concurrency and lifecycle assumptions for events
  Functions like retireEvent and handleRecord manipulate events with assumptions about ownership and concurrency model (pointer usage, lifecycle after dispatch). Without proper documentation, maintainers might misinterpret or misuse these APIs leading to subtle bugs. Effective commenting improves maintainability and reduces defects (https://golang.org/doc/effective_go#commentary). Add detailed comments to each exported method explaining threading, ownership, and lifecycle considerations explicitly, especially for retireEvent and handleRecord.
• [Low]Avoid global logging methods inside libraries to support flexible logging
  The code uses a global WriteLog function directly inside ProcessMonitor methods. This couples the library code to a specific logging mechanism, reducing testability and reusability. Best practices suggest dependency injection of logger interfaces (https://dave.cheney.net/2014/12/05/lets-talk-about-logging). Refactor ProcessMonitor to accept a logger interface or callback function during construction and use that for logging calls, improving modularity and testability.
• [Low]Prefer using int64 consistently for counters and indices representing record counts
  ProcessMonitor.records field is int64, but sequence numbers are converted to int. Mixing int and int64 could cause subtle bugs on 32-bit systems or when sequences grow large. The Go spec recommends consistent usage of sized integer types for counters and indexes (https://golang.org/ref/spec#Numeric_types). Use int64 consistently for sequence numbers and record counters to avoid truncation and overflow issues, especially if these numbers can become very large over time.
• [Low]Improve naming consistency by avoiding abbreviations in field names
  The Event field SentForProcessing uses a mixed naming style. For clarity and maintainability, project conventions usually favor clear full words or well-known abbreviations consistently (https://google.github.io/styleguide/go/style.html#mixed-caps). Rename SentForProcessing to SentForProcessing or SentToProcessing for consistent clarity.
• [Low]Remove redundant found variable reassignments where possible
  Inside PrepareEvent, the found variable is repeatedly reassigned for different keys from eventMap, and sometimes the value is discarded immediately after checking. This can be simplified to idiomatic single line lookup-assign-conditional to improve readability (https://golang.org/doc/effective_go#for). Use if value, found := eventMap["cwd"]; found { ... } style blocks, avoiding separate found variable reuse.

procmon_linux.go

• [High]Always check and handle errors returned from functions, especially in background processing
  In the MonitorProcesses method, p.receive(client) was called without checking for errors. Ignoring errors can lead to silent failures and make debugging difficult. According to Go best practices (Effective Go: Errors), errors should be checked and handled explicitly. Modify MonitorProcesses to capture and handle the error from p.receive:

if err := p.receive(client); err != nil {
WriteLog(fmt.Sprintf("[procmon] receive stopped: %v", err))
}

• [High]Handle specific error cases distinctly to provide robustness and prevent undesired shutdown behavior
  In the receive method, any error from r.Receive was immediately wrapped and returned. Some errors like buffer overflows (e.g., ENOBUFS) can be transient and should be handled differently (e.g., logged and retried) instead of terminating the receiver loop. This approach improves resilience and aligns with error handling guidelines from the Go standard library source and community best practices. Add an if-check to detect a receive overflow error and handle it by calling p.noteReceiveOverflow() and continuing the loop, as shown:

if isReceiveOverflow(err) {
p.noteReceiveOverflow()
continue
}
return errors.Wrap(err, "receive failed")

• [Medium]Reduce repeated map lookups and improve readability by storing repeated expressions in local variables
  In receive, p.Events[int(message.Sequence)] was accessed multiple times. Repeated map lookups with the same key decrease readability and may have minor performance impacts. Idiomatic Go recommends local variables for repeated access as per Effective Go. Refactor by storing p.Events[int(message.Sequence)] in a local variable before using multiple times or replace redundant usage with a single call if possible. In this code, the patch replaces this with p.handleRecord, encapsulating the access and logic.
• [Medium]Encapsulate complex event preparation and processing logic inside a dedicated method to improve maintainability
  The original code had calls spread between PrepareEvent, isEventReady, markEventSent, and eventHandler.HandleEvent. The patch streamlines this by using p.handleRecord which returns a single event to handle. According to Go best practices and clean code principles, encapsulating related logic improves code clarity and reduces the chance of errors. Implement a method like p.handleRecord(Type, Message) that prepares, checks, and marks the event as sent. Then use:

if event := p.handleRecord(rawEvent.Type, message); event != nil {
go eventHandler.HandleEvent(event)
}

• [Low]Log contextual information when errors occur to aid troubleshooting and monitoring
  When p.receive returns an error, the new code logs the error with context. Logging errors helps with observability and aligns with recommendations from the Go logging best practices. Add a logging statement upon error return in MonitorProcesses:

if err := p.receive(client); err != nil {
WriteLog(fmt.Sprintf("[procmon] receive stopped: %v", err))
}

• [Low]Use explicit type conversions only when necessary and keep integer conversions close to usage
  Int conversions are used on message.Sequence frequently. Keeping them close to usage and avoiding repeated conversions improves readability in Go (Effective Go). Keep int(message.Sequence) applied once when needed instead of repeated throughout the method, as the patch has done by refactoring to use handleRecord.

procmon_linux_test.go

• [High]Add context and cancellation support to the receive method
  The receive method on ProcessMonitor currently uses a blocking call to AuditClient.Receive without any context or timeout support. This can cause the system to hang indefinitely if the Receive call blocks or fails to return. According to Go's best practices and effective concurrency patterns (as outlined in golang.org's context package and Go concurrency patterns), long-running operations interacting with external systems should support context for cancellation and deadlines. Modify the receive method to accept a context.Context parameter and use a version of AuditClient.Receive that respects context cancellation. If AuditClient.Receive does not support context natively, consider running it in a separate goroutine and select on context.Done() to allow cancellation and graceful termination.
• [High]Improve error handling by explicitly checking for expected errors and wrapping errors with additional context
  While the code checks for specific syscall errors like ENOBUFS and EBADF, it returns raw errors without providing additional context. As per Go error handling best practices (see 'Error Handling and Go' blog by Dave Cheney), errors should be wrapped with contextual messages using errors.Wrap or fmt.Errorf("...: %w", err) to aid debugging and observability. Wrap returned errors with descriptive context at every return point in the receive method and related functions. For example, instead of returning err directly, use fmt.Errorf("audit client receive failed: %w", err) to clarify the source of the error.
• [Medium]Ensure thread-safe access to shared fields such as receiveOverflow, records, and Events
  The ProcessMonitor struct fields (receiveOverflow, records, Events) are mutated and read in the test and presumably in real code. If these fields can be accessed concurrently, they must be protected by synchronization primitives (mutexes) to prevent data races. According to the Go race detector documentation and race condition best practices (golang.org race docs), unsynchronized concurrent access can result in unpredictable behavior. Add a sync.Mutex or sync.RWMutex to ProcessMonitor and protect all accesses and mutations of shared mutable state. Alternatively, ensure that all calls to receive are serialized or that the struct is only accessed from a single goroutine.
• [Medium]Add validation for incoming audit messages before processing
  The code in fakeNetlink returns audit messages based on the text field without validation. In production code, audit messages should be validated for correctness and sanity to prevent processing malformed data, which is a security best practice (OWASP Input Validation). Introduce validation logic to check audit message format, required fields, and length before processing. Reject or log invalid messages accordingly.
• [Low]Use descriptive test failure messages and table-driven tests for extensibility
  The unit test TestReceiveContinuesAfterOverflow uses static sequences and hardcoded checks without generalized validation or descriptive messages. Following Go testing best practices (Effective Go and Go testing package docs), table-driven tests improve test coverage and readability. Refactor the test into table-driven style enumerating error injection cases and expected outcomes with clear failure messages for each scenario.
• [Low]Implement a Close method on ProcessMonitor to release resources cleanly
  Although the fakeNetlink has a Close method, the ProcessMonitor does not provide resource cleanup APIs, which is important for long-running monitoring systems to avoid resource leaks (Go best practices on resource management). Implement a Close method on ProcessMonitor that calls Close on the embedded AuditClient or Netlink interface, ensuring all network and OS resources are freed when done.
• [Low]Document exported types and methods comprehensively
  There is no commentary on exported types like ProcessMonitor and its methods. The official Go documentation and Effective Go strongly recommend providing clear docstrings following GoDoc conventions to improve maintainability and usability. Add GoDoc comment blocks on all exported types, functions, methods, and test cases explaining their purpose, parameters, and behaviors.

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.