From afb635290173badc48c48846bc9cafdcf63dedd9 Mon Sep 17 00:00:00 2001 From: Marty T <120425148+tippmar-nr@users.noreply.github.com> Date: Fri, 12 Jan 2024 16:04:47 -0600 Subject: [PATCH] fix: Prevent null reference exceptions while serializing LoadedModuleWireModelCollection. (#2185) (#2187) --- ...dModuleWireModelCollectionJsonConverter.cs | 2 +- .../LoadedModuleWireModelCollection.cs | 22 ++----- ...leWireModelCollectionJsonConverterTests.cs | 25 ++++++++ .../LoadedModuleWireModelCollectionTests.cs | 62 ++++++++++++++----- 4 files changed, 79 insertions(+), 32 deletions(-) diff --git a/src/Agent/NewRelic/Agent/Core/JsonConverters/LoadedModuleWireModelCollectionJsonConverter.cs b/src/Agent/NewRelic/Agent/Core/JsonConverters/LoadedModuleWireModelCollectionJsonConverter.cs index 7cbb58d47..5ea8cd427 100644 --- a/src/Agent/NewRelic/Agent/Core/JsonConverters/LoadedModuleWireModelCollectionJsonConverter.cs +++ b/src/Agent/NewRelic/Agent/Core/JsonConverters/LoadedModuleWireModelCollectionJsonConverter.cs @@ -41,7 +41,7 @@ private static void WriteJsonImpl(JsonWriter jsonWriter, LoadedModuleWireModelCo foreach (var item in loadedModule.Data) { jsonWriter.WritePropertyName(item.Key); - jsonWriter.WriteValue(item.Value.ToString()); + jsonWriter.WriteValue(item.Value?.ToString() ?? " "); } jsonWriter.WriteEndObject(); diff --git a/src/Agent/NewRelic/Agent/Core/WireModels/LoadedModuleWireModelCollection.cs b/src/Agent/NewRelic/Agent/Core/WireModels/LoadedModuleWireModelCollection.cs index f03c6d504..ff724d815 100644 --- a/src/Agent/NewRelic/Agent/Core/WireModels/LoadedModuleWireModelCollection.cs +++ b/src/Agent/NewRelic/Agent/Core/WireModels/LoadedModuleWireModelCollection.cs @@ -82,12 +82,7 @@ private static bool TryGetAssemblyName(Assembly assembly, out string assemblyNam assemblyName = Path.GetFileName(assembly.Location); } - if (string.IsNullOrWhiteSpace(assemblyName)) - { - return false; - } - - return true; + return !string.IsNullOrWhiteSpace(assemblyName); } catch { @@ -101,12 +96,7 @@ private static bool TryGetPublicKeyToken(AssemblyName assemblyDetails, out strin try { publicKey = BitConverter.ToString(assemblyDetails.GetPublicKeyToken()).Replace("-", ""); - if (string.IsNullOrWhiteSpace(publicKey)) - { - return false; - } - - return true; + return !string.IsNullOrWhiteSpace(publicKey); } catch { @@ -154,7 +144,7 @@ private static bool TryGetShaFileHashes(Assembly assembly, out string sha1FileHa sha512.Dispose(); } - return true; + return !string.IsNullOrWhiteSpace(sha1FileHash) && !string.IsNullOrWhiteSpace(sha512FileHash); } catch { @@ -169,7 +159,7 @@ private static bool TryGetAssemblyHashCode(Assembly assembly, out string assembl try { assemblyHashCode = assembly.GetHashCode().ToString(); - return true; + return !string.IsNullOrWhiteSpace(assemblyHashCode); } catch { @@ -190,7 +180,7 @@ private static bool TryGetCompanyName(Assembly assembly, out string companyName) } companyName = ((AssemblyCompanyAttribute)attributes[0]).Company; - return true; + return !string.IsNullOrWhiteSpace(companyName); } catch { @@ -211,7 +201,7 @@ private static bool TryGetCopyright(Assembly assembly, out string copyright) } copyright = ((AssemblyCopyrightAttribute)attributes[0]).Copyright; - return true; + return !string.IsNullOrWhiteSpace(copyright); } catch { diff --git a/tests/Agent/UnitTests/Core.UnitTest/JsonConverters/LoadedModuleWireModelCollectionJsonConverterTests.cs b/tests/Agent/UnitTests/Core.UnitTest/JsonConverters/LoadedModuleWireModelCollectionJsonConverterTests.cs index 6eaa178be..ed86263f7 100644 --- a/tests/Agent/UnitTests/Core.UnitTest/JsonConverters/LoadedModuleWireModelCollectionJsonConverterTests.cs +++ b/tests/Agent/UnitTests/Core.UnitTest/JsonConverters/LoadedModuleWireModelCollectionJsonConverterTests.cs @@ -48,5 +48,30 @@ public void LoadedModuleWireModelCollectionIsJsonSerializable() var serialized = JsonConvert.SerializeObject(new[] { loadedModules }, Formatting.None); Assert.AreEqual(expected, serialized); } + + [Test] + public void LoadedModuleWireModelCollectionHandlesNulls() + { + var expected = @"[""Jars"",[[""MyTestAssembly"",""1.0.0"",{""namespace"":""MyTestAssembly"",""publicKeyToken"":""7075626C69636B6579746F6B656E"",""assemblyHashCode"":""42""}]]]"; + + var baseAssemblyName = new AssemblyName(); + baseAssemblyName.Name = BaseAssemblyName; + baseAssemblyName.Version = new Version(BaseAssemblyVersion); + baseAssemblyName.SetPublicKeyToken(Encoding.ASCII.GetBytes(BasePublicKeyToken)); + + var baseTestAssembly = new TestAssembly(); + baseTestAssembly.SetAssemblyName = baseAssemblyName; + baseTestAssembly.SetDynamic = true; // false uses on disk assembly and this won't have one. + baseTestAssembly.SetHashCode = BaseHashCode; + baseTestAssembly.AddCustomAttribute(new AssemblyCompanyAttribute(null)); + baseTestAssembly.AddCustomAttribute(new AssemblyCopyrightAttribute(null)); + + var assemblies = new List { baseTestAssembly }; + + var loadedModules = LoadedModuleWireModelCollection.Build(assemblies); + + var serialized = JsonConvert.SerializeObject(new[] { loadedModules }, Formatting.None); + Assert.AreEqual(expected, serialized); + } } } diff --git a/tests/Agent/UnitTests/Core.UnitTest/WireModels/LoadedModuleWireModelCollectionTests.cs b/tests/Agent/UnitTests/Core.UnitTest/WireModels/LoadedModuleWireModelCollectionTests.cs index 2023c13c4..bc506faee 100644 --- a/tests/Agent/UnitTests/Core.UnitTest/WireModels/LoadedModuleWireModelCollectionTests.cs +++ b/tests/Agent/UnitTests/Core.UnitTest/WireModels/LoadedModuleWireModelCollectionTests.cs @@ -1,16 +1,11 @@ -// Copyright 2020 New Relic, Inc. All rights reserved. +// Copyright 2020 New Relic, Inc. All rights reserved. // SPDX-License-Identifier: Apache-2.0 using System; using System.Collections.Generic; -using System.Linq; using System.Reflection; -using System.Runtime.InteropServices; using System.Text; -using NewRelic.Agent.Core.Metrics; -using NewRelic.Collections; using NUnit.Framework; -using Telerik.JustMock; using static Google.Protobuf.Reflection.SourceCodeInfo.Types; namespace NewRelic.Agent.Core.WireModels @@ -19,13 +14,14 @@ namespace NewRelic.Agent.Core.WireModels public class LoadedModuleWireModelCollectionTests { private const string BaseAssemblyName = "MyTestAssembly"; - private const string BaseAssemblyVersion = "1.0.0"; - private const string BaseAssemblyPath = @"C:\path\to\assembly\MyTestAssembly.dll"; - private const string BaseCompanyName = "MyCompany"; - private const string BaseCopyrightValue = "Copyright 2008"; - private const int BaseHashCode = 42; - private const string BasePublicKeyToken = "publickeytoken"; - private const string BasePublicKey = "7075626C69636B6579746F6B656E"; + + private string BaseAssemblyVersion; + private string BaseAssemblyPath; + private string BaseCompanyName; + private string BaseCopyrightValue; + private int BaseHashCode; + private string BasePublicKeyToken; + private string BasePublicKey; private AssemblyName _baseAssemblyName; private TestAssembly _baseTestAssembly; @@ -33,6 +29,15 @@ public class LoadedModuleWireModelCollectionTests [SetUp] public void SetUp() { + + BaseAssemblyVersion = "1.0.0"; + BaseAssemblyPath = @"C:\path\to\assembly\MyTestAssembly.dll"; + BaseCompanyName = "MyCompany"; + BaseCopyrightValue = "Copyright 2008"; + BaseHashCode = 42; + BasePublicKeyToken = "publickeytoken"; + BasePublicKey = "7075626C69636B6579746F6B656E"; + _baseAssemblyName = new AssemblyName(); _baseAssemblyName.Name = BaseAssemblyName; _baseAssemblyName.Version = new Version(BaseAssemblyVersion); @@ -64,7 +69,6 @@ public int TryGetAssemblyName_UsingCollectionCount(string assemblyName, bool isD { _baseTestAssembly.SetLocation = null; } - _baseTestAssembly.SetAssemblyName = _baseAssemblyName; _baseTestAssembly.SetDynamic = isDynamic; @@ -194,6 +198,35 @@ public void ErrorsHandled_GetCustomAttributes() Assert.False(loadedModule.Data.ContainsKey("sha512Checksum")); } + [Test] + public void ErrorsHandled_GetCustomAttributes_HandlesNulls() + { + _baseTestAssembly = new TestAssembly(); + _baseTestAssembly.SetAssemblyName = _baseAssemblyName; + _baseTestAssembly.SetDynamic = true; // false uses on disk assembly and this won't have one. + _baseTestAssembly.SetHashCode = BaseHashCode; + + + _baseTestAssembly.AddCustomAttribute(new AssemblyCompanyAttribute(null)); + _baseTestAssembly.AddCustomAttribute(new AssemblyCopyrightAttribute(null)); + + + var evilAssembly = new EvilTestAssembly(_baseTestAssembly); + evilAssembly.ItemToTest = ""; + + var assemblies = new List(); + assemblies.Add(evilAssembly); + + var loadedModules = LoadedModuleWireModelCollection.Build(assemblies); + + Assert.AreEqual(1, loadedModules.LoadedModules.Count); + + var loadedModule = loadedModules.LoadedModules[0]; + + Assert.False(loadedModule.Data.ContainsKey("Implementation-Vendor")); + Assert.False(loadedModule.Data.ContainsKey("copyright")); + } + [Test] public void ErrorsHandled_PublickeyToken() { @@ -266,7 +299,6 @@ public string SetLocation } public override string Location => _location; - public void AddCustomAttribute(object attribute) { _customAttributes.Add(attribute);