Skip to content

Commit 12e5d91

Browse files
helsaawyhelsaawy
committed
Check for invalid handle values, callback code cleanup
Validate that Handles are also not `Invalid` (i.e., `uintptr - 1`), since Win32 APIs may feasibly return either invalid or zero handles on error. Add a dedicated callback number counter atomic, since it doesn't need to be protected by `callbackMapLock`. Add debug logs with (un)registered notification number and compute system/process details. Add documentation on current `callbackMap` races. Signed-off-by: Hamza El-Saawy <hamzaelsaawy@microsoft.com>
1 parent 14286c6 commit 12e5d91

8 files changed

Lines changed: 103 additions & 38 deletions

File tree

‎internal/hcs/callback.go‎

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,23 @@ import (
1818
)
1919

2020
var (
21-
nextCallback uintptr
21+
// TODO: refactor callback number to be a uint and not a uintptr
22+
23+
nextCallback callbackCounter
24+
25+
// `callbackMapLock` is used to protect the map itself, and not the values in the map.
26+
// This causes a race condition with `(*notificationWatcherContext).handle`,
27+
// where, if a computeSystem or process is closed immediately after it is opened, then
28+
// the handle may not be updated with the result from
29+
// `vmcompute.HcsRegister[ComputeSystem|Process]Callback` in `registerCallback` when
30+
// `unregisterCallback` is called.
31+
// Similarly with `(*notificationWatcherContext).channels`, if a `unregisterCallback`
32+
// is called while `waitForNotification` or `notificationWatcher` are processing a notification,
33+
// then the former may close the notification channels after the latter two have read
34+
// (and retain a pointer to) the `notificationWatcherContext`, resulting in a send on a closed channel.
35+
// TODO: use a per-context [RW]Mutex to fix the above scenarios.
36+
37+
// protected by [callbackMapLock].
2238
callbackMap = map[uintptr]*notificationWatcherContext{}
2339
callbackMapLock = sync.RWMutex{}
2440

@@ -156,7 +172,7 @@ func notificationWatcher(
156172
callbackMapLock.RUnlock()
157173

158174
if callbackCtx == nil {
159-
entry.WithField("callbackNumber", callbackNumber).Warn("Received notification for unknown callback number")
175+
entry.WithField("callbackNumber", callbackNumber).Warn("received notification for unknown callback number")
160176
return 0
161177
}
162178

‎internal/hcs/process.go‎

Lines changed: 25 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import (
1212
"syscall"
1313
"time"
1414

15+
"github.com/sirupsen/logrus"
1516
"go.opencensus.io/trace"
1617

1718
"github.com/Microsoft/hcsshim/internal/cow"
@@ -20,6 +21,7 @@ import (
2021
"github.com/Microsoft/hcsshim/internal/oc"
2122
"github.com/Microsoft/hcsshim/internal/protocol/guestrequest"
2223
"github.com/Microsoft/hcsshim/internal/vmcompute"
24+
"github.com/Microsoft/hcsshim/internal/winapi"
2325
)
2426

2527
type Process struct {
@@ -97,7 +99,7 @@ func (process *Process) Signal(ctx context.Context, options interface{}) (bool,
9799

98100
operation := "hcs::Process::Signal"
99101

100-
if process.handle == 0 {
102+
if winapi.IsInvalidHandle(process.handle) {
101103
return false, makeProcessError(process, operation, ErrAlreadyClosed, nil)
102104
}
103105

@@ -122,7 +124,7 @@ func (process *Process) Kill(ctx context.Context) (bool, error) {
122124

123125
operation := "hcs::Process::Kill"
124126

125-
if process.handle == 0 {
127+
if winapi.IsInvalidHandle(process.handle) {
126128
return false, makeProcessError(process, operation, ErrAlreadyClosed, nil)
127129
}
128130

@@ -287,7 +289,7 @@ func (process *Process) ResizeConsole(ctx context.Context, width, height uint16)
287289

288290
operation := "hcs::Process::ResizeConsole"
289291

290-
if process.handle == 0 {
292+
if winapi.IsInvalidHandle(process.handle) {
291293
return makeProcessError(process, operation, ErrAlreadyClosed, nil)
292294
}
293295
modifyRequest := hcsschema.ProcessModifyRequest{
@@ -339,7 +341,7 @@ func (process *Process) StdioLegacy() (_ io.WriteCloser, _ io.ReadCloser, _ io.R
339341
process.handleLock.RLock()
340342
defer process.handleLock.RUnlock()
341343

342-
if process.handle == 0 {
344+
if winapi.IsInvalidHandle(process.handle) {
343345
return nil, nil, nil, makeProcessError(process, operation, ErrAlreadyClosed, nil)
344346
}
345347

@@ -388,7 +390,7 @@ func (process *Process) CloseStdin(ctx context.Context) (err error) {
388390
process.handleLock.RLock()
389391
defer process.handleLock.RUnlock()
390392

391-
if process.handle == 0 {
393+
if winapi.IsInvalidHandle(process.handle) {
392394
return makeProcessError(process, operation, ErrAlreadyClosed, nil)
393395
}
394396

@@ -434,7 +436,7 @@ func (process *Process) CloseStdout(ctx context.Context) (err error) {
434436
process.handleLock.Lock()
435437
defer process.handleLock.Unlock()
436438

437-
if process.handle == 0 {
439+
if winapi.IsInvalidHandle(process.handle) {
438440
return nil
439441
}
440442

@@ -458,7 +460,7 @@ func (process *Process) CloseStderr(ctx context.Context) (err error) {
458460
process.handleLock.Lock()
459461
defer process.handleLock.Unlock()
460462

461-
if process.handle == 0 {
463+
if winapi.IsInvalidHandle(process.handle) {
462464
return nil
463465
}
464466

@@ -486,7 +488,7 @@ func (process *Process) Close() (err error) {
486488
defer process.handleLock.Unlock()
487489

488490
// Don't double free this
489-
if process.handle == 0 {
491+
if winapi.IsInvalidHandle(process.handle) {
490492
return nil
491493
}
492494

@@ -524,15 +526,21 @@ func (process *Process) Close() (err error) {
524526
}
525527

526528
func (process *Process) registerCallback(ctx context.Context) error {
529+
callbackNumber := nextCallback.Inc()
530+
531+
log.G(ctx).WithFields(logrus.Fields{
532+
"cid": process.SystemID(),
533+
"pid": process.processID,
534+
"callbackNumber": callbackNumber,
535+
}).Debug("register process callback")
536+
527537
callbackContext := &notificationWatcherContext{
528538
channels: newProcessChannels(),
529539
systemID: process.SystemID(),
530540
processID: process.processID,
531541
}
532542

533543
callbackMapLock.Lock()
534-
callbackNumber := nextCallback
535-
nextCallback++
536544
callbackMap[callbackNumber] = callbackContext
537545
callbackMapLock.Unlock()
538546

@@ -549,6 +557,12 @@ func (process *Process) registerCallback(ctx context.Context) error {
549557
func (process *Process) unregisterCallback(ctx context.Context) error {
550558
callbackNumber := process.callbackNumber
551559

560+
log.G(ctx).WithFields(logrus.Fields{
561+
"cid": process.SystemID(),
562+
"pid": process.processID,
563+
"callbackNumber": callbackNumber,
564+
}).Debug("unregister process callback")
565+
552566
callbackMapLock.RLock()
553567
callbackContext := callbackMap[callbackNumber]
554568
callbackMapLock.RUnlock()
@@ -559,7 +573,7 @@ func (process *Process) unregisterCallback(ctx context.Context) error {
559573

560574
handle := callbackContext.handle
561575

562-
if handle == 0 {
576+
if winapi.IsInvalidHandle(handle) {
563577
return nil
564578
}
565579

‎internal/hcs/system.go‎

Lines changed: 26 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ import (
2121
"github.com/Microsoft/hcsshim/internal/oc"
2222
"github.com/Microsoft/hcsshim/internal/timeout"
2323
"github.com/Microsoft/hcsshim/internal/vmcompute"
24+
"github.com/Microsoft/hcsshim/internal/winapi"
2425
"github.com/sirupsen/logrus"
2526
"go.opencensus.io/trace"
2627
)
@@ -206,7 +207,7 @@ func (computeSystem *System) Start(ctx context.Context) (err error) {
206207

207208
// prevent starting an exited system because waitblock we do not recreate waitBlock
208209
// or rerun waitBackground, so we have no way to be notified of it closing again
209-
if computeSystem.handle == 0 {
210+
if winapi.IsInvalidHandle(computeSystem.handle) {
210211
return makeSystemError(computeSystem, operation, ErrAlreadyClosed, nil)
211212
}
212213

@@ -232,7 +233,7 @@ func (computeSystem *System) Shutdown(ctx context.Context) error {
232233

233234
operation := "hcs::System::Shutdown"
234235

235-
if computeSystem.handle == 0 || computeSystem.stopped() {
236+
if winapi.IsInvalidHandle(computeSystem.handle) || computeSystem.stopped() {
236237
return nil
237238
}
238239

@@ -254,7 +255,7 @@ func (computeSystem *System) Terminate(ctx context.Context) error {
254255

255256
operation := "hcs::System::Terminate"
256257

257-
if computeSystem.handle == 0 || computeSystem.stopped() {
258+
if winapi.IsInvalidHandle(computeSystem.handle) || computeSystem.stopped() {
258259
return nil
259260
}
260261

@@ -351,7 +352,7 @@ func (computeSystem *System) Properties(ctx context.Context, types ...schema1.Pr
351352

352353
operation := "hcs::System::Properties"
353354

354-
if computeSystem.handle == 0 {
355+
if winapi.IsInvalidHandle(computeSystem.handle) {
355356
return nil, makeSystemError(computeSystem, operation, ErrAlreadyClosed, nil)
356357
}
357358

@@ -492,7 +493,7 @@ func (computeSystem *System) statisticsInProc(job *jobobject.JobObject) (*hcssch
492493
func (computeSystem *System) hcsPropertiesV2Query(ctx context.Context, types []hcsschema.PropertyType) (*hcsschema.Properties, error) {
493494
operation := "hcs::System::PropertiesV2"
494495

495-
if computeSystem.handle == 0 {
496+
if winapi.IsInvalidHandle(computeSystem.handle) {
496497
return nil, makeSystemError(computeSystem, operation, ErrAlreadyClosed, nil)
497498
}
498499

@@ -586,7 +587,7 @@ func (computeSystem *System) Pause(ctx context.Context) (err error) {
586587
computeSystem.handleLock.RLock()
587588
defer computeSystem.handleLock.RUnlock()
588589

589-
if computeSystem.handle == 0 {
590+
if winapi.IsInvalidHandle(computeSystem.handle) {
590591
return makeSystemError(computeSystem, operation, ErrAlreadyClosed, nil)
591592
}
592593

@@ -614,7 +615,7 @@ func (computeSystem *System) Resume(ctx context.Context) (err error) {
614615
computeSystem.handleLock.RLock()
615616
defer computeSystem.handleLock.RUnlock()
616617

617-
if computeSystem.handle == 0 {
618+
if winapi.IsInvalidHandle(computeSystem.handle) {
618619
return makeSystemError(computeSystem, operation, ErrAlreadyClosed, nil)
619620
}
620621

@@ -647,7 +648,7 @@ func (computeSystem *System) Save(ctx context.Context, options interface{}) (err
647648
computeSystem.handleLock.RLock()
648649
defer computeSystem.handleLock.RUnlock()
649650

650-
if computeSystem.handle == 0 {
651+
if winapi.IsInvalidHandle(computeSystem.handle) {
651652
return makeSystemError(computeSystem, operation, ErrAlreadyClosed, nil)
652653
}
653654

@@ -665,7 +666,7 @@ func (computeSystem *System) createProcess(ctx context.Context, operation string
665666
computeSystem.handleLock.RLock()
666667
defer computeSystem.handleLock.RUnlock()
667668

668-
if computeSystem.handle == 0 {
669+
if winapi.IsInvalidHandle(computeSystem.handle) {
669670
return nil, nil, makeSystemError(computeSystem, operation, ErrAlreadyClosed, nil)
670671
}
671672

@@ -727,7 +728,7 @@ func (computeSystem *System) OpenProcess(ctx context.Context, pid int) (*Process
727728

728729
operation := "hcs::System::OpenProcess"
729730

730-
if computeSystem.handle == 0 {
731+
if winapi.IsInvalidHandle(computeSystem.handle) {
731732
return nil, makeSystemError(computeSystem, operation, ErrAlreadyClosed, nil)
732733
}
733734

@@ -766,7 +767,7 @@ func (computeSystem *System) CloseCtx(ctx context.Context) (err error) {
766767
defer computeSystem.handleLock.Unlock()
767768

768769
// Don't double free this
769-
if computeSystem.handle == 0 {
770+
if winapi.IsInvalidHandle(computeSystem.handle) {
770771
return nil
771772
}
772773

@@ -789,14 +790,19 @@ func (computeSystem *System) CloseCtx(ctx context.Context) (err error) {
789790
}
790791

791792
func (computeSystem *System) registerCallback(ctx context.Context) error {
793+
callbackNumber := nextCallback.Inc()
794+
795+
log.G(ctx).WithFields(logrus.Fields{
796+
"cid": computeSystem.id,
797+
"callbackNumber": callbackNumber,
798+
}).Debug("register computer system callback")
799+
792800
callbackContext := &notificationWatcherContext{
793801
channels: newSystemChannels(),
794802
systemID: computeSystem.id,
795803
}
796804

797805
callbackMapLock.Lock()
798-
callbackNumber := nextCallback
799-
nextCallback++
800806
callbackMap[callbackNumber] = callbackContext
801807
callbackMapLock.Unlock()
802808

@@ -814,6 +820,11 @@ func (computeSystem *System) registerCallback(ctx context.Context) error {
814820
func (computeSystem *System) unregisterCallback(ctx context.Context) error {
815821
callbackNumber := computeSystem.callbackNumber
816822

823+
log.G(ctx).WithFields(logrus.Fields{
824+
"cid": computeSystem.id,
825+
"callbackNumber": callbackNumber,
826+
}).Debug("unregister computer system callback")
827+
817828
callbackMapLock.RLock()
818829
callbackContext := callbackMap[callbackNumber]
819830
callbackMapLock.RUnlock()
@@ -824,7 +835,7 @@ func (computeSystem *System) unregisterCallback(ctx context.Context) error {
824835

825836
handle := callbackContext.handle
826837

827-
if handle == 0 {
838+
if winapi.IsInvalidHandle(handle) {
828839
return nil
829840
}
830841

@@ -853,7 +864,7 @@ func (computeSystem *System) Modify(ctx context.Context, config interface{}) err
853864

854865
operation := "hcs::System::Modify"
855866

856-
if computeSystem.handle == 0 {
867+
if winapi.IsInvalidHandle(computeSystem.handle) {
857868
return makeSystemError(computeSystem, operation, ErrAlreadyClosed, nil)
858869
}
859870

‎internal/hcs/utils.go‎

Lines changed: 22 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,21 +5,25 @@ package hcs
55
import (
66
"context"
77
"io"
8+
"sync/atomic"
89
"syscall"
910

11+
"github.com/pkg/errors"
12+
"golang.org/x/sys/windows"
13+
1014
"github.com/Microsoft/go-winio"
1115
diskutil "github.com/Microsoft/go-winio/vhd"
16+
1217
"github.com/Microsoft/hcsshim/computestorage"
13-
"github.com/pkg/errors"
14-
"golang.org/x/sys/windows"
18+
"github.com/Microsoft/hcsshim/internal/winapi"
1519
)
1620

1721
// makeOpenFiles calls winio.NewOpenFile for each handle in a slice but closes all the handles
1822
// if there is an error.
1923
func makeOpenFiles(hs []syscall.Handle) (_ []io.ReadWriteCloser, err error) {
2024
fs := make([]io.ReadWriteCloser, len(hs))
2125
for i, h := range hs {
22-
if h != syscall.Handle(0) {
26+
if !winapi.IsInvalidHandle(h) {
2327
if err == nil {
2428
fs[i], err = winio.NewOpenFile(windows.Handle(h))
2529
}
@@ -62,3 +66,18 @@ func CreateNTFSVHD(ctx context.Context, vhdPath string, sizeGB uint32) (err erro
6266

6367
return nil
6468
}
69+
70+
// TODO: move these to a dedicated `internal/sync` package
71+
72+
type callbackCounter struct {
73+
v atomic.Uintptr
74+
}
75+
76+
func (c *callbackCounter) Inc() uintptr { return (&c.v).Add(1) }
77+
78+
// noCopy prevents structs from being copied.
79+
// See: https://golang.org/issues/8005#issuecomment-190753527
80+
type noCopy struct{}
81+
82+
func (*noCopy) Lock() {}
83+
func (*noCopy) Unlock() {}

‎internal/hcs/waithelper.go‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -32,13 +32,14 @@ func waitForNotification(
3232
timeout *time.Duration,
3333
) error {
3434
callbackMapLock.RLock()
35-
if _, ok := callbackMap[callbackNumber]; !ok {
36-
callbackMapLock.RUnlock()
35+
callbackCtx, ok := callbackMap[callbackNumber]
36+
callbackMapLock.RUnlock()
37+
38+
if !ok {
3739
log.G(ctx).WithField("callbackNumber", callbackNumber).Error("failed to waitForNotification: callbackNumber does not exist in callbackMap")
3840
return ErrHandleClose
3941
}
40-
channels := callbackMap[callbackNumber].channels
41-
callbackMapLock.RUnlock()
42+
channels := callbackCtx.channels
4243

4344
expectedChannel := channels[expectedNotification]
4445
if expectedChannel == nil {

‎internal/winapi/doc.go‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
11
// Package winapi contains various low-level bindings to Windows APIs. It can
22
// be thought of as an extension to golang.org/x/sys/windows.
33
package winapi
4+
5+
//go:generate go tool github.com/Microsoft/go-winio/tools/mkwinsyscall -output zsyscall_windows.go ./*.go

0 commit comments

Comments
 (0)