Skip to content

Commit

Permalink
Removing last instances of the BinaryFormatter (#11346)
Browse files Browse the repository at this point in the history 
* The last instances of the BinaryFormatter have been removed

* Update src/Build/BackEnd/Components/Logging/LoggingService.cs

Co-authored-by: Rainer Sigwald <raines@microsoft.com>

* Update src/Shared/Resources/Strings.shared.resx

Co-authored-by: Rainer Sigwald <raines@microsoft.com>

* Fix for CA1823 and SA1508

* Xlf update

* NetFx version emits warning instead of error

---------

Co-authored-by: Rainer Sigwald <raines@microsoft.com>
  • Loading branch information
@MichalPavlik @rainersigwald
MichalPavlik and rainersigwald authored Feb 12, 2025
1 parent 0b44ec8 commit 79d15bc
Show file tree
Hide file tree
Showing 21 changed files with 97 additions and 305 deletions.
42 changes: 13 additions & 29 deletions src/Build.UnitTests/BackEnd/BuildManager_Logging_Tests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -78,28 +78,19 @@ public BuildManager_Logging_Tests(ITestOutputHelper output)
_env = TestEnvironment.Create(output);
}

[DotNetOnlyTheory]
[InlineData("1", true)]
// [InlineData("0", true)] <-- explicitly opting out on core will lead to node crash (as documented)
[InlineData(null, true)]
public void Build_WithCustomBuildArgs_NetCore(string envVariableValue, bool isWarningExpected)
=> TestCustomEventWarning<BuildErrorEventArgs>(envVariableValue, isWarningExpected);

[WindowsFullFrameworkOnlyTheory]
[InlineData("1", true)]
[InlineData("0", false)]
[InlineData(null, true)]
public void Build_WithCustomBuildArgs_Framework(string envVariableValue, bool isWarningExpected) =>
TestCustomEventWarning<BuildWarningEventArgs>(envVariableValue, isWarningExpected);

private void TestCustomEventWarning<T>(string envVariableValue, bool isWarningExpected) where T : LazyFormattedBuildEventArgs
[DotNetOnlyFact]
public void Build_WithCustomBuildArgs_ShouldEmitErrorOnNetCore() => Build_WithCustomBuildArgs_ShouldEmitEvent<BuildErrorEventArgs>();

[WindowsFullFrameworkOnlyFact]
public void Build_WithCustomBuildArgs_ShouldEmitWarningOnFramework() => Build_WithCustomBuildArgs_ShouldEmitEvent<BuildWarningEventArgs>();

private void Build_WithCustomBuildArgs_ShouldEmitEvent<T>() where T : LazyFormattedBuildEventArgs
{
var testFiles = _env.CreateTestProjectWithFiles(string.Empty, new[] { "main", "child1" }, string.Empty);
var testFiles = _env.CreateTestProjectWithFiles(string.Empty, ["main", "child1"], string.Empty);

ILoggingService service = LoggingService.CreateLoggingService(LoggerMode.Synchronous, 1);
service.RegisterLogger(_logger);

_env.SetEnvironmentVariable("MSBUILDCUSTOMBUILDEVENTWARNING", envVariableValue);
_env.SetEnvironmentVariable("MSBUILDNOINPROCNODE", "1");

_buildManager.BeginBuild(BuildParameters);
Expand All @@ -118,24 +109,17 @@ private void TestCustomEventWarning<T>(string envVariableValue, bool isWarningEx
mainProjectPath,
new Dictionary<string, string>(),
MSBuildConstants.CurrentToolsVersion,
new[] { "MainTarget" },
["MainTarget"],
null);

var submission = _buildManager.PendBuildRequest(buildRequestData);
var result = submission.Execute();
var allEvents = _logger.AllBuildEvents;

if (isWarningExpected)
{
allEvents.OfType<T>().ShouldHaveSingleItem();
allEvents.First(x => x is T).Message.ShouldContain(
string.Format(ResourceUtilities.GetResourceString("DeprecatedEventSerialization"),
"MyCustomBuildEventArgs"));
}
else
{
allEvents.OfType<T>().ShouldBeEmpty();
}
allEvents.OfType<T>().ShouldHaveSingleItem();
allEvents.First(x => x is T).Message.ShouldContain(
string.Format(ResourceUtilities.GetResourceString("DeprecatedEventSerialization"),
"MyCustomBuildEventArgs"));
}
finally
{
Expand Down
49 changes: 1 addition & 48 deletions src/Build/BackEnd/Components/Logging/LoggingService.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -980,58 +980,11 @@ public void PacketReceived(int node, INodePacket packet)
LogMessagePacket loggingPacket = (LogMessagePacket)packet;
InjectNonSerializedData(loggingPacket);

WarnOnDeprecatedCustomArgsSerialization(loggingPacket);
ErrorUtilities.VerifyThrow(loggingPacket.EventType != LoggingEventType.CustomEvent, "Custom event types are no longer supported. Does the sending node have a different version?");

ProcessLoggingEvent(loggingPacket.NodeBuildEvent);
}

/// <summary>
/// Serializing unknown CustomEvent which has to use unsecure BinaryFormatter by TranslateDotNet.
/// Since BinaryFormatter is going to be deprecated, log warning so users can use new Extended*EventArgs instead of custom
/// EventArgs derived from existing EventArgs.
/// </summary>
private void WarnOnDeprecatedCustomArgsSerialization(LogMessagePacket loggingPacket)
{
if (loggingPacket.EventType == LoggingEventType.CustomEvent
&& Traits.Instance.EscapeHatches.EnableWarningOnCustomBuildEvent)
{
BuildEventArgs buildEvent = loggingPacket.NodeBuildEvent.Value.Value;
BuildEventContext buildEventContext = buildEvent?.BuildEventContext ?? BuildEventContext.Invalid;

string message = ResourceUtilities.FormatResourceStringStripCodeAndKeyword(
out string warningCode,
out string helpKeyword,
"DeprecatedEventSerialization",
buildEvent?.GetType().Name ?? string.Empty);

BuildWarningEventArgs warning = new(
null,
warningCode,
BuildEventFileInfo.Empty.File,
BuildEventFileInfo.Empty.Line,
BuildEventFileInfo.Empty.Column,
BuildEventFileInfo.Empty.EndLine,
BuildEventFileInfo.Empty.EndColumn,
message,
helpKeyword,
"MSBuild");

warning.BuildEventContext = buildEventContext;
if (warning.ProjectFile == null && buildEventContext.ProjectContextId != BuildEventContext.InvalidProjectContextId)
{
warning.ProjectFile = buildEvent switch
{
BuildMessageEventArgs buildMessageEvent => buildMessageEvent.ProjectFile,
BuildErrorEventArgs buildErrorEvent => buildErrorEvent.ProjectFile,
BuildWarningEventArgs buildWarningEvent => buildWarningEvent.ProjectFile,
_ => null,
};
}

ProcessLoggingEvent(warning);
}
}

/// <summary>
/// Register an instantiated logger which implements the ILogger interface. This logger will be registered to a specific event
/// source (the central logger event source) which will receive all logging messages for a given build.
Expand Down
18 changes: 9 additions & 9 deletions src/Build/BackEnd/Node/OutOfProcNode.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -583,27 +583,27 @@ private void SendPacket(INodePacket packet)
{
if (_nodeEndpoint.LinkStatus == LinkStatus.Active)
{
#if RUNTIME_TYPE_NETCORE
if (packet is LogMessagePacketBase logMessage
&& logMessage.EventType == LoggingEventType.CustomEvent
&& Traits.Instance.EscapeHatches.EnableWarningOnCustomBuildEvent)
&& logMessage.EventType == LoggingEventType.CustomEvent)
{
BuildEventArgs buildEvent = logMessage.NodeBuildEvent.Value.Value;

// Serializing unknown CustomEvent which has to use unsecure BinaryFormatter by TranslateDotNet<T>
// Since BinaryFormatter is deprecated in dotnet 8+, log error so users discover root cause easier
// then by reading CommTrace where it would be otherwise logged as critical infra error.
_loggingService.LogError(_loggingContext?.BuildEventContext ?? BuildEventContext.Invalid, null, BuildEventFileInfo.Empty,
"DeprecatedEventSerialization",
buildEvent?.GetType().Name ?? string.Empty);
#if RUNTIME_TYPE_NETCORE
_loggingService.LogError(
#else
_loggingService.LogWarning(
#endif
_loggingContext?.BuildEventContext ?? BuildEventContext.Invalid, null, BuildEventFileInfo.Empty,
"DeprecatedEventSerialization",
buildEvent?.GetType().Name ?? string.Empty);
}
else
{
_nodeEndpoint.SendData(packet);
}
#else
_nodeEndpoint.SendData(packet);
#endif
}
}

Expand Down
36 changes: 0 additions & 36 deletions src/Framework/BinaryTranslator.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -493,22 +493,6 @@ public void TranslateEnum<T>(ref T value, int numericValue)
value = (T)Enum.ToObject(enumType, numericValue);
}

/// <summary>
/// Translates a value using the .Net binary formatter.
/// </summary>
/// <typeparam name="T">The reference type.</typeparam>
/// <param name="value">The value to be translated.</param>
public void TranslateDotNet<T>(ref T value)
{
if (!TranslateNullable(value))
{
return;
}

BinaryFormatter formatter = new BinaryFormatter();
value = (T)formatter.Deserialize(_packetStream);
}

public void TranslateException(ref Exception value)
{
if (!TranslateNullable(value))
Expand Down Expand Up @@ -1190,26 +1174,6 @@ public void TranslateEnum<T>(ref T value, int numericValue)
_writer.Write(numericValue);
}

/// <summary>
/// Translates a value using the .Net binary formatter.
/// </summary>
/// <typeparam name="T">The reference type.</typeparam>
/// <param name="value">The value to be translated.</param>
public void TranslateDotNet<T>(ref T value)
{
// All the calling paths are already guarded by ChangeWaves.Wave17_10 - so it's a no-op adding it here as well.
// But let's have it here explicitly - so it's clearer for the CodeQL reviewers.
if (!TranslateNullable(value) || !ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave17_10))
{
return;
}

// codeql[cs/dangerous-binary-deserialization] This code needs explicit opt-in to be used (ChangeWaves.Wave17_10). This exists as a temporary compat opt-in for old 3rd party loggers, before they are migrated based on documented guidance.
// The opt-in documentation: https://github.com/dotnet/msbuild/blob/main/documentation/wiki/ChangeWaves.md#1710
BinaryFormatter formatter = new BinaryFormatter();
formatter.Serialize(_packetStream, value);
}

public void TranslateException(ref Exception value)
{
if (!TranslateNullable(value))
Expand Down
12 changes: 0 additions & 12 deletions src/Framework/ITranslator.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -255,18 +255,6 @@ BinaryWriter Writer
void TranslateEnum<T>(ref T value, int numericValue)
where T : struct, Enum;

/// <summary>
/// Translates a value using the .Net binary formatter.
/// </summary>
/// <typeparam name="T">The reference type.</typeparam>
/// <param name="value">The value to be translated.</param>
/// <remarks>
/// The primary purpose of this method is to support serialization of Exceptions and
/// custom build logging events, since these do not support our custom serialization
/// methods.
/// </remarks>
void TranslateDotNet<T>(ref T value);

void TranslateException(ref Exception value);

/// <summary>
Expand Down
23 changes: 0 additions & 23 deletions src/Framework/Traits.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -392,29 +392,6 @@ public SdkReferencePropertyExpansionMode? SdkReferencePropertyExpansion
}
}

/// <summary>
/// Allows displaying the deprecation warning for BinaryFormatter in your current environment.
/// </summary>
public bool EnableWarningOnCustomBuildEvent
{
get
{
var value = Environment.GetEnvironmentVariable("MSBUILDCUSTOMBUILDEVENTWARNING");

if (value == null)
{
// If variable is not set explicitly, for .NETCORE warning appears.
#if RUNTIME_TYPE_NETCORE
return true;
#else
return ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave17_10);
#endif
}

return value == "1";
}
}

public bool UnquoteTargetSwitchParameters
{
get
Expand Down
Loading

0 comments on commit 79d15bc

Please sign in to comment.