runtime/v2/runc: fix leaking socket path

fuweid · fuweid · commit d895118c7c48 · 2021-03-15T18:32:00.000+08:00
When runC shimv2 starts, the StartShim interface will re-exec itself as long-running process, which will read the `address` during initializing. ```happycase Process containerd-shim-runc-v1/v2 start containerd-shim-runc-v1/v2 initializing socket reexec containerd-shim-runc-v1/v2 write address into file initializing read address write back to containerd daemon serving ... remove address in Shutdown call ``` However, there is no synchronization after reexec. Then the data race is like: ```leaking-case Process containerd-shim-runc-v1/v2 start containerd-shim-runc-v1/v2 initializing socket reexec containerd-shim-runc-v1/v2 initializing read address write address into file write back to containerd daemon serving ... fail to remove address because of empty address ``` The `address` should be writen into file first before reexec. And if shutdown the whole service before cleanup temporary resource (like socket file), the Shutdown caller will receive `ttrpc: closed` sometime, which depends on go runtime scheduler. Then it also causes leaking socket files. Since the shimV2-Delete binary API must be called to cleanup shim temporary resource and shimV2-runC-v1 doesn't support grouping multi containers in one, it is safe to remove the socket file in the binary call for shimV2-runC-v1. But for the shimV2-runC-v2 shim, we still cleanup socket in Shutdown. Hopefully we can find a way to cleanup socket in shimV2-Delete binary call. Fix: #5173 Signed-off-by: Wei Fu <fuweid89@gmail.com>
diff --git a/runtime/v2/runc/v1/service.go b/runtime/v2/runc/v1/service.go
@@ -153,6 +153,11 @@ func (s *service) StartShim(ctx context.Context, id, containerdBinary, container
 			_ = shim.RemoveSocket(address)
 		}
 	}()
+	// make sure that reexec shim-v2 binary use the value if need
+	if err := shim.WriteAddress("address", address); err != nil {
+		return "", err
+	}
+
 	f, err := socket.File()
 	if err != nil {
 		return "", err
@@ -174,9 +179,6 @@ func (s *service) StartShim(ctx context.Context, id, containerdBinary, container
 	if err := shim.WritePidFile("shim.pid", cmd.Process.Pid); err != nil {
 		return "", err
 	}
-	if err := shim.WriteAddress("address", address); err != nil {
-		return "", err
-	}
 	if data, err := ioutil.ReadAll(os.Stdin); err == nil {
 		if len(data) > 0 {
 			var any ptypes.Any
@@ -209,6 +211,12 @@ func (s *service) StartShim(ctx context.Context, id, containerdBinary, container
 }
 
 func (s *service) Cleanup(ctx context.Context) (*taskAPI.DeleteResponse, error) {
+	if address, err := shim.ReadAddress("address"); err == nil {
+		if err = shim.RemoveSocket(address); err != nil {
+			return nil, err
+		}
+	}
+
 	path, err := os.Getwd()
 	if err != nil {
 		return nil, err
@@ -562,11 +570,10 @@ func (s *service) Connect(ctx context.Context, r *taskAPI.ConnectRequest) (*task
 }
 
 func (s *service) Shutdown(ctx context.Context, r *taskAPI.ShutdownRequest) (*ptypes.Empty, error) {
+	// please make sure that temporary resource has been cleanup
+	// before shutdown service.
 	s.cancel()
 	close(s.events)
-	if address, err := shim.ReadAddress("address"); err == nil {
-		_ = shim.RemoveSocket(address)
-	}
 	return empty, nil
 }
 
diff --git a/runtime/v2/runc/v2/service.go b/runtime/v2/runc/v2/service.go
@@ -220,6 +220,12 @@ func (s *service) StartShim(ctx context.Context, id, containerdBinary, container
 			_ = shim.RemoveSocket(address)
 		}
 	}()
+
+	// make sure that reexec shim-v2 binary use the value if need
+	if err := shim.WriteAddress("address", address); err != nil {
+		return "", err
+	}
+
 	f, err := socket.File()
 	if err != nil {
 		return "", err
@@ -238,9 +244,6 @@ func (s *service) StartShim(ctx context.Context, id, containerdBinary, container
 	}()
 	// make sure to wait after start
 	go cmd.Wait()
-	if err := shim.WriteAddress("address", address); err != nil {
-		return "", err
-	}
 	if data, err := ioutil.ReadAll(os.Stdin); err == nil {
 		if len(data) > 0 {
 			var any ptypes.Any
@@ -290,6 +293,7 @@ func (s *service) Cleanup(ctx context.Context) (*taskAPI.DeleteResponse, error)
 	if err != nil {
 		return nil, err
 	}
+
 	path := filepath.Join(filepath.Dir(cwd), s.id)
 	ns, err := namespaces.NamespaceRequired(ctx)
 	if err != nil {
@@ -668,15 +672,19 @@ func (s *service) Shutdown(ctx context.Context, r *taskAPI.ShutdownRequest) (*pt
 	if len(s.containers) > 0 {
 		return empty, nil
 	}
-	s.cancel()
-	close(s.events)
 
 	if s.platform != nil {
 		s.platform.Close()
 	}
+
 	if s.shimAddress != "" {
 		_ = shim.RemoveSocket(s.shimAddress)
 	}
+
+	// please make sure that temporary resource has been cleanup
+	// before shutdown service.
+	s.cancel()
+	close(s.events)
 	return empty, nil
 }
 
diff --git a/runtime/v2/shim/shim.go b/runtime/v2/shim/shim.go
@@ -239,6 +239,13 @@ func run(id string, initFunc Init, config Config) error {
 				return err
 			}
 		}
+
+		// NOTE: If the shim server is down(like oom killer), the address
+		// socket might be leaking.
+		if address, err := ReadAddress("address"); err == nil {
+			_ = RemoveSocket(address)
+		}
+
 		select {
 		case <-publisher.Done():
 			return nil
@@ -299,15 +306,11 @@ func serve(ctx context.Context, server *ttrpc.Server, path string) error {
 		return err
 	}
 	go func() {
+		defer l.Close()
 		if err := server.Serve(ctx, l); err != nil &&
 			!strings.Contains(err.Error(), "use of closed network connection") {
 			logrus.WithError(err).Fatal("containerd-shim: ttrpc server failure")
 		}
-		l.Close()
-		if address, err := ReadAddress("address"); err == nil {
-			_ = RemoveSocket(address)
-		}
-
 	}()
 	return nil
 }
