Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Removing last instances of the BinaryFormatter #11346

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Removing last instances of the BinaryFormatter #11346

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2bacc42
The last instances of the BinaryFormatter have been removed
MichalPavlik Jan 28, 2025
57b333a
Update src/Build/BackEnd/Components/Logging/LoggingService.cs
MichalPavlik Jan 29, 2025
8f888bd
Update src/Shared/Resources/Strings.shared.resx
MichalPavlik Jan 29, 2025
5dff5a5
Fix for CA1823 and SA1508
MichalPavlik Jan 29, 2025
e86ebdc
Xlf update
MichalPavlik Jan 29, 2025
bee3596
NetFx version emits warning instead of error
MichalPavlik Jan 29, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 13 additions & 29 deletions src/Build.UnitTests/BackEnd/BuildManager_Logging_Tests.cs
View file
Open in desktop
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
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -971,58 +971,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
Open in desktop
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
MichalPavlik marked this conversation as resolved.
Show resolved Hide resolved
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
Open in desktop
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
Open in desktop
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
Open in desktop
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");
MichalPavlik marked this conversation as resolved.
Show resolved Hide resolved

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
Loading