Race condition in initial struct field parsing

[quantum1423]

The go race detector complains about the initial struct field parsing if two goroutines start packing or unpacking a struct simultaneously before the struct fields are properly parsed. This is usually not an issue, and I've worked around by putting a dummy pack and unpack in init(), but it can be annoying. It's a real race condition: I've had a few extremely rare (as in, once every few hundred runs of all my tests) and difficult-to-reproduce errors (unpacked struct containing garbage) due to this race condition. Now, the garbage occurs only once, but it's still an annoying bug with major security implications (since it violates memory safety).

I would imagine this to be easily fixed by adding a global lock around the struct field parsing.

[lunixbochs]

This will be partially helped by #13 if/when I finish it (fields will be pre-parsed and verified in init() functions, which won't require a lock).

A global lock solution was pushed in 1805db8c18c1bd2551591caa17b22338650e88fb, where a lock globally prevents more than one uncached field set from being parsed at once. Can you confirm if this fixes it?

[quantum1423]

It seems to have fixed it. Thanks!

[quantum1423]

However, after reading the patch it seems like a race is still theoretically possible. It's just going to be even rarer/harder to debug in practice :/

It's the "check, lock then check again" anti-pattern; you can corrupt data in this sequence:

Goroutine 1 checks, and then obtains the lock.
Goroutine 1 begins writing to the fieldCache
Goroutine 2 checks, and sees half-written garbage in fieldCache while doing the check

map read and writes aren't atomic in Go.

Basically, eventually data would be okay, but Goroutine 2 can see garbage and panic/leak secret data out to the world. A map should never be read from when somebody else could be writing to it.

This is a very difficult problem without always acquiring the lock, which would be way too expensive. Fortunately Go's stdlib has sync.Once; you should look into using that. Its implementation is quite subtle, but avoids races or unnecessary performance loss.

[lunixbochs]

sync.Once doesn't apply here, as you can have more than one set of fields to parse.

This hasn't introduced any new failure conditions, as two goroutines with bad timings could already do that :)

The additional assertion you seem to want is that parseFields should be fully thread-safe, which needs a read/write mutex on the cache. I'll implement that.

[quantum1423]

Yeah, I think that it should be thread-safe, since having the first pack/unpack being thread-unsafe and subsequent ones being thread-safe is very surprising behavior.

[lunixbochs]

Can you review 1f112059da835022990ed6e68c645b07cf9848bf then close this issue if it looks good?

Edit: I'm going to modify it slightly as the locking pattern will still allow double-parse.

[quantum1423]

I'm fairly certain parseLock is unnecessary. Multiple goroutines parsing fields at once should be fine: as far as I can see parseFields or parseField does not write to anything shared between goroutines. There is no race condition when multiple goroutines read data simultaneously.

In other words this piece seems to be unnecessary and just a performance drag:

// take a lock so multiple goroutines can't parse fields at once
parseLock.Lock()
defer parseLock.Unlock()
// check a second time, just in case it was parsed by another goroutine during lock
if cached, ok := fieldCache[t]; ok {
    fieldCacheLock.RUnlock()
    return cached, nil
}

[lunixbochs]

I disagree. If we don't hold parseLock and you do have a workload where many of the same struct will be parsed in parallel (which is why you opened this issue), the performance hit of parsing the same struct many times will cause more load than forcing all structs to parse serially. It would be slightly better to only prevent the same type of struct from parsing at once, but then I'd need to track that somewhere and I didn't quickly think of a good data structure for it.

Edit: the read/write lock overlap was fixed in 12f2b39f0606d7ac7c1dadf85fdf545977ac1dad

[quantum1423]

Yes, but it would only cause the load for the first milliseconds where the race is happened. Once the parse data is in the cache, parseLock is just going to add overhead.

My problem was with data corruption / garbage results from pack/unpack, not really the miniscule waste of work. In the long run, removing the lock should give better performance since only a few invocations at most of pack or unpack would incur the extra overhead. In fact, if this were another language I would suggest using thread-local caches!

[lunixbochs]

parseLock adds zero overhead once data is in the cache, because the fast path does not hold it.

[quantum1423]

Ah yes. Sorry for overlooking it. I'm going to close this issue.