benjaminch / pricers

This library supports RTB development for Open RTB common price encryption in Golang.
GNU General Public License v3.0
8 stars 4 forks source link

Concurrency Bug #14

Closed stokito closed 1 year ago

stokito commented 2 years ago

I have a global var pricer *doubleclick.DoubleClickPricer and many requests are using the same pricer to decrypt a price. And I saw some strange results and today finally saw three panics with the following stack:

Error: d.nx != 0
crypto/sha1.(*digest).checkSum(0xc0000f0790)
crypto/sha1/sha1.go:183 +0x16d
crypto/sha1.(*digest).Sum(0x8?, {0x0, 0x0, 0x0})
crypto/sha1/sha1.go:162 +0x7d
crypto/hmac.(*hmac).Sum(0xc000914060, {0x0?, 0x0, 0xc0010a3c01?})
crypto/hmac/hmac.go:58 +0x38
github.com/benjaminch/pricers/helpers.HmacSum({0xc1dc20, 0xc000914060}, {0xc00090e7e0, 0x8, 0x8}, {0xc00090e7d0, 0x10, 0x10})
github.com/benjaminch/pricers@v0.2.2-0.20220905152655-c79300e30332/helpers/helpers.go:87 +0xa3
github.com/benjaminch/pricers/doubleclick.(*DoubleClickPricer).Decrypt(0xc0009140c0, {0xc0008292c0?, 0x26?})
github.com/benjaminch/pricers@v0.2.2-0.20220905152655-c79300e30332/doubleclick/doubleclick_pricer.go:174 +0x6c5

The auction price string was totally correct and I decoded it successfully when tried locally.

So from what I see the panic is thrown from here

func HmacSum(hmac hash.Hash, buf, buf2 []byte) []byte {
    hmac.Reset()
    hmac.Write(buf)
}

And the hmac may be dc.integrityKey or dc.encryptionKey fields. But here their value is changing! That's why two concurrent requests may erase each other values. I don't know how to fix this properly. Just for now I added a lock to avoid concurrent execution. But I'm afraid that I have to create a new instance of the pricer each time. Could you please confirm and suggest any possible fix for this? For example we may copy the i_key and e_key before the hmac calculation.

stokito commented 1 year ago

I checked and creation of a hasher would create too many allocations. So we should reuse it. Since we can't make the pricer thread safe we need to create a new instance per request. It's not that bad because we can use sync.Pool() but still the pricer has a lot of fields that are simply not needed. I'll send a PR to remove them