Skip to content

Commit

Permalink
Remove unnecessary header field in ErrorResponse (#1066)
Browse files Browse the repository at this point in the history
ErrorResponse.Headers is causing a panic inside golang http package.

The following code can be easily crashed if GetObject API returns an error,
such as ErrSlowDown:

```
  reader, err := s3Client.GetObject("my-bucketname", "my-source-objectname", minio.GetObjectOptions{})
  if err != nil {
    log.Fatalln(err)
  }
  defer reader.Close()

  stat, err := reader.Stat()
  if err != nil {
    log.Fatalln(err)
  }

  n, err := s3Client.PutObject("my-bucketname", "my-target-objectname", reader, stat.Size, minio.PutObjectOptions{})
  if err != nil {
    log.Fatalln(err)
  }
```

The reason is that `reader` is passed to s3Client.PutObject therefore to golang http
via request.Body.  Since, reader.Read() can return an ErrorResponse, that error will
be tested by golang http, but since ErrorResponse is uncomparable, then it will cause
a crash with the following stackstrace:

```
  panic: runtime error: comparing uncomparable type minio.ErrorResponse

  goroutine 20 [running]:
  net/http.(*Request).write(0xc0002c2300, 0x761e60, 0xc000069f80, 0x0, 0x0, 0x0, 0x7628a0, 0xc000518780)
          /home/vadmeste/work/go/src/net/http/request.go:647 +0x74c
  net/http.(*persistConn).writeLoop(0xc0000a17a0)
          /home/vadmeste/work/go/src/net/http/transport.go:1888 +0x1b8
  created by net/http.(*Transport).dialConn
          /home/vadmeste/work/go/src/net/http/transport.go:1339 +0x966
  exit status 2
```

Hence, removing Headers since it is a map and it is uncomparable.
  • Loading branch information
vadmeste authored and kannappanr committed Jan 23, 2019
1 parent a42b0e1 commit 0be3a44
Show file tree
Hide file tree
Showing 2 changed files with 12 additions and 7 deletions.
8 changes: 2 additions & 6 deletions api-error-response.go
Expand Up @@ -36,6 +36,8 @@ import (
*/

// ErrorResponse - Is the typed error returned by all API operations.
// ErrorResponse struct should be comparable since it is compared inside
// golang http API (https://github.com/golang/go/issues/29768)
type ErrorResponse struct {
XMLName xml.Name `xml:"Error" json:"-"`
Code string
Expand All @@ -51,9 +53,6 @@ type ErrorResponse struct {

// Underlying HTTP status code for the returned error
StatusCode int `xml:"-" json:"-"`

// Headers of the returned S3 XML error
Headers http.Header `xml:"-" json:"-"`
}

// ToErrorResponse - Returns parsed ErrorResponse struct from body and
Expand Down Expand Up @@ -177,9 +176,6 @@ func httpRespToErrorResponse(resp *http.Response, bucketName, objectName string)
errResp.Message = fmt.Sprintf("Region does not match, expecting region ‘%s’.", errResp.Region)
}

// Save headers returned in the API XML error
errResp.Headers = resp.Header

return errResp
}

Expand Down
11 changes: 10 additions & 1 deletion api-error-response_test.go
Expand Up @@ -77,7 +77,6 @@ func TestHttpRespToErrorResponse(t *testing.T) {
RequestID: resp.Header.Get("x-amz-request-id"),
HostID: resp.Header.Get("x-amz-id-2"),
Region: resp.Header.Get("x-amz-bucket-region"),
Headers: resp.Header,
}
return errResp
}
Expand Down Expand Up @@ -292,3 +291,13 @@ func TestErrWithoutMessage(t *testing.T) {
t.Errorf("Expected \"Error response code InvalidArgument.\", got %s", errResp)
}
}

// Tests if ErrorResponse is comparable since it is compared
// inside golang http code (https://github.com/golang/go/issues/29768)
func TestErrorResponseComparable(t *testing.T) {
var e1 interface{} = ErrorResponse{}
var e2 interface{} = ErrorResponse{}
if e1 != e2 {
t.Fatalf("ErrorResponse should be comparable")
}
}

0 comments on commit 0be3a44

Please sign in to comment.