Repository navigation
BSD mountinfo implementation is unsound #51
Description
There are two problems:
-
According to the documentation of
reflect.SliceHeaderit's never safe to modify or otherwise make use of the contents ofSliceHeader: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.
-
getmntinfomodifies 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 usegetfsstat(2)which allows us to pass our own allocated array.
Activity
kolyshkin commentedon Nov 5, 2020
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 commentedon Nov 6, 2020
Related to using reflect.SliceHeader: moby/moby#41626
cuonglm commentedon Nov 6, 2020
@cyphar Would you elaborating more? I don't see problem with current usage of reflect.SliceHeader in
sys/mountinfo/mountinfo_bsd.go
Line 28 in 1bc8673
cyphar commentedon Nov 7, 2020
I'm not saying it doesn't work, the issue is that
reflect.SliceHeaderis 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 becausegetmntinforeturns 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 aSliceHeader...).cuonglm commentedon Nov 7, 2020
I dont think it can be the issue if you did use reflect.SliceHeader correctly. I invite you to read golang/go#41705 (comment)
cyphar commentedon Jan 18, 2021
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.