Skip to content

Commit 3a7d8dc

Browse files
Ensure safe access to _proc in ProcessInvoker in trace calls to prevent NullReference errors caused due to race condition when _proc is disposed during forceful cancellation/cleanup (#5327)
1 parent 41a793f commit 3a7d8dc

File tree

1 file changed

+14
-11
lines changed

1 file changed

+14
-11
lines changed

src/Agent.Sdk/ProcessInvoker.cs

Lines changed: 14 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -338,7 +338,7 @@ public async Task<int> ExecuteAsync(
338338
afterCancelKillProcessTreeAttemptSignal.Set();
339339
}))
340340
{
341-
Trace.Info($"Process started with process id {_proc.Id}, waiting for process exit.");
341+
Trace.Info($"Process started with process id {_proc?.Id ?? -1}, waiting for process exit.");
342342

343343
while (true)
344344
{
@@ -373,11 +373,11 @@ public async Task<int> ExecuteAsync(
373373

374374
if (_proc.HasExited)
375375
{
376-
Trace.Info($"Finished process {_proc.Id} with exit code {_proc.ExitCode}, and elapsed time {_stopWatch.Elapsed}.");
376+
Trace.Info($"Finished process {_proc?.Id ?? -1} with exit code {_proc?.ExitCode ?? -1}, and elapsed time {_stopWatch.Elapsed}.");
377377
}
378378
else
379379
{
380-
Trace.Info($"Process _proc.HasExited={_proc.HasExited}, Process ID={_proc.Id}, and elapsed time {_stopWatch.Elapsed}.");
380+
Trace.Info($"Process _proc.HasExited={_proc?.HasExited ?? false}, Process ID={_proc?.Id ?? -1}, and elapsed time {_stopWatch.Elapsed}.");
381381
}
382382
}
383383

@@ -459,13 +459,16 @@ internal protected virtual async Task CancelAndKillProcessTree(bool killProcessO
459459
{
460460
bool gracefulShoutdown = TryUseGracefulShutdown && !killProcessOnCancel;
461461

462-
ArgUtil.NotNull(_proc, nameof(_proc));
462+
// Store proc reference locally to prevent race conditions with Dispose()
463+
Process proc = _proc;
464+
ArgUtil.NotNull(proc, nameof(_proc));
465+
463466
if (!killProcessOnCancel)
464467
{
465468
bool sigint_succeed = await SendSIGINT(SigintTimeout);
466469
if (sigint_succeed)
467470
{
468-
Trace.Info($"Process {_proc.Id} cancelled successfully through Ctrl+C/SIGINT.");
471+
Trace.Info($"Process {proc.Id} cancelled successfully through Ctrl+C/SIGINT.");
469472
return;
470473
}
471474

@@ -477,12 +480,12 @@ internal protected virtual async Task CancelAndKillProcessTree(bool killProcessO
477480
bool sigterm_succeed = await SendSIGTERM(SigtermTimeout);
478481
if (sigterm_succeed)
479482
{
480-
Trace.Info($"Process {_proc.Id} terminate successfully through Ctrl+Break/SIGTERM.");
483+
Trace.Info($"Process {proc.Id} terminate successfully through Ctrl+Break/SIGTERM.");
481484
return;
482485
}
483486
}
484487

485-
Trace.Info($"[{_proc.Id}] Kill entire process tree with root {_proc.Id} since both cancel and terminate signal has been ignored by the target process.");
488+
Trace.Info($"[{proc.Id}] Kill entire process tree with root {proc.Id} since both cancel and terminate signal has been ignored by the target process.");
486489
KillProcessTree();
487490
}
488491

@@ -508,7 +511,7 @@ private async Task<bool> SendSIGTERM(TimeSpan timeout)
508511

509512
private void ProcessExitedHandler(object sender, EventArgs e)
510513
{
511-
Trace.Info($"Exited process {_proc.Id} with exit code {_proc.ExitCode}");
514+
Trace.Info($"Exited process {_proc?.Id ?? -1} with exit code {_proc?.ExitCode ?? -1}");
512515
if ((_proc.StartInfo.RedirectStandardError || _proc.StartInfo.RedirectStandardOutput) && _asyncStreamReaderCount != 0)
513516
{
514517
_waitingOnStreams = true;
@@ -545,7 +548,7 @@ private void StartReadStream(StreamReader reader, ConcurrentQueue<string> dataBu
545548
}
546549
}
547550

548-
Trace.Info($"[{_proc.Id}] STDOUT/STDERR stream read finished.");
551+
Trace.Info($"[{_proc?.Id ?? -1}] STDOUT/STDERR stream read finished.");
549552

550553
if (Interlocked.Decrement(ref _asyncStreamReaderCount) == 0 && _waitingOnStreams)
551554
{
@@ -575,15 +578,15 @@ private void StartWriteStream(InputQueue<string> redirectStandardIn, StreamWrite
575578

576579
if (!keepStandardInOpen)
577580
{
578-
Trace.Info($"[{_proc.Id}] Close STDIN after the first redirect finished.");
581+
Trace.Info($"[{_proc?.Id ?? -1}] Close STDIN after the first redirect finished.");
579582
standardIn.Close();
580583
break;
581584
}
582585
}
583586
}
584587
}
585588

586-
Trace.Info($"[{_proc.Id}] STDIN stream write finished.");
589+
Trace.Info($"[{_proc?.Id ?? -1}] STDIN stream write finished.");
587590
});
588591
}
589592

0 commit comments

Comments
 (0)