Applogy: Fix Shared Memory Overwriting

[guscarreon]

Copy of reference field Banner needed to be made inside adapters/applogy/applogy.go in order to avoid a data race on shared memory. Authors of source code should be notified and encourage to make any changes to this PR if they disagree with the proposed approach.

[SyntaxNode]

Please email the Applogy through their bidder info email address to bring this change to their attention.

[SyntaxNode]

Why did the test case change?

[SyntaxNode]

You should move the banner copy and reference inside the if banner.W == nil || banner.H == nil || *banner.W == 0 || *banner.H == 0 { statement, because otherwise the copy isn't necessary.

[hhhjort]

Also, impression is a copy of an element in the Imp slice. The assignment modifies the the Banner pointer of the copy, not the actual impression within the Imp array. The Imp slice is already a shallow copy of the original request, so it is safe to modify the Banner pointer in one of the elements.

[SyntaxNode]

Ah, that's a good point @hhhjort. It shouldn't have failed the new memory tests then though?

[guscarreon]

The assignment modifies the the Banner pointer of the copy but, isn't the Banner pointer of that shallow copy pointing to the exact same space in memory than the original element in the Imp slice?

Given that the Banner field is a reference type, the Imp shallow copy preserves the exact same hexadecimal memory address if I'm not mistaken. In other words, the space in memory where request.Imp[0].Banner
and impression.Banner point to, is the same. What do you think? Do you think this is a false positive?

(dlv) list
> github.com/prebid/prebid-server/adapters/applogy.(*ApplogyAdapter).MakeRequests() ./applogy.go:36 (hits goroutine(20):1 total:1) (PC: 0x133378f)
    31:                                 Message: "Applogy only supports banner, video or native ads",
    32:                         })
    33:                         continue
    34:                 }
    35:                 if impression.Banner != nil {
=>  36:                         banner := impression.Banner
    37:                         if banner.W == nil || banner.H == nil || *banner.W == 0 || *banner.H == 0 {
    38:                                 if len(banner.Format) == 0 {
    39:                                         errs = append(errs, &errortypes.BadInput{
    40:                                                 Message: "banner size information missing",
    41:                                         })
(dlv) p request.Imp[0].Banner==impression.Banner
true

[hhhjort]

It doesn't modify the existing Banner (which would be bad as that is pointing to the shared original request Banner). In line 36 (banner := *impression.Banner), you make banner a local shallow copy of the original Banner. You then modify that local copy. Line 48 you assign impression.Banner, where impression is the loop copy of the actual element in Imp to point to the new local copy of banner. So the temporary impression imp object gets it's Banner pointer assigned to a new, local banner object, and the object in the Imp slice that we are iterating over is unchanged. Thus when the loop ends, request.Imp[n].Banner is unchanged.

[guscarreon]

I committed an update, please let me know if we got it right this time

[guscarreon]

@IncSW,
We recently coded pull request #1756 which 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.

[SyntaxNode]

I think this the best approach for this adapter. Based on the request.Imp = impressions line later in this adapter, it seems their intent is to preserve the bid request. Any other approach would not accomplish that goal.

[IncSW]

@guscarreon,
I'm agree with these changes.

[hhhjort]

LGTM