dotnet / dotnet/msbuild

Unresolvable task parameter type crashes with NullReferenceException instead of reporting a diagnostic

Open
#14,680 2 comments 0 reactions 1 assignee Claimed by @AlesProkop View on GitHub
Area: Engine Area: TaskHost bug Priority:1
Dominant language
C#
Stars
5.5k
Forks
1.5k
Avg merge
1d 8h
Merged PRs (30d)
141

Description

### Summary

When MSBuild cannot resolve a task parameter's type, it crashes with an unhandled `NullReferenceException` ("This is an unhandled exception in MSBuild -- PLEASE UPVOTE AN EXISTING ISSUE OR FILE A NEW ONE") instead of reporting a diagnostic that names the task and the offending parameter.

This is reachable today on a supported configuration: a `Runtime="NET"` task invoked from .NET Framework MSBuild. In that configuration the parent process must reflect over the task's parameters even though the task itself runs in the .NET task host, and that inspection can legitimately fail to resolve a parameter type (see #14681 for one concrete class of failure).

The crash is the bug being reported here, independently of *why* a given type fails to resolve. A user hitting this gets a stack trace with no indication of which task or which parameter is at fault.

### Repro

1. Author a task compiled only for .NET (`net11.0`) with a parameter whose type is not resolvable from .NET Framework — for example `public FileInfo DestinationFile { get; set; }`.
2. Register it for .NET Framework MSBuild:
```xml

```
3. Invoke the task from `MSBuild.exe` (.NET Framework).

Result: unhandled `NullReferenceException`. Expected: a build error naming the task and the parameter that could not be resolved.

### Two distinct crash sites

Both were observed while working on #14451.

**Site A — null hole in `LoadedType.Properties`**

`LoadedType`'s constructor pre-sizes the properties array and then `continue`s past any property whose type cannot be loaded, leaving a `null` element behind:

```csharp
PropertyInfo[] props = type.GetProperties(BindingFlags.Instance | BindingFlags.Public);
Properties = new ReflectableTaskPropertyInfo[props.Length];
...
try
{
pt = props[i].PropertyType;
...
}
catch (Exception e) when (!ExceptionHandling.IsCriticalException(e))
{
// Skip properties that can't be loaded
continue; // <-- leaves Properties[i] == null
}
```
(`src/Framework/Loader/LoadedType.cs`, array allocated ~L106, `continue` ~L148)

`TaskFactoryWrapper.PopulatePropertyInfo` then dereferences that `null`:

```
System.NullReferenceException: Object reference not set to an instance of an object.
at Microsoft.Build.Execution.ReflectableTaskPropertyInfo..ctor(TaskPropertyInfo taskPropertyInfo, Type taskType) in src/Framework/ReflectableTaskPropertyInfo.cs:line 39
at Microsoft.Build.Execution.TaskFactoryWrapper.PopulatePropertyInfo() in src/Build/Instance/TaskFactoryWrapper.cs:line 270
at System.Lazy`1.CreateValue()
at Microsoft.Build.Execution.TaskFactoryWrapper.get_GetNamesOfPropertiesWithRequiredAttribute() in src/Build/Instance/TaskFactoryWrapper.cs:line 137
at Microsoft.Build.BackEnd.TaskExecutionHost.GetNamesOfPropertiesWithRequiredAttribute() in src/Build/BackEnd/TaskExecutionHost/TaskExecutionHost.cs:line 2063
at Microsoft.Build.BackEnd.TaskExecutionHost.SetTaskParameters(IDictionary`2 parameters) in src/Build/BackEnd/TaskExecutionHost/TaskExecutionHost.cs:line 579
```

Note the `ReflectableTaskPropertyInfo` constructor already has `ArgumentNullException.ThrowIfNull(taskType)` for its *other* argument, but dereferences `taskPropertyInfo` unguarded in the base-constructor call, so the null is not caught at the boundary.

**Site B — `null` parameter type flows into `TaskParameterTypeVerifier`**

`TaskExecutionHost.ResolveTaskParameterType` can return `null` — either because `EnableReflectiveTaskExecution` is off, or because `Type.GetType(assemblyQualifiedName)` fails to bind the name in the current process:

```csharp
private static Type ResolveTaskParameterType(LoadedType loadedType, TaskPropertyInfo parameter, int indexOfParameter)
{
if (!loadedType.LoadedViaMetadataLoadContext)
{
return parameter.PropertyType;
}

if (FeatureSwitches.EnableReflectiveTaskExecution)
{
return ResolveTaskParameterTypeByName(loadedType, parameter, indexOfParameter);
}

return null; // <-- and Type.GetType above can also return null
}
```
(`src/Build/BackEnd/TaskExecutionHost/TaskExecutionHost.cs` L1511-L1537)

The caller passes the result straight to `TaskParameterTypeVerifier` without a null check:

```
System.NullReferenceException: Object reference not set to an instance of an object.
at Microsoft.Build.BackEnd.TaskParameterTypeVerifier.TryGetArrayElement(Type parameterType, Type& elementType) in src/Shared/TaskParameterTypeVerifier.cs:line 61
at Microsoft.Build.BackEnd.TaskParameterTypeVerifier.IsValidScalarInputParameter(Type parameterType) in src/Shared/TaskParameterTypeVerifier.cs:line 48
at Microsoft.Build.BackEnd.TaskExecutionHost.SetTaskParameter(String parameterName, String parameterValue, ElementLocation parameterLocation, Boolean isRequired, Boolean& parameterSet) in src/Build/BackEnd/TaskExecutionHost/TaskExecutionHost.cs:line 1428
at Microsoft.Build.BackEnd.TaskExecutionHost.SetTaskParameters(IDictionary`2 parameters) in src/Build/BackEnd/TaskExecutionHost/TaskExecutionHost.cs:line 589
```

A concrete way to hit Site B: a parameter whose type is nested inside the task class itself (for example a nested `enum`). Its assembly-qualified name is `MyTask+MyEnum, My.Tasks.Core, Version=15.1.0.0, ...`; from .NET Framework MSBuild `Type.GetType` binds that simple assembly name to the *.NET Framework* build of the tasks assembly, which does not contain the .NET-only task type, so the lookup returns `null`.

### Proposed fix

Fail observably instead of crashing:

- Do not leave `null` holes in `LoadedType.Properties`. Either build the array with a list and keep it dense, or record the unresolvable property explicitly so the engine can report it by name.
- Null-check the resolved `Type` in `TaskExecutionHost.SetTaskParameter` before handing it to `TaskParameterTypeVerifier`, and log an error identifying the task and the parameter.
- Consider defensively guarding `TaskParameterTypeVerifier` entry points against `null`.

The resulting diagnostic should name the task, the parameter, and ideally the type that failed to load, so the task author can act on it. Today there is no signal at all beyond a stack trace.

### Notes

- The `catch`/`continue` in `LoadedType` is deliberate — `MetadataLoadContext` inspection is expected to be best-effort — so the fix is about the *consumers* not tolerating the resulting hole, and about surfacing a diagnostic rather than silently degrading.
- Found while adding `TarDirectory`/`Untar` in #14451; that PR works around it by only exposing parameter types that resolve. The engine-side crash remains.

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.