Merge pull request #31951 from m1093782566/m109-master-errs

Automatic merge from submit-queue

Fix errors.NewAggregate nil pointer panic

<!--  Thanks for sending a pull request!  Here are some tips for you:
1. If this is your first time, read our contributor guidelines https://github.com/kubernetes/kubernetes/blob/master/CONTRIBUTING.md and developer guide https://github.com/kubernetes/kubernetes/blob/master/docs/devel/development.md
2. If you want *faster* PR reviews, read how: https://github.com/kubernetes/kubernetes/blob/master/docs/devel/faster_reviews.md
3. Follow the instructions for writing a release note: https://github.com/kubernetes/kubernetes/blob/master/docs/devel/pull-requests.md#release-notes
-->

**What this PR does / why we need it**:

Consider the following code block,

```
err := myfunc()
agg := errors.NewAggregate([]error{err})
fmt.Println("aggregate error is %v", agg)
```

If the `err` is **nil**, then it will cause a panic:

```
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
        panic: runtime error: invalid memory address or nil pointer dereference
[signal 0xc0000005 code=0x0 addr=0x20 pc=0x481a61]

goroutine 6 [running]:
panic(0x5769c0, 0xc082002090)
        C:/Go/src/runtime/panic.go:464 +0x3f4
testing.tRunner.func1(0xc082074090)
        C:/Go/src/testing/testing.go:467 +0x199
panic(0x5769c0, 0xc082002090)
        C:/Go/src/runtime/panic.go:426 +0x4f7
k8s.io/kubernetes/pkg/util/errors.aggregate.Error(0xc082002590, 0x1, 0x1, 0x0, 0x0)
        D:/goproject/community/src/k8s.io/kubernetes/pkg/util/errors/errors.go:71 +0x91
k8s.io/kubernetes/pkg/util/errors.(*aggregate).Error(0xc08200c7e0, 0x0, 0x0)
        <autogenerated>:1 +0xb4
k8s.io/kubernetes/pkg/util/errors.TestAggregateWithNil(0xc082074090)
        D:/goproject/community/src/k8s.io/kubernetes/pkg/util/errors/errors_test.go:70 +0x402
testing.tRunner(0xc082074090, 0x67cdb8)
        C:/Go/src/testing/testing.go:473 +0x9f
created by testing.RunTests
        C:/Go/src/testing/testing.go:582 +0x899
exit status 2
```

The root cause is that [aggregate.Error()](https://github.com/kubernetes/kubernetes/blob/master/pkg/util/errors/errors.go#L47) doesn't check if error list contains nil.

We can blame the user didn't check if `err` is nil before passing it to 

```
errors.NewAggregate([]error{err})
``` 

but I think we can check again inside in case of user forget to check outside.
This commit is contained in:
Kubernetes Submit Queue 2016-09-06 02:01:29 -07:00 committed by GitHub
commit 2fe1fcdab8
2 changed files with 53 additions and 1 deletions

View File

@ -31,11 +31,23 @@ type Aggregate interface {
// NewAggregate converts a slice of errors into an Aggregate interface, which
// is itself an implementation of the error interface. If the slice is empty,
// this returns nil.
// It will check if any of the element of input error list is nil, to avoid
// nil pointer panic when call Error().
func NewAggregate(errlist []error) Aggregate {
if len(errlist) == 0 {
return nil
}
return aggregate(errlist)
// In case of input error list contains nil
var errs []error
for _, e := range errlist {
if e != nil {
errs = append(errs, e)
}
}
if len(errs) == 0 {
return nil
}
return aggregate(errs)
}
// This helper implements the error and Errors interfaces. Keeping it private

View File

@ -50,6 +50,46 @@ func TestEmptyAggregate(t *testing.T) {
}
}
func TestAggregateWithNil(t *testing.T) {
var slice []error
slice = []error{nil}
var agg Aggregate
var err error
agg = NewAggregate(slice)
if agg != nil {
t.Errorf("expected nil, got %#v", agg)
}
err = NewAggregate(slice)
if err != nil {
t.Errorf("expected nil, got %#v", err)
}
// Append a non-nil error
slice = append(slice, fmt.Errorf("err"))
agg = NewAggregate(slice)
if agg == nil {
t.Errorf("expected non-nil")
}
if s := agg.Error(); s != "err" {
t.Errorf("expected 'err', got %q", s)
}
if s := agg.Errors(); len(s) != 1 {
t.Errorf("expected one-element slice, got %#v", s)
}
if s := agg.Errors()[0].Error(); s != "err" {
t.Errorf("expected 'err', got %q", s)
}
err = agg.(error)
if err == nil {
t.Errorf("expected non-nil")
}
if s := err.Error(); s != "err" {
t.Errorf("expected 'err', got %q", s)
}
}
func TestSingularAggregate(t *testing.T) {
var slice []error = []error{fmt.Errorf("err")}
var agg Aggregate