Task analyzer: flag MSBuild tasks that spawn child processes with un-redirected stdout/stderr (#7913)
- Dominant language
- C#
- Stars
- 5.5k
- Forks
- 1.5k
- Avg merge
- 1d 8h
- Merged PRs (30d)
- 141
Description
## Summary
Extend the compile-time MSBuild task analyzer (`src/TaskAnalyzer`, `Microsoft.Build.TaskAuthoring.Analyzer`) so that authoring a task that spawns a child process **without redirecting the child's stdout/stderr** produces a warning at build time. This is the static-analysis counterpart to #7913: under the MSBuild Server (and detached worker nodes) a task's — and its child processes' — stdout is not attached to the user's console, so output is silently lost and on Linux the child can even fail/hang.
## Background: what #7913 is
A custom task that does e.g. `Process.Start(new ProcessStartInfo(dotnet, args))` (arcade's `InstallDotNetCore` is the canonical example) relies on the child inheriting the node's stdout. Worker nodes and the Server run headless (`CREATE_NO_WINDOW` / a pipe the client proxies), so:
- the child's output never reaches the terminal/file, and
- if enough is written, the pipe fills and the child hangs (observed on Linux, aspnetcore#43028).
The issue's own recommended fix (workaround **b**) is *"change custom tasks to execute the process with output redirected and capture those outputs"* — exactly the pattern a Roslyn analyzer can nudge authors toward at compile time, long before anyone debugs a lost-output-under-Server report.
## What the analyzer already covers today
Worth stating up front, because half of #7913 is already handled:
- **Direct console writes** — `Console.*` (every member, incl. `Console.Out` / `Console.Error` / `Console.OpenStandardOutput`) is already **MSBuildTask0001 = Error** on *every* `ITask` implementation, regardless of scope. Message: *"interferes with build logging; use Log.LogMessage instead."* ✅
- **`Process.Start` / `new ProcessStartInfo(...)`** — already **MSBuildTask0002 = Warning**, but:
- it's framed around *multithreading / env isolation* (*"use `TaskEnvironment.GetProcessStartInfo`"*), **not** the stdout-reachability problem #7913 is about;
- it's **scope-gated** (`msbuild_task_analyzer.scope=multithreadable_only` turns it off), whereas #7913 breaks **all** tasks under the Server, including single-threaded ones; and
- the suggested fix (`GetProcessStartInfo`) addresses env, not "redirect the output."
**The gap:** a task spawning a child process whose stdout/stderr is not redirected — reported for the right (server-reachability) reason, for **all** tasks, with actionable guidance.
## Proposal
Add a dedicated rule (proposed **MSBuildTask0006**, category `MSBuild.TaskAuthoring`, Warning, **always-on for all `ITask`** like the Console rule — *not* gated by the multithreadable scope) that fires when a task spawns an external process without redirecting output:
> Child processes spawned by a task inherit the build node's stdout/stderr, which are not attached to the console under the MSBuild Server or detached worker nodes (#7913). Set `RedirectStandardOutput`/`RedirectStandardError` and forward the output to `Log`, or use the `Exec` / `ToolTask` infrastructure which does this correctly.
**Charter note:** the analyzer is currently framed as "multithreadable task migration," but the Console rule already encodes a Server-relevant correctness concern for all tasks. This proposes explicitly widening the charter to "task authoring correctness (incl. Server compatibility)."
## Q1 — Can we reliably detect whether output is redirected? (partly)
The "safe" signal is a `ProcessStartInfo` with `RedirectStandardOutput = true` (usually `RedirectStandardError = true` and `UseShellExecute = false`) reaching `Process.Start`. Detectability tiers:
| Pattern | Reliability |
|---|---|
| `Process.Start(new ProcessStartInfo(...) { RedirectStandardOutput = true })` (initializer at the call) | **High** — inspect `IObjectCreationOperation.Initializer` directly |
| `var psi = new ProcessStartInfo(); psi.RedirectStandardOutput = true; Process.Start(psi);` / `p.StartInfo.X = true; p.Start();` | **Best-effort** — needs single-method dataflow (ControlFlowGraph / operation-block accumulation) |
| PSI returned from a helper, set conditionally, or built cross-assembly | **Not decidable** in general (interprocedural flow) — and this is exactly arcade's shape |
We **cannot prove** redirection in the general case. Two viable designs:
- **(A, recommended)** Always warn on process-spawning from a task, and *suppress* only when the immediately-associated initializer sets `RedirectStandardOutput = true`. No false negatives; the false-positive surface is small and trivially silenced by the same initializer authors should be writing anyway.
- **(B)** Warn only when we can see it's *not* redirected. Fewer false positives, but misses the imperative/cross-method cases — i.e. it would miss arcade.
**Caveat:** redirecting is necessary but not sufficient — the author still has to pump `OutputDataReceived`/`ErrorDataReceived` into `Log`. `ToolTask`/`Exec` already do this correctly, so steering authors there is the higher-value message.
## Q2 — Can we have a fixer?
- **`Console.*` → logging:** feasible but heuristic; there is currently no fixer for MSBuildTask0001. `Console.WriteLine(x)` → `Log.LogMessage(MessageImportance.High, x)` only works when the type derives from `Microsoft.Build.Utilities.Task`/`ToolTask` (so `Log` exists); a raw `ITask` needs `BuildEngine.LogMessageEvent(new BuildMessageEventArgs(...))`. Doable with a base-type check; mapping `Console.Error` is a judgment call. Reasonable follow-up.
- **`Process.Start` → redirect + pump:** **not** a safe automatic fix. Correct redirection is a multi-statement, behavior-changing rewrite (set 3 flags, subscribe 2 events, `BeginOutputReadLine`, avoid the sync-`ReadToEnd` deadlock). Best offered as *guidance* ("redirect and forward to `Log`, or convert to `ToolTask`/`Exec`") rather than an auto-rewrite. A "convert to Exec/ToolTask" refactoring is possible long-term but non-trivial.
## Suggested scope for a first PR
1. New always-on rule **MSBuildTask0006** using design (A): warn on `Process.Start`/`ProcessStartInfo` in a task; suppress when the adjacent initializer redirects stdout.
2. `AnalyzerReleases.Unshipped.md` + README + tests (mirroring the existing rule tests).
3. Follow-ups (separate): `Console`→`Log` fixer; "convert to Exec/ToolTask" guidance.
---
Refs: #7913 (arcade `InstallDotNetCore`; aspnetcore#43028 Linux failure). Analyzer lives at `src/TaskAnalyzer` (`Microsoft.Build.TaskAuthoring.Analyzer`, not yet shipped — currently made available to partner repos only).
Contributor guide
No contributing guide indexed for this repository
Research direction
Start in src/TaskAnalyzer and inspect the existing MSBuildTask0001/0002 implementations and their rule tests. Add MSBuildTask0006 with the proposed initializer-based suppression, update AnalyzerReleases.Unshipped.md and the README, then run the analyzer tests to verify warnings for unredirected process output and suppression for redirected output.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- csharp
- Domain
- build-system, tooling
- Issue type
- Feature
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 68/100