Merge pull request containers#20405 from giuseppe/do-not-leak-session…

…s-with-remote

remote: exec: do not leak session IDs on errors

pkg/api/handlers/compat/exec.go

@@ -209,3 +209,30 @@ func ExecStartHandler(w http.ResponseWriter, r *http.Request) {
 	}
 	logrus.Debugf("Attach for container %s exec session %s completed successfully", sessionCtr.ID(), sessionID)
 }
+
+// ExecRemoveHandler removes a exec session.
+func ExecRemoveHandler(w http.ResponseWriter, r *http.Request) {
+	runtime := r.Context().Value(api.RuntimeKey).(*libpod.Runtime)
+
+	sessionID := mux.Vars(r)["id"]
+
+	bodyParams := new(handlers.ExecRemoveConfig)
+
+	if err := json.NewDecoder(r.Body).Decode(&bodyParams); err != nil {
+		utils.Error(w, http.StatusBadRequest, fmt.Errorf("failed to decode parameters for %s: %w", r.URL.String(), err))
+		return
+	}
+
+	sessionCtr, err := runtime.GetExecSessionContainer(sessionID)
+	if err != nil {
+		utils.Error(w, http.StatusNotFound, err)
+		return
+	}
+
+	logrus.Debugf("Removing exec session %s of container %s", sessionID, sessionCtr.ID())
+	if err := sessionCtr.ExecRemove(sessionID, bodyParams.Force); err != nil {
+		utils.InternalServerError(w, err)
+		return
+	}
+	logrus.Debugf("Removing exec session %s for container %s completed successfully", sessionID, sessionCtr.ID())
+}

pkg/api/handlers/types.go

@@ -161,3 +161,7 @@ type ExecStartConfig struct {
 	Height uint16 `json:"h"`
 	Width  uint16 `json:"w"`
 }
+
+type ExecRemoveConfig struct {
+	Force bool `json:"Force"`
+}

pkg/api/server/register_exec.go

@@ -346,5 +346,39 @@ func (s *APIServer) registerExecHandlers(r *mux.Router) error {
 	//   500:
 	//     $ref: "#/responses/internalError"
 	r.Handle(VersionedPath("/libpod/exec/{id}/json"), s.APIHandler(compat.ExecInspectHandler)).Methods(http.MethodGet)
+	// ................. .... ........................ ...... ExecRemoveLibpod
+	// ---
+	// tags:
+	//   - exec
+	// summary: Remove an exec instance
+	// description: |
+	//   Remove a previously set up exec instance. If force is true, the exec session is killed if it is still running.
+	// parameters:
+	//  - in: path
+	//    name: id
+	//    type: string
+	//    required: true
+	//    description: Exec instance ID
+	//  - in: body
+	//    name: control
+	//    description: Attributes for removal
+	//    schema:
+	//      type: object
+	//      properties:
+	//        Force:
+	//          type: boolean
+	//          description: Force removal of the session.
+	// produces:
+	// - application/json
+	// responses:
+	//   200:
+	//     description: no error
+	//   404:
+	//     $ref: "#/responses/execSessionNotFound"
+	//   409:
+	//	   description: container is not running.
+	//   500:
+	//     $ref: "#/responses/internalError"
+	r.Handle(VersionedPath("/libpod/exec/{id}/remove"), s.APIHandler(compat.ExecRemoveHandler)).Methods(http.MethodPost)
 	return nil
 }

pkg/bindings/containers/exec.go

@@ -111,3 +111,41 @@ func ExecStart(ctx context.Context, sessionID string, options *ExecStartOptions)
 
 	return resp.Process(nil)
 }
+
+// ExecRemove removes a given exec session.
+func ExecRemove(ctx context.Context, sessionID string, options *ExecRemoveOptions) error {
+	if options == nil {
+		options = new(ExecRemoveOptions)
+	}
+	_ = options
+	conn, err := bindings.GetClient(ctx)
+	if err != nil {
+		return err
+	}
+
+	logrus.Debugf("Removing exec session ID %q", sessionID)
+
+	// We force Detach to true
+	body := struct {
+		Force bool `json:"Force"`
+	}{
+		Force: false,
+	}
+
+	if options.Force != nil {
+		body.Force = *options.Force
+	}
+
+	bodyJSON, err := json.Marshal(body)
+	if err != nil {
+		return err
+	}
+
+	resp, err := conn.DoRequest(ctx, bytes.NewReader(bodyJSON), http.MethodPost, "/exec/%s/remove", nil, nil, sessionID)
+	if err != nil {
+		return err
+	}
+	defer resp.Body.Close()
+
+	return resp.Process(nil)
+}

pkg/bindings/containers/types.go

@@ -333,3 +333,10 @@ type CopyOptions struct {
 	// by the other type.
 	NoOverwriteDirNonDir *bool
 }
+
+// ExecRemoveOptions are optional options for removing an exec session
+//
+//go:generate go run ../generator/generator.go ExecRemoveOptions
+type ExecRemoveOptions struct {
+	Force *bool
+}

pkg/domain/infra/tunnel/containers.go

@@ -579,13 +579,21 @@ func makeExecConfig(options entities.ExecOptions) *handlers.ExecCreateConfig {
 	return createConfig
 }
 
-func (ic *ContainerEngine) ContainerExec(ctx context.Context, nameOrID string, options entities.ExecOptions, streams define.AttachStreams) (int, error) {
+func (ic *ContainerEngine) ContainerExec(ctx context.Context, nameOrID string, options entities.ExecOptions, streams define.AttachStreams) (exitCode int, retErr error) {
 	createConfig := makeExecConfig(options)
 
 	sessionID, err := containers.ExecCreate(ic.ClientCtx, nameOrID, createConfig)
 	if err != nil {
 		return 125, err
 	}
+	defer func() {
+		if err := containers.ExecRemove(ic.ClientCtx, sessionID, nil); err != nil {
+			if retErr == nil {
+				exitCode = -1
+				retErr = err
+			}
+		}
+	}()
 	startAndAttachOptions := new(containers.ExecStartAndAttachOptions)
 	startAndAttachOptions.WithOutputStream(streams.OutputStream).WithErrorStream(streams.ErrorStream)
 	if streams.InputStream != nil {
@@ -604,13 +612,18 @@ func (ic *ContainerEngine) ContainerExec(ctx context.Context, nameOrID string, o
 	return inspectOut.ExitCode, nil
 }
 
-func (ic *ContainerEngine) ContainerExecDetached(ctx context.Context, nameOrID string, options entities.ExecOptions) (string, error) {
+func (ic *ContainerEngine) ContainerExecDetached(ctx context.Context, nameOrID string, options entities.ExecOptions) (retSessionID string, retErr error) {
 	createConfig := makeExecConfig(options)
 
 	sessionID, err := containers.ExecCreate(ic.ClientCtx, nameOrID, createConfig)
 	if err != nil {
 		return "", err
 	}
+	defer func() {
+		if retErr != nil {
+			_ = containers.ExecRemove(ic.ClientCtx, sessionID, nil)
+		}
+	}()
 
 	if err := containers.ExecStart(ic.ClientCtx, sessionID, nil); err != nil {
 		return "", err

test/system/075-exec.bats

@@ -208,7 +208,6 @@ load helpers
 }
 
 @test "podman exec - does not leak session IDs on invalid command" {
-    skip_if_remote "FIXME FIXME FIXME: this should work on remote, but does not"
     run_podman run -d $IMAGE top
     cid="$output"