Skip to content

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

@lmb

Description

@lmb

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.

Metadata

Metadata

Assignees

No one assigned

    Labels

    enhancementNew feature or request

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions