-
Notifications
You must be signed in to change notification settings - Fork 424
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
argument parsers improvements #4279
base: main
Are you sure you want to change the base?
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
a349ec7
to
58a8fb4
Compare
This comment was marked as duplicate.
This comment was marked as duplicate.
After all refactor done, I pretend to split it in different files. I also have an idea to try to reduce ParseArgs() logic, but gonna play with it only after finishing the whole data parsers. |
58a8fb4
to
7eb0e8f
Compare
7eb0e8f
to
32999c7
Compare
6e04d2f
to
62541c2
Compare
62541c2
to
5c9c6b9
Compare
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 reviewed the first 2 commits and added some doubts.
I assume the comments there will affect the rest of the code so I will continue the review after clarifying these points
@@ -13,11 +13,27 @@ import ( | |||
"github.com/aquasecurity/tracee/pkg/utils/environment" | |||
) | |||
|
|||
type SystemFunctionArgument interface { | |||
|
|||
type systemFunctionArgument interface { |
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.
Why making the interface private?
Isn't the idea behind interfaces to be exposed between different packages?
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.
It is not used (imported) so I just made it private via c071141 by creating a public concrete type with same name - since dealing with the concrete or its pointer is cheaper than dealing with an interface.
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.
As we discussed I'm going to remove the private interface (enforcer). 👍🏼
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.
Well, we'll have to wait a bit to remove that interface since other still not refactored parsers consumes it.
5c9c6b9
to
9bc2df9
Compare
Dealing with a concrete type instead of an interface is more efficient. systemFunctionArgument interface will be replaced by SystemFunctionArgument struct in future efforts.
Iterate slice instead of conditional branch. Return string only instead of the Argument type since it's the only value used. | BenchmarkParseCloneFlags-32 | Before | After | |-----------------------------|----------|--------| | Execution Time (ns/op) | 1117 | 696.7 | | Memory Allocated (B/op) | 1192 | 736 | | Allocations per Operation | 27 | 25 |
When possible iterates slice instead of conditional branch. Return string only instead of the Argument type since it's the only value used. Fix logic, since it wasn't printing O_RDONLY flag alone.
Iterate slice instead of conditional branch. Return string only instead of the Argument type since it's the only value used. Fix logic, since it wasn't printing F_OK flag alone.
Introduce ParseFaccessatFlag to parse faccessat flags. Introduce ParseFchmodatFlag to parse fchmodat flags. Iterate slice instead of conditional branch. Return string only instead of the Argument type since it's the only value used. Fix ParseExecFlag (now ParseExecveatFlag) to check only related flags.
Add missing flags to ParseCapability: CAP_PERFMON, CAP_BPF and CAP_CHECKPOINT_RESTORE Use slice instead of maps. This allows for direct access to values via index. Return string only instead of the Argument type since it's the only value used.
Add missing flags to ParsePrctlOption: PR_SET_IO_FLUSHER, PR_GET_IO_FLUSHER, PR_SET_SYSCALL_USER_DISPATCH, PR_PAC_SET_ENABLED_KEYS, PR_PAC_GET_ENABLED_KEYS, PR_SCHED_CORE, PR_SME_SET_VL, PR_SME_GET_VL, PR_SET_MDWE, PR_GET_MDWE, PR_SET_MEMORY_MERGE and PR_GET_MEMORY_MERGE. Iterate slice instead of fetching a map. Return string only instead of the Argument type since it's the only value used.
Add missing flags to ParseBPFCmd: BPF_PROG_BIND_MAP, BPF_TOKEN_CREATE Use slice instead of maps. This allows for direct access to values via index. Return string only instead of the Argument type since it's the only value used.
Add missing flags to ParsePtraceRequestArgument: PTRACE_GET_THREAD_AREA, PTRACE_SET_THREAD_AREA, PTRACE_ARCH_PRCTL, PTRACE_SYSEMU, PTRACE_SYSEMU_SINGLESTEP, PTRACE_SINGLEBLOCK, PTRACE_GET_RSEQ_CONFIGURATION, PTRACE_SET_SYSCALL_USER_DISPATCH_CONFIG and PTRACE_GET_SYSCALL_USER_DISPATCH_CONFIG. Iterate slice instead of fetching a map. Return string only instead of the Argument type since it's the only value used.
Use slice instead of maps. This allows for direct access to values via index. Return string only instead of the Argument type since it's the only value used.
Add missing flag to ParseSocketDomainArgument: AF_MCTP Use slice instead of maps. This allows for direct access to values via index. Return string only instead of the Argument type since it's the only value used.
syscalls with dirfd arg now parse for special case AT_FDCWD when ParseArgumentsFDs is true.
9bc2df9
to
06328f8
Compare
@@ -23,7 +23,8 @@ func ParseArgs(event *trace.Event) error { | |||
} | |||
} | |||
|
|||
switch ID(event.EventID) { | |||
evtID := ID(event.EventID) |
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.
After all refactoring (optimisation) is complete, I plan to make ParseArgs() smaller (cache wise), fetching parsers from a slice or map based on its eventId. It will certainly help to deduplicate code and expand the parsing to other events.
1. Explain what the PR does
This is a sequence of the effort already started by #4200. As it was getting huge, the continuation will come after.
Improvements include reducing the size of the parser logic by using slices instead of maps whenever possible. This allows for direct access to values via index, and when direct access isn't feasible, iterating through slices is generally more efficient than fetching values from a map.
Return string only instead of the Argument type since it's the only value used.
This PR reduces the size of the final binary by ~56KB.
06328f8 chore: add todo comment
5cae44c feat: parse dirfd for special case AT_FDCWD
a5249cb perf: chore: add flag to ParseSocketDomainArgument
171603e perf: chore: improve ParseSocketcallCall
ea2e2c9 perf: chore: add flags ParsePtraceRequestArgument
4505a3d perf: chore: add missing flags to ParseBPFCmd
f5322b8 perf: chore: add missing flags to ParsePrctlOption
5c43a93 perf: chore: add missing flags to ParseCapability
99dab5a feat: perf: fix: AT flags parsing
6ad46f4 perf: fix: improve ParseAccessMode
c892271 perf: fix: improve ParseOpenFlagArgument
8aa585f perf: improve ParseCloneFlags
13348fe chore: add comment about raw values
05b7830 chore: deprecate internal interface
5cae44c feat: parse dirfd for special case AT_FDCWD
a5249cb perf: chore: add flag to ParseSocketDomainArgument
171603e perf: chore: improve ParseSocketcallCall
ea2e2c9 perf: chore: add flags ParsePtraceRequestArgument
4505a3d perf: chore: add missing flags to ParseBPFCmd
f5322b8 perf: chore: add missing flags to ParsePrctlOption
5c43a93 perf: chore: add missing flags to ParseCapability
99dab5a feat: perf: fix: AT flags parsing
6ad46f4 perf: fix: improve ParseAccessMode
c892271 perf: fix: improve ParseOpenFlagArgument
8aa585f perf: improve ParseCloneFlags
05b7830 chore: deprecate internal interface
2. Explain how to test it
3. Other comments