lunixbochs / struc

Better binary packing for Go
MIT License
564 stars 42 forks source link

fixed race in Pack() on default options initialization. #67

Closed serejkus closed 4 years ago

serejkus commented 4 years ago

Default Options used in Pack() is a global object with zero-initialized fields. Validate() changes PtrSize field value on the first call from zero to 32. It is a data race if multiple goroutines call Pack().

This commit fixes this issue and adds a tests its correctnes. Important note about test is that it should be in a separate package because it needs to be run exclusively so that it is guaranteed that it is run first to catch this race condition.

lunixbochs commented 4 years ago

I don’t believe there’s actually a data race here on any platforms Go supports, because all racing threads should write, then read, the same value regardless of cache and instruction ordering. So this patch just fixes “the data race detector incorrectly identifies multiple threads making the same constant assignment as a race”.

Instead of referencing the default value in multiple places, we can just put an init func right below defaultOptions that calls Validate() on it at import time

serejkus commented 4 years ago

“the data race detector incorrectly identifies multiple threads making the same constant assignment as a race”. Exactly. Binaries with race detector enabled report this as an issue sometimes.

serejkus commented 4 years ago

Do you have any clues on what's happening in Go 1.2 build in travis?

lunixbochs commented 4 years ago

It says “I don't have any idea what to do with '1.2'.” We can just drop the Go 1.2 test, probably add a Go 1.13 test.

serejkus commented 4 years ago

By drop you mean build tag !go1.2 in test or dropping 1.2 from travis?

чт, 12 сент. 2019 г., 1:37 Ryan Hileman notifications@github.com:

It says “I don't have any idea what to do with '1.2'.” We can just drop the Go 1.2 test, probably add a Go 1.13 test.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/lunixbochs/struc/pull/67?email_source=notifications&email_token=AAPURHQ7GW5QXMRR5TFOSMDQJFXLHA5CNFSM4IVZGAJ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD6QDJQY#issuecomment-530592963, or mute the thread https://github.com/notifications/unsubscribe-auth/AAPURHUWPY5D4GU4EHCNBRTQJFXLHANCNFSM4IVZGAJQ .

lunixbochs commented 4 years ago

I just mean editing this file: https://github.com/lunixbochs/struc/blob/master/.travis.yml#L7

serejkus commented 4 years ago

Looks like all checks have passed. Any plans on merge?