msbuild: A crash in evaluator causes MSBuild to fail the build with 0 errors 0 warnings

We should harden MSBuild against crashes in various places to ensure that we get a decent behavior and report the original stack.

A sample exception I’m seeing here causes these symptoms:

Path.CheckInvalidPathChars Line 1394
Path.Combine Line 1204
FileUtilities.GetFullPath Line 667
LazyItemEvaluator`4.LazyItemList.ProcessNonWildCardItemUpdates Line 431
LazyItemEvaluator`4.LazyItemList.ComputeItems Line 402
LazyItemEvaluator`4.LazyItemList.GetItemData Line 290
LazyItemEvaluator`4.<>c.<GetAllItemsDeferred>b__26_0 Line 489
Enumerable.<SelectManyIterator>d__17`2.MoveNext
Buffer`1..ctor
OrderedEnumerable`1.<GetEnumerator>d__1.MoveNext
Evaluator`4.Evaluate Line 665
Evaluator`4.Evaluate Line 320
ProjectInstance.Initialize Line 2752
ProjectInstance..ctor Line 484
BuildRequestConfiguration.<>c__DisplayClass61_0.<LoadProjectIntoConfiguration>b__0 Line 474
BuildRequestConfiguration.InitializeProject Line 500
BuildRequestConfiguration.LoadProjectIntoConfiguration Line 433
RequestBuilder.<BuildProject>d__67.MoveNext Line 1118
AsyncTaskMethodBuilder`1.Start Line 472
RequestBuilder.BuildProject
RequestBuilder.<BuildAndReport>d__58.MoveNext Line 812
AsyncTaskMethodBuilder.Start Line 317
RequestBuilder.BuildAndReport
RequestBuilder.<RequestThreadProc>d__57.MoveNext Line 777
AsyncTaskMethodBuilder.Start Line 317
RequestBuilder.RequestThreadProc
RequestBuilder.<StartBuilderThread>b__52_2 Line 702
Task`1.InnerInvoke Line 680
Task.Execute Line 2499
Task.ExecutionContextCallback Line 2861
ExecutionContext.RunInternal Line 981
ExecutionContext.Run Line 928
Task.ExecuteWithThreadLocal Line 2827
Task.ExecuteEntry Line 2757
TaskScheduler.TryExecuteTask Line 458
RequestBuilder.DedicatedThreadsTaskScheduler.<InjectThread>b__6_0 Line 1411
ThreadHelper.ThreadStart_Context Line 69
ExecutionContext.RunInternal Line 981
ExecutionContext.Run Line 928
ExecutionContext.Run Line 917
ThreadHelper.ThreadStart Line 106

In the Path.Combine, the first argument is: C:\A\src\Shims and the second one is: \Users\kirill\AppData\Local\Microsoft\A\Temp\WebTooling\Schemas\JSON\Catalog\https

Throwing this ArgumentException from this location causes the problem: https://github.com/dotnet/msbuild/blob/d07c47adec8d5cf40718ef9a618b0b959cc8be0d/src/Build/Evaluation/LazyItemEvaluator.cs#L431

About this issue

Original URL
State: closed
Created 3 years ago
Comments: 17 (16 by maintainers)

Commits related to this issue

Merge pull request #7367 from Forgind/better-evaluator-errors Better evaluator errors Fixes #6460 — committed to dotnet/msbuild by Forgind 2 years ago

Most upvoted comments

I recommend you turn on first-chance exceptions in the debugger and step through building this project: https://github.com/dotnet/msbuild/issues/6460#issuecomment-841975519

You will see that the first catch block is here: https://github.com/dotnet/msbuild/blob/09bdfae164eac3b5c9027d803ffa513efaf91095/src/Build/BackEnd/Components/RequestBuilder/RequestBuilder.cs#L1107

The second catch block is here: https://github.com/dotnet/msbuild/blob/09bdfae164eac3b5c9027d803ffa513efaf91095/src/Build/BackEnd/Components/RequestBuilder/RequestBuilder.cs#L823

The problem is that this second catch block is effectively throwing away all non-critical exceptions.

I don’t care about this specific exception (invalid char). This is just a convenient representative of a class of exceptions in the build that will go unnoticed when they happen. As you correctly point out, there could be a plethora of these. We need to harden against all potential exceptions that bubble up to here - if an exception bubbles up to here, it’s a bug in the engine.

I think that this catch block needs to do three things:

ensure the full stack of the exception is logged (by logging an error I presume)
fail the entire build (I think the finally block does that already)
the error should probably include a prompt to open an issue at https://github.com/dotnet/msbuild/issues/new

try/catch around the Evaluator is probably not the right thing because if there’s an exception like this one, we probably don’t want to continue the build.

In addition to all that hardening, we need to fix this particular case at the source - this needs to happen here: https://github.com/dotnet/msbuild/blob/09bdfae164eac3b5c9027d803ffa513efaf91095/src/Build/Evaluation/LazyItemEvaluator.cs#L453

We should sanitize inputs before we call Path.Combine in FileUtilities.GetFullPath, since fileSpec is a string with a zero-char:

But it is vastly more important to harden the engine and safeguard against any non-critical exceptions, not just this one.

KirillOsenkov on Feb 4, 2022

HAH!!! I got it!

RequestBuilder catch block has a hole here for non-fatal exceptions: https://github.com/dotnet/msbuild/blob/d07c47adec8d5cf40718ef9a618b0b959cc8be0d/src/Build/BackEnd/Components/RequestBuilder/RequestBuilder.cs#L841

Modify the previous repro to use this project instead:

<Project>

  <ItemGroup Condition="$(IsInner) == true">
    <File Include="*" />
    <File Update="a" />
  </ItemGroup>

  <Target Name="Build">
    <MSBuild Projects="$(MSBuildThisFileFullPath)" Targets="Inner" Properties="IsInner=true"/>
  </Target>

  <Target Name="Inner">
  </Target>

</Project>

KirillOsenkov on May 17, 2021

OK, I have a minimal repro:

Paste this into 1.csproj:

<Project>

  <ItemGroup>
    <File Include="*" />
    <File Update="a" />
  </ItemGroup>

</Project>

In the same directory, create a file with the name %00. And build.

KirillOsenkov on May 17, 2021