Use of unsafe is unsound

[Stebalien]

Casting to a uintptr, saving the uintptr, and then casting it back is unsound.

bbloom/bbloom.go

Lines 80 to 88 in e2d15f3

	func NewWithBoolset(bs *[]byte, locs uint64) (bloomfilter Bloom) {
	bloomfilter = New(float64(len(*bs)<<3), float64(locs))
	ptr := uintptr(unsafe.Pointer(&bloomfilter.bitset[0]))
	for _, b := range *bs {
	*(*uint8)(unsafe.Pointer(ptr)) = b
	ptr++
	}
	return bloomfilter
	}

bbloom/bbloom.go

Lines 232 to 246 in e2d15f3

	func (bl Bloom) JSONMarshal() []byte {
	bloomImEx := bloomJSONImExport{}
	bloomImEx.SetLocs = uint64(bl.setLocs)
	bloomImEx.FilterSet = make([]byte, len(bl.bitset)<<3)
	ptr := uintptr(unsafe.Pointer(&bl.bitset[0]))
	for i := range bloomImEx.FilterSet {
	bloomImEx.FilterSet[i] = *(*byte)(unsafe.Pointer(ptr))
	ptr++
	}
	data, err := json.Marshal(bloomImEx)
	if err != nil {
	log.Fatal("json.Marshal failed: ", err)
	}
	return data
	}

[AndreasBriese]

What do you mean by unsound? Pls give a working code example that does not use the unsafe pointer. Andreas

…

Am 23.08.2019 um 19:14 schrieb Steven Allen ***@***.***>: Casting to a uintptr, saving the uintptr, and then casting it back is unsound. https://github.com/AndreasBriese/bbloom/blob/e2d15f34fcf99d5dbb871c820ec73f710fca9815/bbloom.go#L80-L88 <https://github.com/AndreasBriese/bbloom/blob/e2d15f34fcf99d5dbb871c820ec73f710fca9815/bbloom.go#L80-L88> https://github.com/AndreasBriese/bbloom/blob/e2d15f34fcf99d5dbb871c820ec73f710fca9815/bbloom.go#L232-L246 <https://github.com/AndreasBriese/bbloom/blob/e2d15f34fcf99d5dbb871c820ec73f710fca9815/bbloom.go#L232-L246> — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#9?email_source=notifications&email_token=AAKKFC33XGDZQ2YKYIIVJ4TQGALJBA5CNFSM4IPBVSPKYY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4HHC36OQ>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAKKFC2HSS3C3QWDD674VD3QGALJBANCNFSM4IPBVSPA>.

[Stebalien]

I'm saying that converting a uintptr back to a unsafe.Pointer across multiple statements is unsound.

https://golang.org/pkg/unsafe/#Pointer

[AndreasBriese]

Hey Steve, the ptr is not carried around multiple statements. the ptr is locally casted, incremented and changed by a bitoperation. This behaviour is not volatile at runtime - at least not in the golang binary. Anyway - when bbloom was written in 2015 i guess the math/bits package was not part of the standard library (came 2017/18 ? from the guys working on math64 and integer ssa ... implementation?). Maybe it is a good idea to go for the for the present standard library and following your suggestion. I will look into it and will replace unsafe use, where it is suitable without performance loss. If you have changed the API maybe you should remaster your code with a different name i.e bbbloom ? Andreas

…

Am 23.08.2019 um 22:34 schrieb Steven Allen ***@***.***>: I'm saying that converting a uintptr back to a unsafe.Pointer across multiple statements is unsound. https://golang.org/pkg/unsafe/#Pointer <https://golang.org/pkg/unsafe/#Pointer> — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#9?email_source=notifications&email_token=AAKKFCYMH4K4CKC3W3JTT2DQGBCWTA5CNFSM4IPBVSPKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD5BILTY#issuecomment-524453327>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAKKFC3HWWKVMFOFEVECYGLQGBCWTANCNFSM4IPBVSPA>.

[Stebalien]

the ptr is not carried around multiple statements. the ptr is locally casted, incremented and changed by a bitoperation. This behaviour is not volatile at runtime - at least not in the golang binary.

Take a look at the functions I'm quoting in the issue description. You can't save a uintptr to the stack then convert it back to an unsafe.Pointer later.

Take the JSONMarshal function. The garbage collector is allowed to pause the function between lines 236 and 238, move anything it wants, update the pointers, then restart the function. At that point, the saved uintptr could be pointing at anything.

[AndreasBriese]

... p.s. look at (3) here https://golang.org/pkg/unsafe/#Pointer <https://golang.org/pkg/unsafe/#Pointer>

…

Am 23.08.2019 um 22:34 schrieb Steven Allen ***@***.***>: I'm saying that converting a uintptr back to a unsafe.Pointer across multiple statements is unsound. https://golang.org/pkg/unsafe/#Pointer <https://golang.org/pkg/unsafe/#Pointer> — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#9?email_source=notifications&email_token=AAKKFCYMH4K4CKC3W3JTT2DQGBCWTA5CNFSM4IPBVSPKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD5BILTY#issuecomment-524453327>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAKKFC3HWWKVMFOFEVECYGLQGBCWTANCNFSM4IPBVSPA>.

[Stebalien]

That's exactly what I'm looking at:

Note that both conversions must appear in the same expression, with only the intervening arithmetic between them:

// INVALID: uintptr cannot be stored in variable
// before conversion back to Pointer.
u := uintptr(p)
p = unsafe.Pointer(u + offset)

[AndreasBriese]

I considered Your comments and opted for the way documented at https://golang.org/pkg/unsafe/#Pointer
in the latest push.
5f4d56d#diff-eebbf48a54279308068f73108bc90f16
Thx for discussing this thoroughly. That should solve the issues you pointed at.
Andreas

[Stebalien]

That looks safe. Thanks!