Panics happen when left with zero length []Imp #1462

guscarreon · 2020-08-27T01:44:38Z

Sometimes an openrtb.BidRequest's []Imp slice gets completely emptied after filtering out imps with wrong media types in adapters/info.go as defined in the bidder's static/bidder-info/*.yaml file. This pull request modifies the InfoAwareBidder's filtering process to not send requests with an empty Imp slice to any adapter to avoid panics in case the bidder's adapter de-references an slice element.

Test request:

{
	"id": "some-request-id",
	"site": {
		"page": "prebid.org"
	},
	"imp": [{
		"id": "some-impression-id",
		"video": {
			"mimes": ["video/mp4"],
			"w": 640,
			"h": 480
		},
		"ext": {
			"33across": {
				"siteId": "fake-site-id",
				"productId": "inview"
			}
		}
	}],
	"tmax": 100000
}

Test request output before this PR's changes:
Client

╰─➤  curl --data '{"id":"some-request-id","site":{"page":"prebid.org"},"imp":[{"id":"some-impression-id","video":{"mimes":["video/mp4"],"w":640,"h":480},"ext":{"33across":{"siteId":"fake-site-id","productId":"inview"}}}],"tmax":100000}' http://localhost:8000/openrtb2/auction
{"id":"some-request-id","ext":{"responsetimemillis":{"":0},"tmaxrequest":100000}}

Server

╰─➤  ./prebid-server
E0826 21:23:42.755239   29799 exchange.go:429] OpenRTB auction recovered panic from Bidder 33across: runtime error: index out of range [0] with length 0. Account id: unknown, All Bidders: 33across, Stack trace is: goroutine 172 [running]:
runtime/debug.Stack(0xc0005e3118, 0xc00048e5b8, 0x8)
        /usr/local/opt/go/libexec/src/runtime/debug/stack.go:24 +0x9d
github.com/prebid/prebid-server/exchange.(*exchange).recoverSafely.func1.1(0xc00061d5c0, 0xc00048e590, 0x8, 0xc0005a0380, 0xc00062ff00, 0xc00040c3c0)
        /Users/gcarreongutierrez/go/src/github.com/prebid/prebid-server/exchange/exchange.go:431 +0x319
panic(0x18adae0, 0xc0003268e0)
        /usr/local/opt/go/libexec/src/runtime/panic.go:969 +0x166
github.com/prebid/prebid-server/adapters/33across.preprocess(0xc00007d548, 0x10e2733, 0xc00098a0d0)
        /Users/gcarreongutierrez/go/src/github.com/prebid/prebid-server/adapters/33across/33across.go:72 +0x446
github.com/prebid/prebid-server/adapters/33across.(*TtxAdapter).makeRequest(0xc00062a560, 0xc0008c6280, 0xc00007d840, 0x104de6e, 0x101644a, 0xc0002f9570)
        /Users/gcarreongutierrez/go/src/github.com/prebid/prebid-server/adapters/33across/33across.go:48 +0x77
github.com/prebid/prebid-server/adapters/33across.(*TtxAdapter).MakeRequests(0xc00062a560, 0xc0008c6280, 0xc0009f7300, 0x1, 0xc0009f7330, 0x1, 0x1, 0x1, 0xc0009f7330)
        /Users/gcarreongutierrez/go/src/github.com/prebid/prebid-server/adapters/33across/33across.go:32 +0x3f
github.com/prebid/prebid-server/adapters.(*InfoAwareBidder).MakeRequests(0xc00032ade0, 0xc0008c6280, 0xc0009f7300, 0xc00007d9a8, 0x1692386, 0xc000913790, 0x1a6ebe0, 0xc0006a2000, 0xc0007b2000)
        /Users/gcarreongutierrez/go/src/github.com/prebid/prebid-server/adapters/info.go:64 +0x12d
github.com/prebid/prebid-server/exchange.(*bidderAdapter).requestBid(0xc0006aaa50, 0x1a71a60, 0xc00040c240, 0xc0008c6280, 0xc00048e590, 0x8, 0x3ff0000000000000, 0x1a68860, 0xc00019c0e0, 0xc0009f7300, ...)
        /Users/gcarreongutierrez/go/src/github.com/prebid/prebid-server/exchange/bidder.go:118 +0x65
github.com/prebid/prebid-server/exchange.(*validatedBidder).requestBid(0xc00062bc60, 0x1a71a60, 0xc00040c240, 0xc0008c6280, 0xc00048e590, 0x8, 0x3ff0000000000000, 0x1a68860, 0xc00019c0e0, 0xc0009f7300, ...)
        /Users/gcarreongutierrez/go/src/github.com/prebid/prebid-server/exchange/bidder_validate_bids.go:32 +0xbf
github.com/prebid/prebid-server/exchange.(*exchange).getAllBids.func1(0xc00048e590, 0x8, 0xc00048e590, 0x8, 0xc0008c6280, 0xc0005a0380, 0x1a68860, 0xc00019c0e0)
        /Users/gcarreongutierrez/go/src/github.com/prebid/prebid-server/exchange/exchange.go:364 +0x29c
github.com/prebid/prebid-server/exchange.(*exchange).recoverSafely.func1(0xc00048e590, 0x8, 0xc00048e590, 0x8, 0xc0008c6280, 0xc0005a0380, 0x1a68860, 0xc00019c0e0)
        /Users/gcarreongutierrez/go/src/github.com/prebid/prebid-server/exchange/exchange.go:439 +0xf6
created by github.com/prebid/prebid-server/exchange.(*exchange).getAllBids
        /Users/gcarreongutierrez/go/src/github.com/prebid/prebid-server/exchange/exchange.go:393 +0x20d

After this changes
Client

╰─➤  curl --data '{"id":"some-request-id","site":{"page":"prebid.org"},"imp":[{"id":"some-impression-id","video":{"mimes":["video/mp4"],"w":640,"h":480},"ext":{"33across":{"siteId":"fake-site-id","productId":"inview"}}}],"tmax":100000}' http://localhost:8000/openrtb2/auction
{"id":"some-request-id","ext":{"errors":{"33across":[{"code":2,"message":"request.imp[0] uses video, but this bidder doesn't support it"},{"code":2,"message":"Bid request didn't contain media types supported by bidder"}]},"responsetimemillis":{"33across":0},"tmaxrequest":100000}}

Server

╰─➤  ./prebid-server

guscarreon · 2020-08-27T01:49:10Z

adapters/info.go

+
+	if len(request.Imp) == 0 {
+		return nil, []error{BadInput("Empty Imp array in bid request")}
+	}


The true branch of this if statement is unreachable because request validation in endpoints/openrtb2/*auction.go discards requests with empty []Imp. Nevertheless, I believe having it in place makes for a more robust function at O(1) cost. What do you think?

This info bidder is a special case. (.. and kind of gross? when wouldn't we have an info aware bidder? why not just build this into the core?.. all questions for another time).

If this code is unreachable, I vote to remove it.

Corrected it so we can exit with an error without the need of this unreachable code. Let me know what you think of the new logic.

Removed

SyntaxNode · 2020-08-27T05:27:05Z

adapters/info.go

+
+	if len(request.Imp) == 0 {
+		return nil, []error{BadInput("Empty Imp array in bid request")}
+	}


This info bidder is a special case. (.. and kind of gross? when wouldn't we have an info aware bidder? why not just build this into the core?.. all questions for another time).

If this code is unreachable, I vote to remove it.

SyntaxNode · 2020-08-27T05:28:58Z

adapters/info.go

@@ -57,6 +62,11 @@ func (i *InfoAwareBidder) MakeRequests(request *openrtb.BidRequest, reqInfo *Ext
 	// see if any imps need to be removed, and another to do the removing if necessary.
 	numToFilter, errs := i.pruneImps(request.Imp, allowedMediaTypes)
 	if numToFilter != 0 {
+		// If no valid imps left, avoid expensive filtering process


Can you please reword this comment? It's not avoiding filtering, but rather exiting early, right? We weren't generating an error before this PR.

SyntaxNode · 2020-08-27T05:29:39Z

adapters/info_test.go

@@ -56,6 +58,7 @@ func TestSiteNotSupported(t *testing.T) {
 }

 func TestImpFiltering(t *testing.T) {
+	// Set up


Nitpick: The comments don't seem helpful here. Please consider removing.

SyntaxNode · 2020-08-27T05:30:38Z

adapters/info_test.go

-				Video: &openrtb.Video{},
+
+	// Define test cases
+	type testOut struct {


Does this really have to be a separate struct? Why not add expectedErrors and expectedImps to the testCases struct?

guscarreon · 2020-08-27T16:24:18Z

adapters/info_test.go

+		}
+
+		// Extra MakeRequests() call check: our mockBidder returns an adapter request for every imp
+		assert.Equal(t, test.expectedImpLen, len(actualAdapterRequests), "Test failed. Incorrect lenght of filtered imps: %s", test.description)


@SyntaxNode let me know if you agree with this "extra" check. Strictly speaking, we don't need it. Do you want me to remove it?

I think it's fine to keep. There's a few test cases with expected imps. Consider using assert.Len though.

SyntaxNode · 2020-08-27T16:38:37Z

adapters/info.go

+
+	// If all imps in bid request come with unsupported media types, exit
+	if numToFilter == len(request.Imp) {
+		return nil, append(errs, BadInput("Bid request didn't contain media types supported by bidder"))


Nitpick: supported by the bidder

SyntaxNode · 2020-08-27T16:40:30Z

adapters/info_test.go

+		}
+
+		// Extra MakeRequests() call check: our mockBidder returns an adapter request for every imp
+		assert.Equal(t, test.expectedImpLen, len(actualAdapterRequests), "Test failed. Incorrect lenght of filtered imps: %s", test.description)


I think it's fine to keep. There's a few test cases with expected imps. Consider using assert.Len though.

First draft

eac0201

guscarreon commented Aug 27, 2020

View reviewed changes

SyntaxNode changed the title ~~Panics happen when left with zero lenght []Imp~~ Panics happen when left with zero length []Imp Aug 27, 2020

SyntaxNode reviewed Aug 27, 2020

View reviewed changes

SyntaxNode requested a review from mansinahar August 27, 2020 05:31

SyntaxNode assigned SyntaxNode and mansinahar Aug 27, 2020

Feedback corrections, implicit empty imp check

cf54599

guscarreon commented Aug 27, 2020

View reviewed changes

SyntaxNode reviewed Aug 27, 2020

View reviewed changes

Error message correction and assert.Len

d0429a1

SyntaxNode approved these changes Aug 27, 2020

View reviewed changes

mansinahar approved these changes Aug 27, 2020

View reviewed changes

mansinahar merged commit 1f87497 into prebid:master Aug 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Panics happen when left with zero length []Imp #1462

Panics happen when left with zero length []Imp #1462

guscarreon commented Aug 27, 2020

guscarreon Aug 27, 2020 •

edited

Loading

SyntaxNode Aug 27, 2020

guscarreon Aug 27, 2020

SyntaxNode Aug 27, 2020

SyntaxNode Aug 27, 2020

guscarreon Aug 27, 2020

SyntaxNode Aug 27, 2020

guscarreon Aug 27, 2020

SyntaxNode Aug 27, 2020

guscarreon Aug 27, 2020

guscarreon Aug 27, 2020

SyntaxNode Aug 27, 2020

SyntaxNode Aug 27, 2020

SyntaxNode Aug 27, 2020

Panics happen when left with zero length []Imp #1462

Panics happen when left with zero length []Imp #1462

Conversation

guscarreon commented Aug 27, 2020

guscarreon Aug 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guscarreon Aug 27, 2020 •

edited

Loading