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

sys: prevent clobbering existing entries in fd tracing registry #1043

Merged
merged 2 commits into from
May 25, 2023

Conversation

ti-mo
Copy link
Collaborator

@ti-mo ti-mo commented May 25, 2023

newFD() didn't check whether the stack it tried to insert for the given fd would overwrite an existing entry.

Use LoadOrStore to check if a stack is already present, and panic if there is.

@lmb lmb force-pushed the tb/no-clobber-fd-traces branch from 0a6f6d2 to 3e729b6 Compare May 25, 2023 10:36
@ti-mo
Copy link
Collaborator Author

ti-mo commented May 25, 2023

@lmb I'm not sure what your patch accomplishes, it just moves delete + remove finalizer a bit earlier in Close()?

lmb and others added 2 commits May 25, 2023 12:06
When introducing the FD leak detector we didn't take FD.Forget()
into account, which triggers a spurious leak report when running
individual iterator tests in package link. We didn't detect this
because of another bug in the leak detector the following commit
fixes.

FD.Forget() is a really dodgy API which is only really used from
tests. Refactor the tests to be less dodgy and remove FD.Forget.

Also deal with closed FDs in File() properly.

Signed-off-by: Lorenz Bauer <lmb@isovalent.com>
newFD() didn't check whether the stack it tried to insert for the given fd
would overwrite an existing entry.

Use LoadOrStore to check if a stack is already present, and panic if there is.

Signed-off-by: Timo Beckers <timo@isovalent.com>
@lmb lmb force-pushed the tb/no-clobber-fd-traces branch from 3e729b6 to 795cbe4 Compare May 25, 2023 11:07
@ti-mo ti-mo merged commit 5915ce8 into cilium:master May 25, 2023
@ti-mo ti-mo deleted the tb/no-clobber-fd-traces branch May 25, 2023 13:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants