Skip to content

Commit 4a4bb85

Browse files
authored
Merge pull request from GHSA-36xw-fx78-c5r4
Use path based unix socket for shims
2 parents 7ccd064 + 126b35c commit 4a4bb85

File tree

11 files changed

+260
-55
lines changed

11 files changed

+260
-55
lines changed

Diff for: cmd/containerd-shim/main_unix.go

+12-4
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,7 @@ var (
7171
func init() {
7272
flag.BoolVar(&debugFlag, "debug", false, "enable debug output in logs")
7373
flag.StringVar(&namespaceFlag, "namespace", "", "namespace that owns the shim")
74-
flag.StringVar(&socketFlag, "socket", "", "abstract socket path to serve")
74+
flag.StringVar(&socketFlag, "socket", "", "socket path to serve")
7575
flag.StringVar(&addressFlag, "address", "", "grpc address back to main containerd")
7676
flag.StringVar(&workdirFlag, "workdir", "", "path used to storge large temporary data")
7777
flag.StringVar(&runtimeRootFlag, "runtime-root", process.RuncRoot, "root directory for the runtime")
@@ -202,10 +202,18 @@ func serve(ctx context.Context, server *ttrpc.Server, path string) error {
202202
f.Close()
203203
path = "[inherited from parent]"
204204
} else {
205-
if len(path) > 106 {
206-
return errors.Errorf("%q: unix socket path too long (> 106)", path)
205+
const (
206+
abstractSocketPrefix = "\x00"
207+
socketPathLimit = 106
208+
)
209+
p := strings.TrimPrefix(path, "unix://")
210+
if len(p) == len(path) {
211+
p = abstractSocketPrefix + p
207212
}
208-
l, err = net.Listen("unix", "\x00"+path)
213+
if len(p) > socketPathLimit {
214+
return errors.Errorf("%q: unix socket path too long (> %d)", p, socketPathLimit)
215+
}
216+
l, err = net.Listen("unix", p)
209217
}
210218
if err != nil {
211219
return err

Diff for: cmd/ctr/commands/shim/shim.go

+5-3
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ import (
2424
"io/ioutil"
2525
"net"
2626
"path/filepath"
27+
"strings"
2728

2829
"github.com/containerd/console"
2930
"github.com/containerd/containerd/cmd/ctr/commands"
@@ -240,10 +241,11 @@ func getTaskService(context *cli.Context) (task.TaskService, error) {
240241
s1 := filepath.Join(string(filepath.Separator), "containerd-shim", ns, id, "shim.sock")
241242
// this should not error, ctr always get a default ns
242243
ctx := namespaces.WithNamespace(gocontext.Background(), ns)
243-
s2, _ := shim.SocketAddress(ctx, id)
244+
s2, _ := shim.SocketAddress(ctx, context.GlobalString("address"), id)
245+
s2 = strings.TrimPrefix(s2, "unix://")
244246

245-
for _, socket := range []string{s1, s2} {
246-
conn, err := net.Dial("unix", "\x00"+socket)
247+
for _, socket := range []string{s2, "\x00" + s1} {
248+
conn, err := net.Dial("unix", socket)
247249
if err == nil {
248250
client := ttrpc.NewClient(conn)
249251

Diff for: runtime/v1/linux/bundle.go

+11-4
Original file line numberDiff line numberDiff line change
@@ -91,7 +91,7 @@ func ShimRemote(c *Config, daemonAddress, cgroup string, exitHandler func()) Shi
9191
return func(b *bundle, ns string, ropts *runctypes.RuncOptions) (shim.Config, client.Opt) {
9292
config := b.shimConfig(ns, c, ropts)
9393
return config,
94-
client.WithStart(c.Shim, b.shimAddress(ns), daemonAddress, cgroup, c.ShimDebug, exitHandler)
94+
client.WithStart(c.Shim, b.shimAddress(ns, daemonAddress), daemonAddress, cgroup, c.ShimDebug, exitHandler)
9595
}
9696
}
9797

@@ -117,6 +117,11 @@ func (b *bundle) NewShimClient(ctx context.Context, namespace string, getClientO
117117

118118
// Delete deletes the bundle from disk
119119
func (b *bundle) Delete() error {
120+
address, _ := b.loadAddress()
121+
if address != "" {
122+
// we don't care about errors here
123+
client.RemoveSocket(address)
124+
}
120125
err := atomicDelete(b.path)
121126
if err == nil {
122127
return atomicDelete(b.workDir)
@@ -133,9 +138,11 @@ func (b *bundle) legacyShimAddress(namespace string) string {
133138
return filepath.Join(string(filepath.Separator), "containerd-shim", namespace, b.id, "shim.sock")
134139
}
135140

136-
func (b *bundle) shimAddress(namespace string) string {
137-
d := sha256.Sum256([]byte(filepath.Join(namespace, b.id)))
138-
return filepath.Join(string(filepath.Separator), "containerd-shim", fmt.Sprintf("%x.sock", d))
141+
const socketRoot = "/run/containerd"
142+
143+
func (b *bundle) shimAddress(namespace, socketPath string) string {
144+
d := sha256.Sum256([]byte(filepath.Join(socketPath, namespace, b.id)))
145+
return fmt.Sprintf("unix://%s/%x", filepath.Join(socketRoot, "s"), d)
139146
}
140147

141148
func (b *bundle) loadAddress() (string, error) {

Diff for: runtime/v1/shim/client/client.go

+82-10
Original file line numberDiff line numberDiff line change
@@ -59,9 +59,17 @@ func WithStart(binary, address, daemonAddress, cgroup string, debug bool, exitHa
5959
return func(ctx context.Context, config shim.Config) (_ shimapi.ShimService, _ io.Closer, err error) {
6060
socket, err := newSocket(address)
6161
if err != nil {
62-
return nil, nil, err
62+
if !eaddrinuse(err) {
63+
return nil, nil, err
64+
}
65+
if err := RemoveSocket(address); err != nil {
66+
return nil, nil, errors.Wrap(err, "remove already used socket")
67+
}
68+
if socket, err = newSocket(address); err != nil {
69+
return nil, nil, err
70+
}
6371
}
64-
defer socket.Close()
72+
6573
f, err := socket.File()
6674
if err != nil {
6775
return nil, nil, errors.Wrapf(err, "failed to get fd for socket %s", address)
@@ -108,6 +116,8 @@ func WithStart(binary, address, daemonAddress, cgroup string, debug bool, exitHa
108116
if stderrLog != nil {
109117
stderrLog.Close()
110118
}
119+
socket.Close()
120+
RemoveSocket(address)
111121
}()
112122
log.G(ctx).WithFields(logrus.Fields{
113123
"pid": cmd.Process.Pid,
@@ -142,6 +152,26 @@ func WithStart(binary, address, daemonAddress, cgroup string, debug bool, exitHa
142152
}
143153
}
144154

155+
func eaddrinuse(err error) bool {
156+
cause := errors.Cause(err)
157+
netErr, ok := cause.(*net.OpError)
158+
if !ok {
159+
return false
160+
}
161+
if netErr.Op != "listen" {
162+
return false
163+
}
164+
syscallErr, ok := netErr.Err.(*os.SyscallError)
165+
if !ok {
166+
return false
167+
}
168+
errno, ok := syscallErr.Err.(syscall.Errno)
169+
if !ok {
170+
return false
171+
}
172+
return errno == syscall.EADDRINUSE
173+
}
174+
145175
// setupOOMScore gets containerd's oom score and adds +1 to it
146176
// to ensure a shim has a lower* score than the daemons
147177
func setupOOMScore(shimPid int) error {
@@ -214,31 +244,73 @@ func writeFile(path, address string) error {
214244
return os.Rename(tempPath, path)
215245
}
216246

247+
const (
248+
abstractSocketPrefix = "\x00"
249+
socketPathLimit = 106
250+
)
251+
252+
type socket string
253+
254+
func (s socket) isAbstract() bool {
255+
return !strings.HasPrefix(string(s), "unix://")
256+
}
257+
258+
func (s socket) path() string {
259+
path := strings.TrimPrefix(string(s), "unix://")
260+
// if there was no trim performed, we assume an abstract socket
261+
if len(path) == len(s) {
262+
path = abstractSocketPrefix + path
263+
}
264+
return path
265+
}
266+
217267
func newSocket(address string) (*net.UnixListener, error) {
218-
if len(address) > 106 {
219-
return nil, errors.Errorf("%q: unix socket path too long (> 106)", address)
268+
if len(address) > socketPathLimit {
269+
return nil, errors.Errorf("%q: unix socket path too long (> %d)", address, socketPathLimit)
270+
}
271+
var (
272+
sock = socket(address)
273+
path = sock.path()
274+
)
275+
if !sock.isAbstract() {
276+
if err := os.MkdirAll(filepath.Dir(path), 0600); err != nil {
277+
return nil, errors.Wrapf(err, "%s", path)
278+
}
220279
}
221-
l, err := net.Listen("unix", "\x00"+address)
280+
l, err := net.Listen("unix", path)
222281
if err != nil {
223-
return nil, errors.Wrapf(err, "failed to listen to abstract unix socket %q", address)
282+
return nil, errors.Wrapf(err, "failed to listen to unix socket %q (abstract: %t)", address, sock.isAbstract())
283+
}
284+
if err := os.Chmod(path, 0600); err != nil {
285+
l.Close()
286+
return nil, err
224287
}
225288

226289
return l.(*net.UnixListener), nil
227290
}
228291

292+
// RemoveSocket removes the socket at the specified address if
293+
// it exists on the filesystem
294+
func RemoveSocket(address string) error {
295+
sock := socket(address)
296+
if !sock.isAbstract() {
297+
return os.Remove(sock.path())
298+
}
299+
return nil
300+
}
301+
229302
func connect(address string, d func(string, time.Duration) (net.Conn, error)) (net.Conn, error) {
230303
return d(address, 100*time.Second)
231304
}
232305

233-
func annonDialer(address string, timeout time.Duration) (net.Conn, error) {
234-
address = strings.TrimPrefix(address, "unix://")
235-
return dialer.Dialer("\x00"+address, timeout)
306+
func anonDialer(address string, timeout time.Duration) (net.Conn, error) {
307+
return dialer.Dialer(socket(address).path(), timeout)
236308
}
237309

238310
// WithConnect connects to an existing shim
239311
func WithConnect(address string, onClose func()) Opt {
240312
return func(ctx context.Context, config shim.Config) (shimapi.ShimService, io.Closer, error) {
241-
conn, err := connect(address, annonDialer)
313+
conn, err := connect(address, anonDialer)
242314
if err != nil {
243315
return nil, nil, err
244316
}

Diff for: runtime/v2/runc/v1/service.go

+14-4
Original file line numberDiff line numberDiff line change
@@ -131,20 +131,26 @@ func (s *service) StartShim(ctx context.Context, id, containerdBinary, container
131131
if err != nil {
132132
return "", err
133133
}
134-
address, err := shim.SocketAddress(ctx, id)
134+
address, err := shim.SocketAddress(ctx, containerdAddress, id)
135135
if err != nil {
136136
return "", err
137137
}
138138
socket, err := shim.NewSocket(address)
139139
if err != nil {
140-
return "", err
140+
if !shim.SocketEaddrinuse(err) {
141+
return "", err
142+
}
143+
if err := shim.RemoveSocket(address); err != nil {
144+
return "", errors.Wrap(err, "remove already used socket")
145+
}
146+
if socket, err = shim.NewSocket(address); err != nil {
147+
return "", err
148+
}
141149
}
142-
defer socket.Close()
143150
f, err := socket.File()
144151
if err != nil {
145152
return "", err
146153
}
147-
defer f.Close()
148154

149155
cmd.ExtraFiles = append(cmd.ExtraFiles, f)
150156

@@ -153,6 +159,7 @@ func (s *service) StartShim(ctx context.Context, id, containerdBinary, container
153159
}
154160
defer func() {
155161
if err != nil {
162+
_ = shim.RemoveSocket(address)
156163
cmd.Process.Kill()
157164
}
158165
}()
@@ -551,6 +558,9 @@ func (s *service) Connect(ctx context.Context, r *taskAPI.ConnectRequest) (*task
551558
func (s *service) Shutdown(ctx context.Context, r *taskAPI.ShutdownRequest) (*ptypes.Empty, error) {
552559
s.cancel()
553560
close(s.events)
561+
if address, err := shim.ReadAddress("address"); err == nil {
562+
_ = shim.RemoveSocket(address)
563+
}
554564
return empty, nil
555565
}
556566

Diff for: runtime/v2/runc/v2/service.go

+33-10
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,6 @@ import (
2525
"os"
2626
"os/exec"
2727
"path/filepath"
28-
"strings"
2928
"sync"
3029
"syscall"
3130
"time"
@@ -105,6 +104,10 @@ func New(ctx context.Context, id string, publisher shim.Publisher, shutdown func
105104
return nil, errors.Wrap(err, "failed to initialized platform behavior")
106105
}
107106
go s.forward(ctx, publisher)
107+
108+
if address, err := shim.ReadAddress("address"); err == nil {
109+
s.shimAddress = address
110+
}
108111
return s, nil
109112
}
110113

@@ -124,7 +127,8 @@ type service struct {
124127

125128
containers map[string]*runc.Container
126129

127-
cancel func()
130+
shimAddress string
131+
cancel func()
128132
}
129133

130134
func newCommand(ctx context.Context, id, containerdBinary, containerdAddress, containerdTTRPCAddress string) (*exec.Cmd, error) {
@@ -183,30 +187,48 @@ func (s *service) StartShim(ctx context.Context, id, containerdBinary, container
183187
break
184188
}
185189
}
186-
address, err := shim.SocketAddress(ctx, grouping)
190+
address, err := shim.SocketAddress(ctx, containerdAddress, grouping)
187191
if err != nil {
188192
return "", err
189193
}
194+
190195
socket, err := shim.NewSocket(address)
191196
if err != nil {
192-
if strings.Contains(err.Error(), "address already in use") {
197+
// the only time where this would happen is if there is a bug and the socket
198+
// was not cleaned up in the cleanup method of the shim or we are using the
199+
// grouping functionality where the new process should be run with the same
200+
// shim as an existing container
201+
if !shim.SocketEaddrinuse(err) {
202+
return "", errors.Wrap(err, "create new shim socket")
203+
}
204+
if shim.CanConnect(address) {
193205
if err := shim.WriteAddress("address", address); err != nil {
194-
return "", err
206+
return "", errors.Wrap(err, "write existing socket for shim")
195207
}
196208
return address, nil
197209
}
198-
return "", err
210+
if err := shim.RemoveSocket(address); err != nil {
211+
return "", errors.Wrap(err, "remove pre-existing socket")
212+
}
213+
if socket, err = shim.NewSocket(address); err != nil {
214+
return "", errors.Wrap(err, "try create new shim socket 2x")
215+
}
199216
}
200-
defer socket.Close()
217+
defer func() {
218+
if retErr != nil {
219+
socket.Close()
220+
_ = shim.RemoveSocket(address)
221+
}
222+
}()
201223
f, err := socket.File()
202224
if err != nil {
203225
return "", err
204226
}
205-
defer f.Close()
206227

207228
cmd.ExtraFiles = append(cmd.ExtraFiles, f)
208229

209230
if err := cmd.Start(); err != nil {
231+
f.Close()
210232
return "", err
211233
}
212234
defer func() {
@@ -273,7 +295,6 @@ func (s *service) Cleanup(ctx context.Context) (*taskAPI.DeleteResponse, error)
273295
if err != nil {
274296
return nil, err
275297
}
276-
277298
runtime, err := runc.ReadRuntime(path)
278299
if err != nil {
279300
return nil, err
@@ -652,7 +673,9 @@ func (s *service) Shutdown(ctx context.Context, r *taskAPI.ShutdownRequest) (*pt
652673
if s.platform != nil {
653674
s.platform.Close()
654675
}
655-
676+
if s.shimAddress != "" {
677+
_ = shim.RemoveSocket(s.shimAddress)
678+
}
656679
return empty, nil
657680
}
658681

0 commit comments

Comments
 (0)