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] fix probe_read_str return type #3236

Merged
merged 1 commit into from
Dec 20, 2024
Merged

[fix] fix probe_read_str return type #3236

merged 1 commit into from
Dec 20, 2024

Conversation

arthur-zhang
Copy link
Contributor

@arthur-zhang arthur-zhang commented Dec 18, 2024

Fixes potential bug

Description

Since size is an unsigned type (__u32), it will never be less than 0. Therefore, the following code logic will never be executed:

__u32 size = 0;

if (size < 0) {
    flags |= EVENT_ERROR_FILENAME;
    size = 0;
}

Changelog

Fix return type of variable from probe_read_str from __u32 to __s32 in bpf_execve_event.c read_path to catch error cases.

@arthur-zhang arthur-zhang requested a review from a team as a code owner December 18, 2024 07:13
Copy link
Contributor

@olsajiri olsajiri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the fix
please add the description from the PR to the changelog

@@ -130,7 +130,7 @@ FUNC_INLINE __u32
read_path(void *ctx, struct msg_execve_event *event, void *filename)
{
struct msg_process *p = &event->process;
__u32 size = 0;
int size = 0;
__u32 flags = 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice catch, please use __s32

@mtardy mtardy added the release-note/bug This PR fixes an issue in a previous release of Tetragon. label Dec 20, 2024
Copy link
Member

@mtardy mtardy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey! I touched your changelog, could you squash your two commits into one so that we can merge this :)

@mtardy mtardy added the area/bpf This is related to BPF code label Dec 20, 2024
Copy link

netlify bot commented Dec 20, 2024

Deploy Preview for tetragon ready!

Name Link
🔨 Latest commit c7ea4aa
🔍 Latest deploy log https://app.netlify.com/sites/tetragon/deploys/67653eff66ad1600089971de
😎 Deploy Preview https://deploy-preview-3236--tetragon.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@mtardy
Copy link
Member

mtardy commented Dec 20, 2024

Hey, you have 4 commits now, you can do this to squash those commits into one:

git rebase -i <first commit of the branch>^

Then put f for fixup or s for squash on the commits you want to squash into the first one.
Then when ready push with

git push origin HEAD --force-with-lease

@arthur-zhang
Copy link
Contributor Author

Hey, you have 4 commits now, you can do this to squash those commits into one:

git rebase -i <first commit of the branch>^

Then put f for fixup or s for squash on the commits you want to squash into the first one. Then when ready push with

git push origin HEAD --force-with-lease

ok,

Hey, you have 4 commits now, you can do this to squash those commits into one:

git rebase -i <first commit of the branch>^

Then put f for fixup or s for squash on the commits you want to squash into the first one. Then when ready push with

git push origin HEAD --force-with-lease

thanks, done.

Copy link
Member

@mtardy mtardy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! :)

@mtardy mtardy merged commit 1ccf6c1 into cilium:main Dec 20, 2024
40 of 41 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/bpf This is related to BPF code release-note/bug This PR fixes an issue in a previous release of Tetragon.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants