From b80a6c4298ab3477beac66f63f7b03b2c0a9cccc Mon Sep 17 00:00:00 2001 From: Eddy Nakamura Date: Fri, 16 Oct 2020 14:34:50 -0300 Subject: [PATCH] Fixing null reference in propertyfetcher (#1350) replacing fetch for tryfetch fixing netcoreapp3.1 issue in htp instrumentation adding back Fetch method adding null validation; adding null test --- .../Implementation/HttpInListener.cs | 6 +-- .../Implementation/HttpInListener.cs | 10 ++--- .../GrpcClientDiagnosticListener.cs | 2 +- .../HttpHandlerDiagnosticListener.cs | 8 ++-- .../SqlClientDiagnosticListener.cs | 14 +++---- .../Instrumentation/PropertyFetcher.cs | 37 ++++++++++++++++--- .../Instrumentation/PropertyFetcherTest.cs | 14 +++++-- 7 files changed, 61 insertions(+), 30 deletions(-) diff --git a/src/OpenTelemetry.Instrumentation.AspNet/Implementation/HttpInListener.cs b/src/OpenTelemetry.Instrumentation.AspNet/Implementation/HttpInListener.cs index b95c8fc1459..52ccb350f79 100644 --- a/src/OpenTelemetry.Instrumentation.AspNet/Implementation/HttpInListener.cs +++ b/src/OpenTelemetry.Instrumentation.AspNet/Implementation/HttpInListener.cs @@ -32,7 +32,7 @@ internal class HttpInListener : ListenerHandler private const string ActivityOperationName = "Microsoft.AspNet.HttpReqIn.Start"; private static readonly Func> HttpRequestHeaderValuesGetter = (request, name) => request.Headers.GetValues(name); private readonly PropertyFetcher routeFetcher = new PropertyFetcher("Route"); - private readonly PropertyFetcher routeTemplateFetcher = new PropertyFetcher("RouteTemplate"); + private readonly PropertyFetcher routeTemplateFetcher = new PropertyFetcher("RouteTemplate"); private readonly AspNetInstrumentationOptions options; private readonly ActivitySourceAdapter activitySource; @@ -195,8 +195,8 @@ public override void OnStopActivity(Activity activity, object payload) { var subRouteData = attributeRouting.GetValue(0); - var route = this.routeFetcher.Fetch(subRouteData); - template = this.routeTemplateFetcher.Fetch(route) as string; + _ = this.routeFetcher.TryFetch(subRouteData, out var route); + _ = this.routeTemplateFetcher.TryFetch(route, out template); } } else if (routeData.Route is Route route) diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs index 453671bb74d..4f408520967 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs @@ -54,7 +54,7 @@ public HttpInListener(string name, AspNetCoreInstrumentationOptions options, Act [System.Diagnostics.CodeAnalysis.SuppressMessage("Reliability", "CA2000:Dispose objects before losing scope", Justification = "The objects should not be disposed.")] public override void OnStartActivity(Activity activity, object payload) { - HttpContext context = this.startContextFetcher.Fetch(payload); + _ = this.startContextFetcher.TryFetch(payload, out HttpContext context); if (context == null) { AspNetCoreInstrumentationEventSource.Log.NullPayload(nameof(HttpInListener), nameof(this.OnStartActivity)); @@ -151,7 +151,7 @@ public override void OnStopActivity(Activity activity, object payload) { if (activity.IsAllDataRequested) { - HttpContext context = this.stopContextFetcher.Fetch(payload); + _ = this.stopContextFetcher.TryFetch(payload, out HttpContext context); if (context == null) { AspNetCoreInstrumentationEventSource.Log.NullPayload(nameof(HttpInListener), nameof(this.OnStopActivity)); @@ -214,9 +214,9 @@ public override void OnCustom(string name, Activity activity, object payload) // The reason to use reflection is to avoid a reference on MVC package. // This package can be used with non-MVC apps and this logic simply wouldn't run. // Taking reference on MVC will increase size of deployment for non-MVC apps. - var actionDescriptor = this.beforeActionActionDescriptorFetcher.Fetch(payload); - var attributeRouteInfo = this.beforeActionAttributeRouteInfoFetcher.Fetch(actionDescriptor); - var template = this.beforeActionTemplateFetcher.Fetch(attributeRouteInfo); + _ = this.beforeActionActionDescriptorFetcher.TryFetch(payload, out var actionDescriptor); + _ = this.beforeActionAttributeRouteInfoFetcher.TryFetch(actionDescriptor, out var attributeRouteInfo); + _ = this.beforeActionTemplateFetcher.TryFetch(attributeRouteInfo, out var template); if (!string.IsNullOrEmpty(template)) { diff --git a/src/OpenTelemetry.Instrumentation.GrpcNetClient/Implementation/GrpcClientDiagnosticListener.cs b/src/OpenTelemetry.Instrumentation.GrpcNetClient/Implementation/GrpcClientDiagnosticListener.cs index 4b59c88cdd6..bc399bd19c5 100644 --- a/src/OpenTelemetry.Instrumentation.GrpcNetClient/Implementation/GrpcClientDiagnosticListener.cs +++ b/src/OpenTelemetry.Instrumentation.GrpcNetClient/Implementation/GrpcClientDiagnosticListener.cs @@ -42,7 +42,7 @@ public GrpcClientDiagnosticListener(ActivitySourceAdapter activitySource, GrpcCl public override void OnStartActivity(Activity activity, object payload) { - if (!(this.startRequestFetcher.Fetch(payload) is HttpRequestMessage request)) + if (!this.startRequestFetcher.TryFetch(payload, out HttpRequestMessage request) || request == null) { GrpcInstrumentationEventSource.Log.NullPayload(nameof(GrpcClientDiagnosticListener), nameof(this.OnStartActivity)); return; diff --git a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs index 6e9213cb974..18edcde1dfd 100644 --- a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs +++ b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs @@ -81,7 +81,7 @@ public HttpHandlerDiagnosticListener(HttpClientInstrumentationOptions options, A public override void OnStartActivity(Activity activity, object payload) { - if (!(this.startRequestFetcher.Fetch(payload) is HttpRequestMessage request)) + if (!this.startRequestFetcher.TryFetch(payload, out HttpRequestMessage request) || request == null) { HttpInstrumentationEventSource.Log.NullPayload(nameof(HttpHandlerDiagnosticListener), nameof(this.OnStartActivity)); return; @@ -131,7 +131,7 @@ public override void OnStopActivity(Activity activity, object payload) { // https://github.com/dotnet/runtime/blob/master/src/libraries/System.Net.Http/src/System/Net/Http/DiagnosticsHandler.cs // requestTaskStatus is not null - var requestTaskStatus = this.stopRequestStatusFetcher.Fetch(payload); + _ = this.stopRequestStatusFetcher.TryFetch(payload, out var requestTaskStatus); if (requestTaskStatus != TaskStatus.RanToCompletion) { @@ -146,7 +146,7 @@ public override void OnStopActivity(Activity activity, object payload) } } - if (this.stopResponseFetcher.Fetch(payload) is HttpResponseMessage response) + if (this.stopResponseFetcher.TryFetch(payload, out HttpResponseMessage response) && response != null) { try { @@ -173,7 +173,7 @@ public override void OnException(Activity activity, object payload) { if (activity.IsAllDataRequested) { - if (!(this.stopExceptionFetcher.Fetch(payload) is Exception exc)) + if (!this.stopExceptionFetcher.TryFetch(payload, out Exception exc) || exc == null) { HttpInstrumentationEventSource.Log.NullPayload(nameof(HttpHandlerDiagnosticListener), nameof(this.OnException)); return; diff --git a/src/OpenTelemetry.Instrumentation.SqlClient/Implementation/SqlClientDiagnosticListener.cs b/src/OpenTelemetry.Instrumentation.SqlClient/Implementation/SqlClientDiagnosticListener.cs index 485e0ee3a72..cafe93897c3 100644 --- a/src/OpenTelemetry.Instrumentation.SqlClient/Implementation/SqlClientDiagnosticListener.cs +++ b/src/OpenTelemetry.Instrumentation.SqlClient/Implementation/SqlClientDiagnosticListener.cs @@ -75,7 +75,7 @@ public override void OnCustom(string name, Activity activity, object payload) return; } - var command = this.commandFetcher.Fetch(payload); + _ = this.commandFetcher.TryFetch(payload, out var command); if (command == null) { SqlClientInstrumentationEventSource.Log.NullPayload(nameof(SqlClientDiagnosticListener), name); @@ -85,8 +85,8 @@ public override void OnCustom(string name, Activity activity, object payload) if (activity.IsAllDataRequested) { - var connection = this.connectionFetcher.Fetch(command); - var database = this.databaseFetcher.Fetch(connection); + _ = this.connectionFetcher.TryFetch(command, out var connection); + _ = this.databaseFetcher.TryFetch(connection, out var database); activity.DisplayName = (string)database; try @@ -98,15 +98,15 @@ public override void OnCustom(string name, Activity activity, object payload) SqlClientInstrumentationEventSource.Log.EnrichmentException(ex); } - var dataSource = this.dataSourceFetcher.Fetch(connection); - var commandText = this.commandTextFetcher.Fetch(command); + _ = this.dataSourceFetcher.TryFetch(connection, out var dataSource); + _ = this.commandTextFetcher.TryFetch(command, out var commandText); activity.SetTag(SemanticConventions.AttributeDbSystem, MicrosoftSqlServerDatabaseSystemName); activity.SetTag(SemanticConventions.AttributeDbName, (string)database); this.options.AddConnectionLevelDetailsToActivity((string)dataSource, activity); - if (this.commandTypeFetcher.Fetch(command) is CommandType commandType) + if (this.commandTypeFetcher.TryFetch(command, out CommandType commandType)) { switch (commandType) { @@ -183,7 +183,7 @@ public override void OnCustom(string name, Activity activity, object payload) { if (activity.IsAllDataRequested) { - if (this.exceptionFetcher.Fetch(payload) is Exception exception) + if (this.exceptionFetcher.TryFetch(payload, out Exception exception) && exception != null) { activity.SetStatus(Status.Error.WithDescription(exception.Message)); } diff --git a/src/OpenTelemetry/Instrumentation/PropertyFetcher.cs b/src/OpenTelemetry/Instrumentation/PropertyFetcher.cs index e060ee55231..6ac0571cc93 100644 --- a/src/OpenTelemetry/Instrumentation/PropertyFetcher.cs +++ b/src/OpenTelemetry/Instrumentation/PropertyFetcher.cs @@ -45,6 +45,28 @@ public PropertyFetcher(string propertyName) /// Property fetched. public T Fetch(object obj) { + if (!this.TryFetch(obj, out T value)) + { + throw new ArgumentException("Supplied object was null or did not match the expected type.", nameof(obj)); + } + + return value; + } + + /// + /// Try to fetch the property from the object. + /// + /// Object to be fetched. + /// Fetched value. + /// if the property was fetched. + public bool TryFetch(object obj, out T value) + { + if (obj == null) + { + value = default; + return false; + } + if (this.innerFetcher == null) { var type = obj.GetType().GetTypeInfo(); @@ -57,7 +79,7 @@ public T Fetch(object obj) this.innerFetcher = PropertyFetch.FetcherForProperty(property); } - return this.innerFetcher.Fetch(obj); + return this.innerFetcher.TryFetch(obj, out value); } // see https://github.com/dotnet/corefx/blob/master/src/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/DiagnosticSourceEventSource.cs @@ -81,9 +103,10 @@ public static PropertyFetch FetcherForProperty(PropertyInfo propertyInfo) return (PropertyFetch)Activator.CreateInstance(instantiatedTypedPropertyFetcher, propertyInfo); } - public virtual T Fetch(object obj) + public virtual bool TryFetch(object obj, out T value) { - return default; + value = default; + return false; } private class TypedPropertyFetch : PropertyFetch @@ -96,14 +119,16 @@ public TypedPropertyFetch(PropertyInfo property) this.propertyFetch = (Func)property.GetMethod.CreateDelegate(typeof(Func)); } - public override T Fetch(object obj) + public override bool TryFetch(object obj, out T value) { if (obj is TDeclaredObject o) { - return this.propertyFetch(o); + value = this.propertyFetch(o); + return true; } - return default; + value = default; + return false; } } } diff --git a/test/OpenTelemetry.Tests/Instrumentation/PropertyFetcherTest.cs b/test/OpenTelemetry.Tests/Instrumentation/PropertyFetcherTest.cs index 866c29191fc..ab004f1f75c 100644 --- a/test/OpenTelemetry.Tests/Instrumentation/PropertyFetcherTest.cs +++ b/test/OpenTelemetry.Tests/Instrumentation/PropertyFetcherTest.cs @@ -27,8 +27,7 @@ public void FetchValidProperty() { var activity = new Activity("test"); var fetch = new PropertyFetcher("DisplayName"); - var result = fetch.Fetch(activity); - + Assert.True(fetch.TryFetch(activity, out string result)); Assert.Equal(activity.DisplayName, result); } @@ -37,13 +36,20 @@ public void FetchInvalidProperty() { var activity = new Activity("test"); var fetch = new PropertyFetcher("DisplayName2"); - var result = fetch.Fetch(activity); + Assert.False(fetch.TryFetch(activity, out string result)); var fetchInt = new PropertyFetcher("DisplayName2"); - var resultInt = fetchInt.Fetch(activity); + Assert.False(fetchInt.TryFetch(activity, out int resultInt)); Assert.Equal(default, result); Assert.Equal(default, resultInt); } + + [Fact] + public void FetchNullProperty() + { + var fetch = new PropertyFetcher("null"); + Assert.False(fetch.TryFetch(null, out _)); + } } }