Skip to content

pin: WalkDir makes it too easy to leak objects #1652

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
Closed
lmb opened this issue Jan 21, 2025 · 1 comment · Fixed by #1736
Closed

pin: WalkDir makes it too easy to leak objects #1652

lmb opened this issue Jan 21, 2025 · 1 comment · Fixed by #1736
Labels
enhancement New feature or request

Comments

@lmb
Copy link
Collaborator

lmb commented Jan 21, 2025 •
edited
Loading

It's very easy to accidentally leak objects when using WalkDir, and kind of awkward to do the right thing (because obj can be nil?) This is why https://pkg.go.dev/github.com/cilium/ebpf/btf#HandleIterator.Take exists for handles. It might be good to do another pass over this API.

Originally posted by @lmb in #1651 (comment)

More context from Mahe:

Mmmh I'm slightly confused now, this it the typical use we have in Tetragon:

https://github.com/cilium/tetragon/blob/d72f98aefb8e88c06caec7cfbc687bbeabf97a5f/cmd/tetra/debug/progs.go#L408-L445
https://github.com/cilium/tetragon/blob/d72f98aefb8e88c06caec7cfbc687bbeabf97a5f/pkg/bugtool/maps.go#L148-L176

Maybe having an example in the doc would make thing more clear?

As far as I can tell both of these examples leak objects. Not a big deal in these cases since the processes are short lived (I think?) but an indication that the API is bug prone. My gut feeling is that this is because of wrapping WalkDir which in itself is a really clunky API.

Ideas:

  1. Drop Walk outright and have callers do the call to Load themselves. That makes it clearer that they are reponsible for cleaning up.
  2. Only invoke Walk for valid objects so that an unconditional defer obj.Close() at the top of the function works.

Since 1.24 is around the corner it might also be interesting to consider what it would look like to turn this API into an iterator.
@lmb lmb changed the title It's very easy to accidentally leak objects when using WalkDir, and kind of awkward to do the right thing (because obj can be nil?) This is why https://pkg.go.dev/github.com/cilium/ebpf/btf#HandleIterator.Take exists for handles. It might be good to do another pass over this API. pin: WalkDir makes it too easy to leak objects Jan 21, 2025
@lmb lmb added bug Something isn't working enhancement New feature or request and removed bug Something isn't working labels Jan 21, 2025
@lmb lmb mentioned this issue Jan 21, 2025
Merged
@ti-mo
Copy link
Collaborator

ti-mo commented Jan 27, 2025 •
edited
Loading

As far as I can tell, Go iterators are also callback-based and would require something similar to HandleIterator.Take if we decide to always Close() under the covers.

Drop Walk outright and have callers do the call to Load themselves. That makes it clearer that they are reponsible for cleaning up.

The API aims to unburden the caller from this exact pain. The library already does the (runtime) work of opening the pin and determining its type. Closing it and requiring the caller to repeat the same work feels very inefficient; doubly so if these are short-lived objects.

I think this is a documentation issue; this isn't much different from any other omission of Close(). I'm of course open to other API forms if there are better suggestions, but iirc Go iterators lack the feedback mechanism that WalkDir provides, e.g. skipping directories to avoid unnecessary tree visits.

Haven't really heard about people taking issue with filepath.WalkDir(), I think it's just okay personally.

I think a pin.Iterator would be ideal, that would give us the most flexibility.

Only invoke Walk for valid objects so that an unconditional defer obj.Close() at the top of the function works.

Not sure what you mean by invalid objects, but there's a stat() call at the beginning that makes sure it's called on a bpffs instance. All nodes visited by WalkDir are valid bpf objects, currently meaning pin, prog or map. btf can't be pinned (yet).

lmb added a commit to lmb/ebpf that referenced this issue Mar 31, 2025
@lmb
pin: make WalkDir harder to misuse and add Windows support
abb27a8 
The current WalkDir API requires the user to close each object that
is iterated over. This isn't very intuitive, the two uses of the
function in the tetragon code base end up leaking objects.
It's also not straight forward to implement WalkDir on Windows, since
BPFFS isn't really a thing on that platform.

Instead, turn WalkDir into an iterator which returns a new Pin object.
By default, objects are closed once they have been iterated over, the
user is expected to clone them if necessary.

One downside is that directories are not returned by the iteration
anymore. The existing code in tetragon doesn't use them and is
therefore not affected. There is also no more fine grained control
over whether to descent into a directory. Aborting an iteration is
still supported and as efficient as before.

Fixes: cilium#1652
Signed-off-by: Lorenz Bauer <[email protected]>
lmb added a commit to lmb/ebpf that referenced this issue Mar 31, 2025
@lmb
pin: make WalkDir harder to misuse and add Windows support
ef68628 
The current WalkDir API requires the user to close each object that
is iterated over. This isn't very intuitive, the two uses of the
function in the tetragon code base end up leaking objects.
It's also not straight forward to implement WalkDir on Windows, since
BPFFS isn't really a thing on that platform.

Instead, turn WalkDir into an iterator which returns a new Pin object.
By default, objects are closed once they have been iterated over. The
user may call Pin.Take if they wish to retain the object. This is
similar to the existing link.Iterator.Take.

One downside is that directories are not returned by the iteration
anymore. The existing code in tetragon doesn't use them and is
therefore not affected. There is also no more fine grained control
over whether to descent into a directory. Aborting an iteration is
still supported and as efficient as before.

Fixes: cilium#1652
Signed-off-by: Lorenz Bauer <[email protected]>
@lmb lmb mentioned this issue Mar 31, 2025
Merged
lmb added a commit to lmb/ebpf that referenced this issue Apr 14, 2025
@lmb
pin: make WalkDir harder to misuse and add Windows support
218443a 
The current WalkDir API requires the user to close each object that
is iterated over. This isn't very intuitive, the two uses of the
function in the tetragon code base end up leaking objects.
It's also not straight forward to implement WalkDir on Windows, since
BPFFS isn't really a thing on that platform.

Instead, turn WalkDir into an iterator which returns a new Pin object.
By default, objects are closed once they have been iterated over. The
user may call Pin.Take if they wish to retain the object. This is
similar to the existing link.Iterator.Take.

One downside is that directories are not returned by the iteration
anymore. The existing code in tetragon doesn't use them and is
therefore not affected. There is also no more fine grained control
over whether to descent into a directory. Aborting an iteration is
still supported and as efficient as before.

Fixes: cilium#1652
Signed-off-by: Lorenz Bauer <[email protected]>
@lmb lmb closed this as completed in #1736 Apr 16, 2025
@lmb lmb closed this as completed in ae10986 Apr 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

pin: make WalkDir harder to misuse and add Windows support lmb/ebpf
2 participants
@lmb @ti-mo