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

fix(processors): change args values by name #3838

Merged

Conversation

AlonZivony
Copy link
Collaborator

1. Explain what the PR does

Change all places that change args values in processors to find arguments by their names instead of index.
This way the order of the arguments received from the kernel shall not cause bugs.

2. Explain how to test it

3. Other comments

@geyslan
Copy link
Member

geyslan commented Feb 1, 2024

@AlonZivony even not reproducing the issue I was doing changes similar to yours. That's great.

Could you just add another fix to it? Please, see procTreeForkRemoveArgs(). It's not updating ArgNum.

@josedonizetti
Copy link
Collaborator

@AlonZivony Thanks! Not doing it through a set is a current problem of migrating to the new event structure, kind of doing something similar there! :D

@geyslan
Copy link
Member

geyslan commented Feb 1, 2024

@AlonZivony even not reproducing the issue I was doing changes similar to yours. That's great.

Could you just add another fix to it? Please, see procTreeForkRemoveArgs(). It's not updating ArgNum.

Already done that fix in #3839

@yanivagman
Copy link
Collaborator

LGTM

Change all places that change args values in processors to find
arguments by their names instead of index.
This way the order of the arguments received from the kernel shall not
cause bugs.

Co-authored-by: Geyslan Gregório <geyslan@gmail.com>
@geyslan
Copy link
Member

geyslan commented Feb 4, 2024

LGTM

Please don't merge before getting this into AlonZivony#4

@AlonZivony

@AlonZivony
Copy link
Collaborator Author

@geyslan I added it as part of this PR.
This is why I have put you as a co-author for the commit

@NDStrahilevitz
Copy link
Collaborator

@AlonZivony Can this be merged for the release?

@yanivagman yanivagman merged commit b8f5516 into aquasecurity:main Feb 7, 2024
32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants