From c698c9b1ca4c7195a49b1c19840f8528898e22e3 Mon Sep 17 00:00:00 2001 From: Shantanu Kotambkar <52007797+skotambkar@users.noreply.github.com> Date: Thu, 29 Jul 2021 15:42:58 -0700 Subject: [PATCH] Handle error for deferred close calls (#1326) Handle error for deferred close() --- .../b18b4ffbdf9a45239616f48de1ef2096.json | 14 ++ aws/generate.go | 14 +- config/resolve_credentials_test.go | 12 +- feature/cloudfront/sign/privkey.go | 12 +- feature/ec2/imds/api_op_GetIAMInfo.go | 13 +- .../api_op_GetInstanceIdentityDocument.go | 13 +- feature/ec2/imds/api_op_GetToken.go | 13 +- feature/s3/manager/upload_test.go | 24 ++- internal/awstesting/custom_ca_bundle.go | 24 ++- internal/ini/ini.go | 19 ++- internal/ini/walker_test.go | 149 ++++++++++-------- 11 files changed, 212 insertions(+), 95 deletions(-) create mode 100644 .changelog/b18b4ffbdf9a45239616f48de1ef2096.json diff --git a/.changelog/b18b4ffbdf9a45239616f48de1ef2096.json b/.changelog/b18b4ffbdf9a45239616f48de1ef2096.json new file mode 100644 index 00000000000..06d0ebf00b7 --- /dev/null +++ b/.changelog/b18b4ffbdf9a45239616f48de1ef2096.json @@ -0,0 +1,14 @@ +{ + "id": "b18b4ffb-df9a-4523-9616-f48de1ef2096", + "type": "feature", + "collapse": true, + "description": "adds error handling for defered close calls", + "modules": [ + ".", + "config", + "feature/cloudfront/sign", + "feature/ec2/imds", + "feature/s3/manager", + "internal/ini" + ] +} diff --git a/aws/generate.go b/aws/generate.go index d71e4574ca4..206971a9baa 100644 --- a/aws/generate.go +++ b/aws/generate.go @@ -24,14 +24,22 @@ func main() { } } -func generateFile(filename string, tmplName string, types ptr.Scalars) error { +func generateFile(filename string, tmplName string, types ptr.Scalars) (err error) { f, err := os.Create(filename) if err != nil { return fmt.Errorf("failed to create %s file, %v", filename, err) } - defer f.Close() - if err := ptrTmpl.ExecuteTemplate(f, tmplName, types); err != nil { + defer func() { + closeErr := f.Close() + if err == nil { + err = closeErr + } else if closeErr != nil { + err = fmt.Errorf("close error: %v, original error: %w", closeErr, err) + } + }() + + if err = ptrTmpl.ExecuteTemplate(f, tmplName, types); err != nil { return fmt.Errorf("failed to generate %s file, %v", filename, err) } diff --git a/config/resolve_credentials_test.go b/config/resolve_credentials_test.go index fdd430600a8..a3888bbfdf7 100644 --- a/config/resolve_credentials_test.go +++ b/config/resolve_credentials_test.go @@ -121,7 +121,7 @@ func setupCredentialsEndpoints(t *testing.T) (aws.EndpointResolver, func()) { } } -func ssoTestSetup() (func(), error) { +func ssoTestSetup() (fn func(), err error) { dir, err := ioutil.TempDir("", "sso-test") if err != nil { return nil, err @@ -139,7 +139,15 @@ func ssoTestSetup() (func(), error) { os.RemoveAll(dir) return nil, err } - defer tokenFile.Close() + + defer func() { + closeErr := tokenFile.Close() + if err == nil { + err = closeErr + } else if closeErr != nil { + err = fmt.Errorf("close error: %v, original error: %w", closeErr, err) + } + }() _, err = tokenFile.WriteString(fmt.Sprintf(ssoTokenCacheFile, time.Now(). Add(15*time.Minute). diff --git a/feature/cloudfront/sign/privkey.go b/feature/cloudfront/sign/privkey.go index ffb3c3a751d..cb3113684c0 100644 --- a/feature/cloudfront/sign/privkey.go +++ b/feature/cloudfront/sign/privkey.go @@ -12,12 +12,20 @@ import ( // LoadPEMPrivKeyFile reads a PEM encoded RSA private key from the file name. // A new RSA private key will be returned if no error. -func LoadPEMPrivKeyFile(name string) (*rsa.PrivateKey, error) { +func LoadPEMPrivKeyFile(name string) (key *rsa.PrivateKey, err error) { file, err := os.Open(name) if err != nil { return nil, err } - defer file.Close() + + defer func() { + closeErr := file.Close() + if err == nil { + err = closeErr + } else if closeErr != nil { + err = fmt.Errorf("close error: %v, original error: %w", closeErr, err) + } + }() return LoadPEMPrivKey(file) } diff --git a/feature/ec2/imds/api_op_GetIAMInfo.go b/feature/ec2/imds/api_op_GetIAMInfo.go index 62a466e9a9e..24845dccd6d 100644 --- a/feature/ec2/imds/api_op_GetIAMInfo.go +++ b/feature/ec2/imds/api_op_GetIAMInfo.go @@ -62,15 +62,22 @@ func buildGetIAMInfoPath(params interface{}) (string, error) { return getIAMInfoPath, nil } -func buildGetIAMInfoOutput(resp *smithyhttp.Response) (interface{}, error) { - defer resp.Body.Close() +func buildGetIAMInfoOutput(resp *smithyhttp.Response) (v interface{}, err error) { + defer func() { + closeErr := resp.Body.Close() + if err == nil { + err = closeErr + } else if closeErr != nil { + err = fmt.Errorf("response body close error: %v, original error: %w", closeErr, err) + } + }() var buff [1024]byte ringBuffer := smithyio.NewRingBuffer(buff[:]) body := io.TeeReader(resp.Body, ringBuffer) imdsResult := &GetIAMInfoOutput{} - if err := json.NewDecoder(body).Decode(&imdsResult.IAMInfo); err != nil { + if err = json.NewDecoder(body).Decode(&imdsResult.IAMInfo); err != nil { return nil, &smithy.DeserializationError{ Err: fmt.Errorf("failed to decode instance identity document, %w", err), Snapshot: ringBuffer.Bytes(), diff --git a/feature/ec2/imds/api_op_GetInstanceIdentityDocument.go b/feature/ec2/imds/api_op_GetInstanceIdentityDocument.go index 9a8dc8360de..a87758ed302 100644 --- a/feature/ec2/imds/api_op_GetInstanceIdentityDocument.go +++ b/feature/ec2/imds/api_op_GetInstanceIdentityDocument.go @@ -63,15 +63,22 @@ func buildGetInstanceIdentityDocumentPath(params interface{}) (string, error) { return getInstanceIdentityDocumentPath, nil } -func buildGetInstanceIdentityDocumentOutput(resp *smithyhttp.Response) (interface{}, error) { - defer resp.Body.Close() +func buildGetInstanceIdentityDocumentOutput(resp *smithyhttp.Response) (v interface{}, err error) { + defer func() { + closeErr := resp.Body.Close() + if err == nil { + err = closeErr + } else if closeErr != nil { + err = fmt.Errorf("response body close error: %v, original error: %w", closeErr, err) + } + }() var buff [1024]byte ringBuffer := smithyio.NewRingBuffer(buff[:]) body := io.TeeReader(resp.Body, ringBuffer) output := &GetInstanceIdentityDocumentOutput{} - if err := json.NewDecoder(body).Decode(&output.InstanceIdentityDocument); err != nil { + if err = json.NewDecoder(body).Decode(&output.InstanceIdentityDocument); err != nil { return nil, &smithy.DeserializationError{ Err: fmt.Errorf("failed to decode instance identity document, %w", err), Snapshot: ringBuffer.Bytes(), diff --git a/feature/ec2/imds/api_op_GetToken.go b/feature/ec2/imds/api_op_GetToken.go index 2f58b9cfa5b..841f802c1a3 100644 --- a/feature/ec2/imds/api_op_GetToken.go +++ b/feature/ec2/imds/api_op_GetToken.go @@ -67,8 +67,15 @@ func buildGetTokenPath(interface{}) (string, error) { return getTokenPath, nil } -func buildGetTokenOutput(resp *smithyhttp.Response) (interface{}, error) { - defer resp.Body.Close() +func buildGetTokenOutput(resp *smithyhttp.Response) (v interface{}, err error) { + defer func() { + closeErr := resp.Body.Close() + if err == nil { + err = closeErr + } else if closeErr != nil { + err = fmt.Errorf("response body close error: %v, original error: %w", closeErr, err) + } + }() ttlHeader := resp.Header.Get(tokenTTLHeader) tokenTTL, err := strconv.ParseInt(ttlHeader, 10, 64) @@ -77,7 +84,7 @@ func buildGetTokenOutput(resp *smithyhttp.Response) (interface{}, error) { } var token strings.Builder - if _, err := io.Copy(&token, resp.Body); err != nil { + if _, err = io.Copy(&token, resp.Body); err != nil { return nil, fmt.Errorf("unable to read API token, %w", err) } diff --git a/feature/s3/manager/upload_test.go b/feature/s3/manager/upload_test.go index 1effe363527..ca149bbd884 100644 --- a/feature/s3/manager/upload_test.go +++ b/feature/s3/manager/upload_test.go @@ -1034,7 +1034,13 @@ func newMockS3UploadServer(tb testing.TB, partHandler []http.Handler) *mockS3Upl } func (s mockS3UploadServer) handleRequest(w http.ResponseWriter, r *http.Request) { - defer r.Body.Close() + defer func() { + closeErr := r.Body.Close() + if closeErr != nil { + failRequest(w, 0, "BodyCloseError", + fmt.Sprintf("request body close error: %v", closeErr)) + } + }() _, hasUploads := r.URL.Query()["uploads"] @@ -1091,7 +1097,13 @@ type successPartHandler struct { } func (h successPartHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { - defer r.Body.Close() + defer func() { + closeErr := r.Body.Close() + if closeErr != nil { + failRequest(w, 0, "BodyCloseError", + fmt.Sprintf("request body close error: %v", closeErr)) + } + }() n, err := io.Copy(ioutil.Discard, r.Body) if err != nil { @@ -1128,7 +1140,13 @@ type failPartHandler struct { } func (h *failPartHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { - defer r.Body.Close() + defer func() { + closeErr := r.Body.Close() + if closeErr != nil { + failRequest(w, 0, "BodyCloseError", + fmt.Sprintf("request body close error: %v", closeErr)) + } + }() if h.failsRemaining == 0 && h.successHandler != nil { h.successHandler.ServeHTTP(w, r) diff --git a/internal/awstesting/custom_ca_bundle.go b/internal/awstesting/custom_ca_bundle.go index 13edc7bcf1e..47754ef7e82 100644 --- a/internal/awstesting/custom_ca_bundle.go +++ b/internal/awstesting/custom_ca_bundle.go @@ -1,6 +1,7 @@ package awstesting import ( + "fmt" "io/ioutil" "net" "net/http" @@ -9,12 +10,19 @@ import ( "time" ) -func availableLocalAddr(ip string) (string, error) { +func availableLocalAddr(ip string) (v string, err error) { l, err := net.Listen("tcp", ip+":0") if err != nil { return "", err } - defer l.Close() + defer func() { + closeErr := l.Close() + if err == nil { + err = closeErr + } else if closeErr != nil { + err = fmt.Errorf("ip listener close error: %v, original error: %w", closeErr, err) + } + }() return l.Addr().String(), nil } @@ -82,7 +90,7 @@ func CleanupTLSBundleFiles(files ...string) error { return nil } -func createTmpFile(b []byte) (string, error) { +func createTmpFile(b []byte) (v string, err error) { bundleFile, err := ioutil.TempFile(os.TempDir(), "aws-sdk-go-session-test") if err != nil { return "", err @@ -93,7 +101,15 @@ func createTmpFile(b []byte) (string, error) { return "", err } - defer bundleFile.Close() + defer func() { + closeErr := bundleFile.Close() + if err == nil { + err = closeErr + } else if closeErr != nil { + err = fmt.Errorf("file close error: %v, original error: %w", closeErr, err) + } + }() + return bundleFile.Name(), nil } diff --git a/internal/ini/ini.go b/internal/ini/ini.go index 4a80eb9a99d..f7406231318 100644 --- a/internal/ini/ini.go +++ b/internal/ini/ini.go @@ -1,18 +1,27 @@ package ini import ( + "fmt" "io" "os" ) // OpenFile takes a path to a given file, and will open and parse // that file. -func OpenFile(path string) (Sections, error) { - f, err := os.Open(path) - if err != nil { - return Sections{}, &UnableToReadFile{Err: err} +func OpenFile(path string) (sections Sections, err error) { + f, oerr := os.Open(path) + if oerr != nil { + return Sections{}, &UnableToReadFile{Err: oerr} } - defer f.Close() + + defer func() { + closeErr := f.Close() + if err == nil { + err = closeErr + } else if closeErr != nil { + err = fmt.Errorf("close error: %v, original error: %w", closeErr, err) + } + }() return Parse(f, path) } diff --git a/internal/ini/walker_test.go b/internal/ini/walker_test.go index 4d56e022f8d..a59e6dc1537 100644 --- a/internal/ini/walker_test.go +++ b/internal/ini/walker_test.go @@ -2,6 +2,7 @@ package ini import ( "encoding/json" + "fmt" "io/ioutil" "os" "path/filepath" @@ -11,86 +12,95 @@ import ( func TestValidDataFiles(t *testing.T) { const expectedFileSuffix = "_expected" - err := filepath.Walk(filepath.Join("testdata", "valid"), func(path string, info os.FileInfo, err error) error { - if strings.HasSuffix(path, expectedFileSuffix) { - return nil - } + err := filepath.Walk(filepath.Join("testdata", "valid"), + func(path string, info os.FileInfo, fnErr error) (err error) { + if strings.HasSuffix(path, expectedFileSuffix) { + return nil + } - if info.IsDir() { - return nil - } - - f, err := os.Open(path) - if err != nil { - t.Errorf("%s: unexpected error, %v", path, err) - } - defer f.Close() - - tree, err := ParseAST(f) - if err != nil { - t.Errorf("%s: unexpected parse error, %v", path, err) - } - - v := NewDefaultVisitor(path) - err = Walk(tree, v) - if err != nil { - t.Errorf("%s: unexpected walk error, %v", path, err) - } - - expectedPath := path + "_expected" - e := map[string]interface{}{} - - b, err := ioutil.ReadFile(expectedPath) - if err != nil { - // ignore files that do not have an expected file - return nil - } + if info.IsDir() { + return nil + } - err = json.Unmarshal(b, &e) - if err != nil { - t.Errorf("unexpected error during deserialization, %v", err) - } + f, err := os.Open(path) + if err != nil { + t.Errorf("%s: unexpected error, %v", path, err) + } + + defer func() { + closeErr := f.Close() + if err == nil { + err = closeErr + } else if closeErr != nil { + err = fmt.Errorf("file close error: %v, original error: %w", closeErr, err) + } + }() - for profile, tableIface := range e { - p, ok := v.Sections.GetSection(profile) - if !ok { - t.Fatal("could not find profile " + profile) + tree, err := ParseAST(f) + if err != nil { + t.Errorf("%s: unexpected parse error, %v", path, err) } - table := tableIface.(map[string]interface{}) - for k, v := range table { - switch e := v.(type) { - case string: - a := p.String(k) - if e != a { - t.Errorf("%s: expected %v, but received %v for profile %v", path, e, a, profile) - } - case int: - a := p.Int(k) - if int64(e) != a { - t.Errorf("%s: expected %v, but received %v for profile %v", path, e, a, profile) - } - case float64: - v := p.values[k] - if v.Type == IntegerType { + v := NewDefaultVisitor(path) + err = Walk(tree, v) + if err != nil { + t.Errorf("%s: unexpected walk error, %v", path, err) + } + + expectedPath := path + "_expected" + e := map[string]interface{}{} + + b, err := ioutil.ReadFile(expectedPath) + if err != nil { + // ignore files that do not have an expected file + return nil + } + + err = json.Unmarshal(b, &e) + if err != nil { + t.Errorf("unexpected error during deserialization, %v", err) + } + + for profile, tableIface := range e { + p, ok := v.Sections.GetSection(profile) + if !ok { + t.Fatal("could not find profile " + profile) + } + + table := tableIface.(map[string]interface{}) + for k, v := range table { + switch e := v.(type) { + case string: + a := p.String(k) + if e != a { + t.Errorf("%s: expected %v, but received %v for profile %v", path, e, a, profile) + } + case int: a := p.Int(k) if int64(e) != a { t.Errorf("%s: expected %v, but received %v for profile %v", path, e, a, profile) } - } else { - a := p.Float64(k) - if e != a { - t.Errorf("%s: expected %v, but received %v for profile %v", path, e, a, profile) + case float64: + v := p.values[k] + if v.Type == IntegerType { + a := p.Int(k) + if int64(e) != a { + t.Errorf("%s: expected %v, but received %v for profile %v", path, e, a, profile) + } + } else { + a := p.Float64(k) + if e != a { + t.Errorf("%s: expected %v, but received %v for profile %v", path, e, a, profile) + } } + default: + t.Errorf("unexpected type: %T", e) } - default: - t.Errorf("unexpected type: %T", e) } } - } - return nil - }) + return nil + }) if err != nil { t.Fatalf("Error while walking the file tree rooted at root, %d", err) } @@ -134,7 +144,12 @@ func TestInvalidDataFiles(t *testing.T) { if err != nil { t.Errorf("unexpected error, %v", err) } - defer f.Close() + defer func() { + closeErr := f.Close() + if closeErr != nil { + t.Errorf("unexpected file close error: %v", closeErr) + } + }() tree, err := ParseAST(f) if err != nil && !c.expectedParseError {