Skip to content

Commit d082725

Browse files
dcantahdcantah
authored
Fix checkptr error with > 1 process in job object (microsoft#1284)
The `AllPids` method on JOBOBJECT_BASIC_PROCESS_ID_LIST would allocate a very large array to store any pids in the job object, cast the memory to this array, and then slice down to what elements it actually took up in the array based off the value of what was in the NumberOfProcessIdsInList field. The checkptr compile option doesn't like when slices don't have an explicit length and capacity so this change updates the slicing to use a three-index slice to set the capacity to the same as the length. Before for fmt.Println(len(arr), cap(arr)) -> 2, 134217727 After: -> 2, 2 This change additionally adds a test in internal/jobobject and modifies the TestExecWithJob test in internal/exec to verify that checkptr doesn't get angry when we hit a codepath that performs the cast Signed-off-by: Daniel Canter <[email protected]>
1 parent 134fdfa commit d082725

5 files changed

Lines changed: 84 additions & 16 deletions

File tree

‎internal/exec/exec_test.go‎

Lines changed: 49 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -130,8 +130,8 @@ func TestExecStdinPowershell(t *testing.T) {
130130
t.Logf("exit code was: %d", e.ExitCode())
131131
}
132132

133-
func TestExecWithJob(t *testing.T) {
134-
// Test that we can assign a process to a job object at creation time.
133+
func TestExecsWithJob(t *testing.T) {
134+
// Test that we can assign processes to a job object at creation time.
135135
job, err := jobobject.Create(context.Background(), &jobobject.Options{Name: "test"})
136136
if err != nil {
137137
log.Fatal(err)
@@ -140,9 +140,9 @@ func TestExecWithJob(t *testing.T) {
140140

141141
e, err := New(
142142
`C:\Windows\System32\ping.exe`,
143-
"ping 127.0.0.1",
143+
"ping -t 127.0.0.1",
144144
WithJobObject(job),
145-
WithStdio(true, false, false),
145+
WithStdio(false, false, false),
146146
WithEnv(os.Environ()),
147147
)
148148
if err != nil {
@@ -154,25 +154,62 @@ func TestExecWithJob(t *testing.T) {
154154
t.Fatalf("failed to start process: %v", err)
155155
}
156156

157-
pids, err := job.Pids()
157+
// Launch a second process and check pids.
158+
e2, err := New(
159+
`C:\Windows\System32\ping.exe`,
160+
"ping -t 127.0.0.1",
161+
WithJobObject(job),
162+
WithStdio(false, false, false),
163+
WithEnv(os.Environ()),
164+
)
158165
if err != nil {
159166
t.Fatal(err)
160167
}
161168

162-
if len(pids) == 0 {
163-
t.Fatal("no pids found in job object")
169+
err = e2.Start()
170+
if err != nil {
171+
t.Fatalf("failed to start process: %v", err)
172+
}
173+
174+
pidMap := map[int]struct{}{
175+
e.Pid(): {},
176+
e2.Pid(): {},
164177
}
165178

166-
// Should only be one process in the job, being the process we launched and added to it.
167-
if pids[0] != uint32(e.Pid()) {
179+
pids, err := job.Pids()
180+
if err != nil {
168181
t.Fatal(err)
169182
}
170183

171-
err = e.Wait()
184+
if len(pids) != 2 {
185+
t.Fatalf("should be two pids in job object, got: %d", len(pids))
186+
}
187+
188+
for _, pid := range pids {
189+
if _, ok := pidMap[int(pid)]; !ok {
190+
t.Fatalf("failed to find pid %d in job object", pid)
191+
}
192+
}
193+
194+
err = e.Kill()
172195
if err != nil {
173-
t.Fatalf("error waiting for process: %v", err)
196+
t.Fatalf("error killing process: %v", err)
197+
}
198+
199+
err = e2.Kill()
200+
if err != nil {
201+
t.Fatalf("error killing process: %v", err)
202+
}
203+
204+
_ = e.Wait()
205+
_ = e2.Wait()
206+
207+
if !e.Exited() {
208+
t.Fatalf("Process %d should have exited after kill", e.Pid())
209+
}
210+
if !e2.Exited() {
211+
t.Fatalf("Process %d should have exited after kill", e2.Pid())
174212
}
175-
t.Logf("exit code was: %d", e.ExitCode())
176213
}
177214

178215
func TestPseudoConsolePowershell(t *testing.T) {

‎internal/jobobject/jobobject.go‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -364,9 +364,8 @@ func (job *JobObject) Pids() ([]uint32, error) {
364364
}
365365

366366
bufInfo := (*winapi.JOBOBJECT_BASIC_PROCESS_ID_LIST)(unsafe.Pointer(&buf[0]))
367-
bufPids := bufInfo.AllPids()
368367
pids := make([]uint32, bufInfo.NumberOfProcessIdsInList)
369-
for i, bufPid := range bufPids {
368+
for i, bufPid := range bufInfo.AllPids() {
370369
pids[i] = uint32(bufPid)
371370
}
372371
return pids, nil

‎internal/jobobject/jobobject_test.go‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -261,3 +261,35 @@ func TestNoMoreProcessesMessageTerminate(t *testing.T) {
261261
t.Fatal("didn't receive no more processes message within timeout")
262262
}
263263
}
264+
265+
func TestVerifyPidCount(t *testing.T) {
266+
// This test verifies that job.Pids() returns the right info and works with > 1 process.
267+
options := &Options{
268+
Name: "test",
269+
Notifications: true,
270+
}
271+
job, err := Create(context.Background(), options)
272+
if err != nil {
273+
t.Fatal(err)
274+
}
275+
defer job.Close()
276+
277+
numProcs := 2
278+
_, err = createProcsAndAssign(numProcs, job)
279+
if err != nil {
280+
t.Fatal(err)
281+
}
282+
283+
pids, err := job.Pids()
284+
if err != nil {
285+
t.Fatal(err)
286+
}
287+
288+
if len(pids) != numProcs {
289+
t.Fatalf("expected %d processes in the job, got: %d", numProcs, len(pids))
290+
}
291+
292+
if err := job.Terminate(1); err != nil {
293+
t.Fatal(err)
294+
}
295+
}

‎internal/winapi/jobobject.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -93,7 +93,7 @@ type JOBOBJECT_BASIC_PROCESS_ID_LIST struct {
9393

9494
// AllPids returns all the process Ids in the job object.
9595
func (p *JOBOBJECT_BASIC_PROCESS_ID_LIST) AllPids() []uintptr {
96-
return (*[(1 << 27) - 1]uintptr)(unsafe.Pointer(&p.ProcessIdList[0]))[:p.NumberOfProcessIdsInList]
96+
return (*[(1 << 27) - 1]uintptr)(unsafe.Pointer(&p.ProcessIdList[0]))[:p.NumberOfProcessIdsInList:p.NumberOfProcessIdsInList]
9797
}
9898

9999
// https://docs.microsoft.com/en-us/windows/win32/api/winnt/ns-winnt-jobobject_basic_accounting_information

‎test/vendor/github.com/Microsoft/hcsshim/internal/winapi/jobobject.go‎

Lines changed: 1 addition & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)