33across: Fix Shared Memory Overwriting

[guscarreon]

@curlyblueeagle,
Recently coded pull request #1756 is meant to enable Prebid Server to find data race conditions in adapter bidder source code. According to this newly coded feature, your adapter might be writing to shared memory and this PR implements an approach to prevent it. Please let us know whether or not you agree with the changes proposed in this pull request or if you rather want to code the fix yourself.

When it comes to adapters' source code improvements, we usually wait a week for owners to come back to us. If we get no feedback, we will go ahead to review and merge this pull request ourselves.

We'd love to hear your take on this issue.

[curlyblueeagle]

I am okay with the proposed changes, can you explain where a race condition could potentially occur in the code?

[guscarreon]

I am okay with the proposed changes, can you explain where a race condition could potentially occur in the code?

@curlyblueeagle thank you for setting aside time to review this change. The openrtb.BidRequest parameter that MakeRequests receives is a "shallow" copy. A copy is shallow when the exact values of basic types such as strings and ints where copied from the original to the destination but as far as reference types: new copies of these objects where not created. Therefore, the shallow copy preserves the pointer values of the orginal which are pointing to shared memory that, given the parallel nature of Prebid Server core, might be rewritten by a different thread. Out of the bidRequest fields we have to carefully watch the reference types bidRequest.Site, bidRequest.Imp[i], bidRequest.App, bidRequest.Device, bidRequest.User, bidRequest.Source, and bidRequest.Regs.

type BidRequest struct {
      .
      .
    ID string `json:"id"`  <-- Basic type, can be read and modified without a problem
      .
      .
    Imp []Imp `json:"imp"` <-- Reference type, every `Imp` element can be read BUT NOT modified
      .                        because it points to memory that might be shared with other parallel processes
      .
    Site *Site `json:"site,omitempty"` <-- Reference type, every `Imp` element can be read BUT NOT modified
      .                                    because it points to memory that might be accessed by other parallel processes
      .                                    The only safe way to modify is to create a copy, modify the copy and assign the pointer 
      .                                    to the local pointer value: 
      .                                       siteLocalCopy := *bidRequest.Site //Creates a shallow copy, we can modify its basic types now
      .                                       siteLocalCopy.Name = "anyOtherValue" //Perfectly safe
      .                                       bidRequest.Site = &siteLocalCopy //perfectly safe because the original space in memory pointed
      .                                                                        //by bidRequest.Site only got read and not modified
    Test int8 `json:"test,omitempty"` <-- Basic type, can be read and modified without a problem
      .
      .
    Ext json.RawMessage `json:"ext,omitempty"` <-- Reference type but these fields usually get Unmarshalled which is a read
                                                   operation so usually there's no problem with these 
}

Same goes for reference fields found in bidRequest.Site, bidRequest.Imp[i], bidRequest.App, bidRequest.Device, bidRequest.User, bidRequest.Source, and bidRequest.Regs. That's the reason we needed to create a copy of bidRequest.Imp[i].Video in your adapter. Video is a reference type of the Imp struct.

I hope this clarifies a bit.

[curlyblueeagle]

@guscarreon thanks for the explanation, super helpful! We will definitely bear this mind in future.

[SyntaxNode]

bidRequest.Imp[i]

You don't need to worry about changes to those elements. Like the bid request, this is also a deep copy. This is the only exception within the request object.

We will definitely bear this mind in future.

@guscarreon is adding a separate PR to test for these modifications rather than relying on our code reviews. This was the intention of the race condition tests, but they weren't good enough and will removed.