BSD mountinfo implementation is unsound

[cyphar]

There are two problems:

• According to the documentation of reflect.SliceHeader it's never safe to modify or otherwise make use of the contents of SliceHeader:

  SliceHeader is the runtime representation of a slice. It cannot be used safely or portably and its representation may change in a later release. Moreover, the Data field is not sufficient to guarantee the data it references will not be garbage collected, so programs must keep a separate, correctly typed pointer to the underlying data.

  I'm sure that this is more of a CYA statement than anything else, but it does mean that technically our usage of this is unsound -- and ultimately the fix is just to switch to a C-style loop over the pointers.

• getmntinfo modifies a global variable, which means that if multiple goroutines try to get mountinfo at the same time we will end up potentially modifying the global structure during iteration. We could work around this by mutexing it or something, but a simpler solution would be to just use getfsstat(2) which allows us to pass our own allocated array.

[kolyshkin]

Related: #24 (one other issue is freebsd implementation requires cgo)

We need someone who works with FreeBSD to fix this, and we need some CI to test it on.

[kolyshkin]

Related to using reflect.SliceHeader: https://github.com/moby/moby/pull/41626

[cuonglm]

@cyphar Would you elaborating more? I don't see problem with current usage of reflect.SliceHeader in https://github.com/moby/sys/blob/1bc8673b57550ddf85262eb0fed0aac651a37dab/mountinfo/mountinfo_bsd.go#L28

[cyphar]

I'm not saying it doesn't work, the issue is that reflect.SliceHeader is explicitly described in the documentation as being fundamentally unsound to use. There are other issues with it (namely the GC doesn't know how to track the pointer and there are cases where you could get use-after-frees) but because getmntinfo returns a static pointer in memory, this isn't as much of a concern (though I do wonder if there are cases where Go would decide to try to GC the head of a SliceHeader...).

[cuonglm]

I'm not saying it doesn't work, the issue is that reflect.SliceHeader is explicitly described in the documentation as being fundamentally unsound to use. There are other issues with it (namely the GC doesn't know how to track the pointer and there are cases where you could get use-after-frees) but because getmntinfo returns a static pointer in memory, this isn't as much of a concern (though I do wonder if there are cases where Go would decide to try to GC the head of a SliceHeader...).

I dont think it can be the issue if you did use reflect.SliceHeader correctly. I invite you to read https://github.com/golang/go/issues/41705#issuecomment-701706990

[cyphar]

Fair enough, I was going by the literal reading of the documentation (and I've been bitten by Go's runtime far too many times to be optimistic that its behaviour will be nice). But if the Go authors say the example is valid then there's only one correctness issue in the BSD implementation.