fix: address possible panic due to offset checking in handleInsertErrors

External reporter identified case where improper bounds checking can cause panic when comparing the index value from structured error response. Fixes: googleapis#3519
shollyman · Jan 11, 2021 · 3e94c9a · 3e94c9a
1 parent 78ee29b
commit 3e94c9a
Show file tree

Hide file tree

Showing 2 changed files with 31 additions and 11 deletions.
diff --git a/bigquery/inserter.go b/bigquery/inserter.go
@@ -229,7 +229,7 @@ func handleInsertErrors(ierrs []*bq.TableDataInsertAllResponseInsertErrors, rows
 	}
 	var errs PutMultiError
 	for _, e := range ierrs {
-		if int(e.Index) > len(rows) {
+		if int(e.Index) >= len(rows) {
 			return fmt.Errorf("internal error: unexpected row index: %v", e.Index)
 		}
 		rie := RowInsertionError{

diff --git a/bigquery/inserter_test.go b/bigquery/inserter_test.go
@@ -16,6 +16,7 @@ package bigquery
 
 import (
 	"errors"
+	"fmt"
 	"strconv"
 	"testing"
 
@@ -113,22 +114,27 @@ func TestHandleInsertErrors(t *testing.T) {
 		{InsertId: "b"},
 	}
 	for _, test := range []struct {
-		in   []*bq.TableDataInsertAllResponseInsertErrors
-		want error
+		description string
+		in          []*bq.TableDataInsertAllResponseInsertErrors
+		want        error
 	}{
 		{
-			in:   nil,
-			want: nil,
+			description: "nil errors",
+			in:          nil,
+			want:        nil,
 		},
 		{
-			in:   []*bq.TableDataInsertAllResponseInsertErrors{{Index: 1}},
-			want: PutMultiError{RowInsertionError{InsertID: "b", RowIndex: 1}},
+			description: "single error last row",
+			in:          []*bq.TableDataInsertAllResponseInsertErrors{{Index: 1}},
+			want:        PutMultiError{RowInsertionError{InsertID: "b", RowIndex: 1}},
 		},
 		{
-			in:   []*bq.TableDataInsertAllResponseInsertErrors{{Index: 1}},
-			want: PutMultiError{RowInsertionError{InsertID: "b", RowIndex: 1}},
+			description: "single error first row",
+			in:          []*bq.TableDataInsertAllResponseInsertErrors{{Index: 0}},
+			want:        PutMultiError{RowInsertionError{InsertID: "a", RowIndex: 0}},
 		},
 		{
+			description: "errors with extended message",
 			in: []*bq.TableDataInsertAllResponseInsertErrors{
 				{Errors: []*bq.ErrorProto{{Message: "m0"}}, Index: 0},
 				{Errors: []*bq.ErrorProto{{Message: "m1"}}, Index: 1},
@@ -138,10 +144,24 @@ func TestHandleInsertErrors(t *testing.T) {
 				RowInsertionError{InsertID: "b", RowIndex: 1, Errors: []error{&Error{Message: "m1"}}},
 			},
 		},
+		{
+			description: "invalid index",
+			in: []*bq.TableDataInsertAllResponseInsertErrors{
+				{Errors: []*bq.ErrorProto{{Message: "m0"}}, Index: 2},
+			},
+			want: fmt.Errorf("internal error: unexpected row index: 2"),
+		},
 	} {
 		got := handleInsertErrors(test.in, rows)
-		if !testutil.Equal(got, test.want) {
-			t.Errorf("%#v:\ngot\n%#v\nwant\n%#v", test.in, got, test.want)
+		_, ok := got.(PutMultiError)
+		if ok {
+			if !testutil.Equal(got, test.want) {
+				t.Errorf("(case: %s)\nin %#v\ngot\n%#v\nwant\n%#v", test.description, test.in, got, test.want)
+			}
+		} else {
+			if got != nil && got.Error() != test.want.Error() {
+				t.Errorf("(case: %s)\nin %#v:\ngot\n%#v\nwant\n%#v", test.description, test.in, got, test.want)
+			}
 		}
 	}
 }