Fix file descriptor leaks and add test

* Add response.Body.Close() where needed to release HTTP
  connections to API server.
* Add tests to ensure no general leaks occur. 100% coverage would be
  required to ensure no leaks on any call.
* Update code comments to be godoc correct

Signed-off-by: Jhon Honce <[email protected]>

cmd/podman/generate/kube.go

@@ -2,6 +2,7 @@ package pods
 
 import (
 	"fmt"
+	"io"
 	"io/ioutil"
 	"os"
 
@@ -61,6 +62,10 @@ func kube(cmd *cobra.Command, args []string) error {
 	if err != nil {
 		return err
 	}
+	if r, ok := report.Reader.(io.ReadCloser); ok {
+		defer r.Close()
+	}
+
 	if cmd.Flags().Changed("filename") {
 		if _, err := os.Stat(kubeFile); err == nil {
 			return errors.Errorf("cannot write to %q; file exists", kubeFile)

pkg/bindings/connection.go

@@ -56,7 +56,7 @@ func NewConnection(ctx context.Context, uri string) (context.Context, error) {
 	return NewConnectionWithIdentity(ctx, uri, "")
 }
 
-// NewConnection takes a URI as a string and returns a context with the
+// NewConnectionWithIdentity takes a URI as a string and returns a context with the
 // Connection embedded as a value.  This context needs to be passed to each
 // endpoint to work correctly.
 //
@@ -149,6 +149,7 @@ func pingNewConnection(ctx context.Context) error {
 	if err != nil {
 		return err
 	}
+	defer response.Body.Close()
 
 	if response.StatusCode == http.StatusOK {
 		versionHdr := response.Header.Get("Libpod-API-Version")
@@ -338,7 +339,7 @@ func (c *Connection) DoRequest(httpBody io.Reader, httpMethod, endpoint string,
 		req.Header.Set(key, val)
 	}
 	// Give the Do three chances in the case of a comm/service hiccup
-	for i := 0; i < 3; i++ {
+	for i := 1; i <= 3; i++ {
 		response, err = c.Client.Do(req) // nolint
 		if err == nil {
 			break
@@ -358,7 +359,7 @@ func FiltersToString(filters map[string][]string) (string, error) {
 	return jsoniter.MarshalToString(lowerCaseKeys)
 }
 
-// IsInformation returns true if the response code is 1xx
+// IsInformational returns true if the response code is 1xx
 func (h *APIResponse) IsInformational() bool {
 	return h.Response.StatusCode/100 == 1
 }

pkg/bindings/containers/archive.go

@@ -27,6 +27,7 @@ func Stat(ctx context.Context, nameOrID string, path string) (*entities.Containe
 	if err != nil {
 		return nil, err
 	}
+	defer response.Body.Close()
 
 	var finalErr error
 	if response.StatusCode == http.StatusNotFound {
@@ -53,7 +54,9 @@ func CopyFromArchive(ctx context.Context, nameOrID string, path string, reader i
 	return CopyFromArchiveWithOptions(ctx, nameOrID, path, reader, nil)
 }
 
-// CopyFromArchiveWithOptions FIXME: remove this function and make CopyFromArchive accept the option as the last parameter in podman 4.0
+// CopyFromArchiveWithOptions copy files into container
+//
+// FIXME: remove this function and make CopyFromArchive accept the option as the last parameter in podman 4.0
 func CopyFromArchiveWithOptions(ctx context.Context, nameOrID string, path string, reader io.Reader, options *CopyOptions) (entities.ContainerCopyFunc, error) {
 	conn, err := bindings.GetClient(ctx)
 	if err != nil {
@@ -72,13 +75,15 @@ func CopyFromArchiveWithOptions(ctx context.Context, nameOrID string, path strin
 		if err != nil {
 			return err
 		}
+
 		if response.StatusCode != http.StatusOK {
 			return errors.New(response.Status)
 		}
 		return response.Process(nil)
 	}, nil
 }
 
+// CopyToArchive copy files from container
 func CopyToArchive(ctx context.Context, nameOrID string, path string, writer io.Writer) (entities.ContainerCopyFunc, error) {
 	conn, err := bindings.GetClient(ctx)
 	if err != nil {
@@ -91,11 +96,14 @@ func CopyToArchive(ctx context.Context, nameOrID string, path string, writer io.
 	if err != nil {
 		return nil, err
 	}
+
 	if response.StatusCode != http.StatusOK {
+		defer response.Body.Close()
 		return nil, response.Process(nil)
 	}
 
 	return func() error {
+		defer response.Body.Close()
 		_, err := io.Copy(writer, response.Body)
 		return err
 	}, nil

pkg/bindings/containers/attach.go

@@ -134,7 +134,9 @@ func Attach(ctx context.Context, nameOrID string, stdin io.Reader, stdout io.Wri
 	if err != nil {
 		return err
 	}
+
 	if !(response.IsSuccess() || response.IsInformational()) {
+		defer response.Body.Close()
 		return response.Process(nil)
 	}
 
@@ -207,7 +209,7 @@ func Attach(ctx context.Context, nameOrID string, stdin io.Reader, stdout io.Wri
 			}
 		}
 	} else {
-		logrus.Debugf("Copying standard streams of container in non-terminal mode")
+		logrus.Debugf("Copying standard streams of container %q in non-terminal mode", ctnr.ID)
 		for {
 			// Read multiplexed channels and write to appropriate stream
 			fd, l, err := DemuxHeader(socket, buffer)
@@ -324,6 +326,8 @@ func resizeTTY(ctx context.Context, endpoint string, height *int, width *int) er
 	if err != nil {
 		return err
 	}
+	defer rsp.Body.Close()
+
 	return rsp.Process(nil)
 }
 
@@ -407,6 +411,7 @@ func ExecStartAndAttach(ctx context.Context, sessionID string, options *ExecStar
 	if err != nil {
 		return err
 	}
+	defer resp.Body.Close()
 
 	respStruct := new(define.InspectExecSession)
 	if err := resp.Process(respStruct); err != nil {
@@ -477,6 +482,8 @@ func ExecStartAndAttach(ctx context.Context, sessionID string, options *ExecStar
 	if err != nil {
 		return err
 	}
+	defer response.Body.Close()
+
 	if !(response.IsSuccess() || response.IsInformational()) {
 		return response.Process(nil)
 	}

pkg/bindings/containers/checkpoint.go

@@ -27,6 +27,8 @@ func Checkpoint(ctx context.Context, nameOrID string, options *CheckpointOptions
 	if err != nil {
 		return nil, err
 	}
+	defer response.Body.Close()
+
 	return &report, response.Process(&report)
 }
 
@@ -54,5 +56,7 @@ func Restore(ctx context.Context, nameOrID string, options *RestoreOptions) (*en
 	if err != nil {
 		return nil, err
 	}
+	defer response.Body.Close()
+
 	return &report, response.Process(&report)
 }

pkg/bindings/containers/commit.go

@@ -28,5 +28,7 @@ func Commit(ctx context.Context, nameOrID string, options *CommitOptions) (handl
 	if err != nil {
 		return id, err
 	}
+	defer response.Body.Close()
+
 	return id, response.Process(&id)
 }

pkg/bindings/containers/containers.go

@@ -42,6 +42,8 @@ func List(ctx context.Context, options *ListOptions) ([]entities.ListContainer,
 	if err != nil {
 		return containers, err
 	}
+	defer response.Body.Close()
+
 	return containers, response.Process(&containers)
 }
 
@@ -66,6 +68,8 @@ func Prune(ctx context.Context, options *PruneOptions) ([]*reports.PruneReport,
 	if err != nil {
 		return nil, err
 	}
+	defer response.Body.Close()
+
 	return reports, response.Process(&reports)
 }
 
@@ -90,6 +94,8 @@ func Remove(ctx context.Context, nameOrID string, options *RemoveOptions) error
 	if err != nil {
 		return err
 	}
+	defer response.Body.Close()
+
 	return response.Process(nil)
 }
 
@@ -113,6 +119,8 @@ func Inspect(ctx context.Context, nameOrID string, options *InspectOptions) (*de
 	if err != nil {
 		return nil, err
 	}
+	defer response.Body.Close()
+
 	inspect := define.InspectContainerData{}
 	return &inspect, response.Process(&inspect)
 }
@@ -136,6 +144,8 @@ func Kill(ctx context.Context, nameOrID string, options *KillOptions) error {
 	if err != nil {
 		return err
 	}
+	defer response.Body.Close()
+
 	return response.Process(nil)
 }
 
@@ -154,6 +164,8 @@ func Pause(ctx context.Context, nameOrID string, options *PauseOptions) error {
 	if err != nil {
 		return err
 	}
+	defer response.Body.Close()
+
 	return response.Process(nil)
 }
 
@@ -176,6 +188,8 @@ func Restart(ctx context.Context, nameOrID string, options *RestartOptions) erro
 	if err != nil {
 		return err
 	}
+	defer response.Body.Close()
+
 	return response.Process(nil)
 }
 
@@ -199,6 +213,8 @@ func Start(ctx context.Context, nameOrID string, options *StartOptions) error {
 	if err != nil {
 		return err
 	}
+	defer response.Body.Close()
+
 	return response.Process(nil)
 }
 
@@ -231,6 +247,7 @@ func Stats(ctx context.Context, containers []string, options *StatsOptions) (cha
 
 	go func() {
 		defer close(statsChan)
+		defer response.Body.Close()
 
 		dec := json.NewDecoder(response.Body)
 		doStream := true
@@ -245,6 +262,7 @@ func Stats(ctx context.Context, containers []string, options *StatsOptions) (cha
 		default:
 			// fall through and do some work
 		}
+
 		var report entities.ContainerStatsReport
 		if err := dec.Decode(&report); err != nil {
 			report = entities.ContainerStatsReport{Error: err}
@@ -279,6 +297,7 @@ func Top(ctx context.Context, nameOrID string, options *TopOptions) ([]string, e
 	if err != nil {
 		return nil, err
 	}
+	defer response.Body.Close()
 
 	body := handlers.ContainerTopOKBody{}
 	if err = response.Process(&body); err != nil {
@@ -311,6 +330,8 @@ func Unpause(ctx context.Context, nameOrID string, options *UnpauseOptions) erro
 	if err != nil {
 		return err
 	}
+	defer response.Body.Close()
+
 	return response.Process(nil)
 }
 
@@ -334,6 +355,8 @@ func Wait(ctx context.Context, nameOrID string, options *WaitOptions) (int32, er
 	if err != nil {
 		return exitCode, err
 	}
+	defer response.Body.Close()
+
 	return exitCode, response.Process(&exitCode)
 }
 
@@ -353,6 +376,8 @@ func Exists(ctx context.Context, nameOrID string, options *ExistsOptions) (bool,
 	if err != nil {
 		return false, err
 	}
+	defer response.Body.Close()
+
 	return response.IsSuccess(), nil
 }
 
@@ -374,6 +399,8 @@ func Stop(ctx context.Context, nameOrID string, options *StopOptions) error {
 	if err != nil {
 		return err
 	}
+	defer response.Body.Close()
+
 	return response.Process(nil)
 }
 
@@ -393,6 +420,8 @@ func Export(ctx context.Context, nameOrID string, w io.Writer, options *ExportOp
 	if err != nil {
 		return err
 	}
+	defer response.Body.Close()
+
 	if response.StatusCode/100 == 2 {
 		_, err = io.Copy(w, response.Body)
 		return err
@@ -416,6 +445,8 @@ func ContainerInit(ctx context.Context, nameOrID string, options *InitOptions) e
 	if err != nil {
 		return err
 	}
+	defer response.Body.Close()
+
 	if response.StatusCode == http.StatusNotModified {
 		return errors.Wrapf(define.ErrCtrStateInvalid, "container %s has already been created in runtime", nameOrID)
 	}
@@ -435,5 +466,7 @@ func ShouldRestart(ctx context.Context, nameOrID string, options *ShouldRestartO
 	if err != nil {
 		return false, err
 	}
+	defer response.Body.Close()
+
 	return response.IsSuccess(), nil
 }

pkg/bindings/containers/create.go

@@ -30,5 +30,7 @@ func CreateWithSpec(ctx context.Context, s *specgen.SpecGenerator, options *Crea
 	if err != nil {
 		return ccr, err
 	}
+	defer response.Body.Close()
+
 	return ccr, response.Process(&ccr)
 }

pkg/bindings/containers/diff.go

@@ -26,6 +26,8 @@ func Diff(ctx context.Context, nameOrID string, options *DiffOptions) ([]archive
 	if err != nil {
 		return nil, err
 	}
+	defer response.Body.Close()
+
 	var changes []archive.Change
 	return changes, response.Process(&changes)
 }

pkg/bindings/containers/exec.go

@@ -39,6 +39,7 @@ func ExecCreate(ctx context.Context, nameOrID string, config *handlers.ExecCreat
 	if err != nil {
 		return "", err
 	}
+	defer resp.Body.Close()
 
 	respStruct := new(handlers.ExecCreateResponse)
 	if err := resp.Process(respStruct); err != nil {
@@ -66,6 +67,7 @@ func ExecInspect(ctx context.Context, sessionID string, options *ExecInspectOpti
 	if err != nil {
 		return nil, err
 	}
+	defer resp.Body.Close()
 
 	respStruct := new(define.InspectExecSession)
 	if err := resp.Process(respStruct); err != nil {
@@ -103,6 +105,7 @@ func ExecStart(ctx context.Context, sessionID string, options *ExecStartOptions)
 	if err != nil {
 		return err
 	}
+	defer resp.Body.Close()
 
 	return resp.Process(nil)
 }

pkg/bindings/containers/healthcheck.go

@@ -26,5 +26,7 @@ func RunHealthCheck(ctx context.Context, nameOrID string, options *HealthCheckOp
 	if err != nil {
 		return nil, err
 	}
+	defer response.Body.Close()
+
 	return &status, response.Process(&status)
 }

pkg/bindings/containers/logs.go

@@ -33,6 +33,7 @@ func Logs(ctx context.Context, nameOrID string, options *LogOptions, stdoutChan,
 	if err != nil {
 		return err
 	}
+	defer response.Body.Close()
 
 	buffer := make([]byte, 1024)
 	for {