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.

Sign up for GitHub

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

Jump to bottom

datapath/linux/arp: avoid leaking sock fd if unix.SetNonblock fails in func listen #15646

Merged
merged 1 commit into from
Apr 12, 2021
Merged

datapath/linux/arp: avoid leaking sock fd if unix.SetNonblock fails in func listen #15646

merged 1 commit into from
Apr 12, 2021

Conversation

tklauser
Copy link
Member

At this point, the sock fd is not yet wrapped in an *os.File, so it
needs to be closed explicitly on error.

@tklauser
datapath/linux/arp: avoid leaking sock fd if unix.SetNonblock fails i…
c3b34eb 
…n func listen

At this point, the sock fd is not yet wrapped in an *os.File, so it
needs to be closed explicitly on error.

Signed-off-by: Tobias Klauser <tobias@cilium.io>
@tklauser tklauser added the release-note/misc This PR makes changes that have no direct user impact. label Apr 12, 2021
@tklauser tklauser requested review from a team and jibi April 12, 2021 10:09
@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.10.0 Apr 12, 2021
brb
brb approved these changes Apr 12, 2021
View reviewed changes
Copy link
Member

@brb brb left a comment

Choose a reason for hiding this comment

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

Thanks!

How did you find this? With a static analysis tool?

@tklauser
Copy link
Member Author

Thanks!

How did you find this? With a static analysis tool?

Nope, just caught this through manual code review. I manually checked whether we have any other cases of this (grepped for os.NewFile and checked how we deal with the fd) in our code base and it seems this is the only one. There are some cases in vendored dependencies though and I'm about to send PRs for these.

@tklauser
Copy link
Member Author

test-me-please

@tklauser
Copy link
Member Author

BPF checkpatch is complaining about the commit subject length: https://github.com/cilium/cilium/pull/15646/checks?check_run_id=2322742643 All other tests passed and necessary reviews are in. Marking as ready to merge.

@tklauser tklauser added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Apr 12, 2021
@qmonnet qmonnet merged commit 73b0d4a into cilium:master Apr 12, 2021
1.10.0 automation moved this from In progress to Done Apr 12, 2021
@tklauser tklauser deleted the arping-avoid-fd-leak-on-setnonblock-fail branch April 13, 2021 09:35
This was referenced Apr 28, 2021
Closed
Merged
@brb brb mentioned this pull request Apr 28, 2021
Merged
@brb brb mentioned this pull request Apr 28, 2021
Merged
This was referenced May 13, 2021
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jibi jibi jibi approved these changes

@brb brb brb approved these changes

Assignees

@jibi jibi

Labels
ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact.
Projects
No open projects
1.10.0
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@tklauser @jibi @brb @qmonnet