Reauthor runtime errors. #2660

michaelcfanning · 2023-04-30T23:36:26Z

BRK: RuntimeConditions now of type long to permit more flag values. Many literal values have changed for individual members. #2660
BRK: RuntimeConditions.OneOrMoreFilesSkippedDueToSize renamed to OneOrMoreFilesSkippedDueToExceedingSizeLimits. #2660
BUG: Properly report skipping empty files (rather than reporting file was skipped due to exceeding size limits). #2660
NEW: Provide convenience enumerator at the SarifLog level that iterates over all results in all runs in the log. #2660
NEW: Provide Notes.LogEmptyFileSkipped helper for reporting zero-byte files skipped at scan time. #2660

michaelcfanning · 2023-04-30T23:45:19Z

src/Sarif.Driver/Sdk/MultithreadedAnalyzeCommandBase.cs


-                //TBD resolve type mismatch
-                _ignoredFilesCount += (uint)context.TargetsProvider.Skipped.Count;
+                    Notes.LogEmptyFileSkipped(context, artifact.Uri.GetFilePath());


LogEmptyFileSkipped

Note that we don't track/report out on skipping 0-byte files, though we do set a non-fatal runtime condition. How interesting is it that we don't scan a 0-byte file?

I suppose we still have a problem, though, there's at least a theoretical case that someone's analysis might be intended to flag all zero-byte files. :) We could allow this lower-bound threshold to be configurable and the core engine code would probably need to be updated.

michaelcfanning · 2023-04-30T23:47:02Z

src/Test.UnitTests.Sarif/Core/SarifLogTests.cs

@@ -302,62 +302,69 @@ private static SarifLog GetSarifLogWithMinimalUniqueData()
        }

        [Fact]
-        public async Task SarifLog_Post_WithValidParameters_ShouldNotThrownAnExceptionWhenRequestIsValid()


SarifLog_Post_WithValidParameters_ShouldNotThrownAnExceptionWhenRequestIsValid

These tests were poorly factored and as a result made it difficult to locate an underlying test helper bug. It's a good object lesson for why we factor tests the way that we do, it's to lower costs in resolving regressions (in this case introduced by an update to net6.0 from netcoreapp3.1).

michaelcfanning · 2023-04-30T23:47:34Z

src/Test.UnitTests.Sarif/Core/StackTests.cs

@@ -38,7 +38,7 @@ public void Stack_CreateFromException()
            {
                File.Create(Path.GetInvalidFileNameChars()[0].ToString(), 0);
            }
-            catch (ArgumentException exception)
+            catch (IOException exception)


IOException

Change in behavior from netcoreapp3.1, File.Create now raises an IOException for illegaly chars (which makes sense).

michaelcfanning · 2023-04-30T23:47:55Z

src/Test.UnitTests.Sarif/Writers/SarifLoggerTests.cs

@@ -845,13 +845,13 @@ public void SarifLogger_EnhancesRunWithAdditionalAnalysisTargets()
        [Fact]
        public void SarifLogger_AcceptsOverrideOfDefaultEncoding()
        {
-            const string Utf8 = "UTF-8";
-            const string Utf7 = "UTF-7";


UTF

UTF7 is entirely deprecated in net6.0 as an insecure format to support.

michaelcfanning · 2023-04-30T23:49:49Z

src/Test.Utilities.Sarif/HttpMockHelper.cs

@@ -20,6 +20,11 @@ public static StringContent AnyContent()
            return new StringContent(AnyContentText);
        }

+        public static HttpResponseMessage CreateOKResponse()


CreateOKResponse

So, this code illustrates some of the dangers of statics. There is no such thing as a static response, once you read the stream from it, that stream is disposed.

I'm surprised we haven't gotten burned by this before. We need to revisit this test class. I took a small change along the lines of what I think we'll need, helpers that dynamically create new instances of the default data.

Though honestly, a helper that wraps a single line of code may not be worth implementing at all. :)

michaelcfanning · 2023-04-30T23:50:41Z

src/Test.Utilities.Sarif/TestUtilitiesExtensions.cs

-            context.RuntimeExceptions?[0].InnerException.Should().BeNull();
-            context.RuntimeExceptions?[0].Should().BeNull();
-
+            context.RuntimeExceptions?[0].InnerException?.ToString().Should().BeNull();


ToString

These are subtle changes that publish more actionable data to the test logs when failures occur. Now we'll get all of ex.ToString() (which is very rich and useful) rather than a limited report that's constrained to the exception type.

src/Test.UnitTests.Sarif/Core/SarifLogTests.cs

+                new HttpRequestMessage(HttpMethod.Post, postUri)
+                {
+                    Content = new StreamContent(CreateSarifLogStream())
+                },


src/Sarif/Core/SarifLog.cs

+        /// <returns>A Result enumerator for the SARIF log.</returns>
+        public IEnumerable<Result> Results()
+        {
+            if (this.Runs?.Count > 0 == false) { yield break; }


KalleOlaviNiemitalo · 2023-05-03T16:47:03Z

src/Sarif/Core/SarifLog.cs

+
+            foreach (Run run in this.Runs)
+            {
+                if (run.Results?.Count > 0 == false) { continue; }


The > operator indeed returns bool rather than bool?. But if you used the ! operator, then you'd have to add parentheses if (!(run.Results?.Count > 0)), which would look more complex.

I think if (run.Results?.Count ?? 0 == 0) would be quite readable here.

KalleOlaviNiemitalo · 2023-05-03T16:48:28Z

src/Sarif.Driver/Sdk/ExitApplicationException.cs

+
+        public override string ToString()
+        {
+            return $"{ExitReason}: {base.ToString()}";


Does CodeQL expect $"{ExitReason}: {base}"? I don't think C# allows that.

src/Test.UnitTests.Sarif/Core/SarifLogTests.cs

            httpMock.Mock(
-                new HttpRequestMessage(HttpMethod.Post, postUri) { Content = new StreamContent(memoryStream) },
+                new HttpRequestMessage(HttpMethod.Post, postUri) { Content = new StreamContent(CreateSarifLogStream()) },


src/Test.UnitTests.Sarif/Core/SarifLogTests.cs

+                new HttpRequestMessage(HttpMethod.Post, postUri)
+                {
+                    Content = new StreamContent(CreateSarifLogStream())
+                },


HulonJenkins

yongyan-gh · 2023-05-01T16:59:49Z

src/Sarif/Notes.cs

+                    context.MaxFileSizeInKilobytes.ToString()));
+
+            context.RuntimeErrors |= RuntimeConditions.OneOrMoreEmptyFilesSkipped;
+        }


nit: missing a empty line after this function

yongyan-gh · 2023-05-01T17:34:39Z

src/Sarif/Warnings.cs

        {
            if (context == null)
            {
                throw new ArgumentNullException(nameof(context));
            }

-            // {0} file(s)s were skipped for analysis due to exceeding size limits
+            // {0} file(s)s were skipped for analysis due to exceeding size limit
            // (currently configured as {1} kilobytes). The 'max-file-size-in-kb'
            // command-line argument can be used to increase this threshold.
            context.Logger.LogConfigurationNotification(


LogConfigurationNotification

may move the common code into a function and resuse it

schlaman-ms

shaopeng-gh · 2023-05-01T19:24:38Z

src/Test.FunctionalTests.Sarif/SarifValidatorTests.cs

@@ -197,7 +197,7 @@ public void ValidatesAllTestFiles()
            Path.Combine("v2", "ConverterTestData"),
            Path.Combine("v2", "SpecExamples"),
            Path.Combine("v2", "ObsoleteFormats"),
-            Path.Combine("..", "..", "Test.UnitTests.Sarif", "netcoreapp3.1", "TestData")
+            Path.Combine("..", "..", "Test.UnitTests.Sarif", "net6.0", "TestData")


net6.0

fyi Suvam mentioned to me coyote testing does not support 6.0 yet, see if we want to keep in 3.1

shaopeng-gh

shaopeng-gh · 2023-05-04T23:16:38Z

src/Sarif/RuntimeConditions.cs

@@ -11,51 +11,98 @@ namespace Microsoft.CodeAnalysis.Sarif
    // code paths. These conditions are a combination of fatal
    // and non-fatal circumstances
    [Flags]
-    public enum RuntimeConditions : uint
+    public enum RuntimeConditions : long


long

@michaelcfanning we changed this to long, should we change the usage of it to use long as well,
e.g. I see the run result was int, should we also change to long?

abstract public int Run(T options);

Reauthor runtime errors.

2b14753

michaelcfanning commented Apr 30, 2023

View reviewed changes

Update release notes.

9f3df20

github-advanced-security bot found potential problems May 1, 2023

View reviewed changes

michaelcfanning marked this pull request as ready for review May 1, 2023 15:03

michaelcfanning requested review from EasyRhinoMSFT, marmegh and cfaucon as code owners May 1, 2023 15:03

HulonJenkins approved these changes May 1, 2023

View reviewed changes

yongyan-gh reviewed May 1, 2023

View reviewed changes

schlaman-ms approved these changes May 1, 2023

View reviewed changes

shaopeng-gh reviewed May 1, 2023

View reviewed changes

shaopeng-gh approved these changes May 1, 2023

View reviewed changes

Merge branch 'main' into runtime-errors

77c99fb

michaelcfanning merged commit dc372ae into main May 3, 2023
8 of 9 checks passed

michaelcfanning deleted the runtime-errors branch May 3, 2023 15:10

shaopeng-gh reviewed May 4, 2023

View reviewed changes

shaopeng-gh mentioned this pull request May 5, 2023

Add test to repro error in Linux ERR999.UnhandledEngineException : System.InvalidOperationException: This operation is not supported for a relative URI. microsoft/binskim#884

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reauthor runtime errors. #2660

Reauthor runtime errors. #2660

michaelcfanning commented Apr 30, 2023 •

edited

Loading

michaelcfanning Apr 30, 2023

michaelcfanning Apr 30, 2023

michaelcfanning Apr 30, 2023

michaelcfanning Apr 30, 2023

michaelcfanning Apr 30, 2023

michaelcfanning Apr 30, 2023

KalleOlaviNiemitalo May 3, 2023

KalleOlaviNiemitalo May 3, 2023

HulonJenkins left a comment

yongyan-gh May 1, 2023 •

edited

Loading

yongyan-gh May 1, 2023

schlaman-ms left a comment

shaopeng-gh May 1, 2023

shaopeng-gh left a comment

shaopeng-gh May 4, 2023 •

edited

Loading

Reauthor runtime errors. #2660

Reauthor runtime errors. #2660

Conversation

michaelcfanning commented Apr 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HulonJenkins left a comment

Choose a reason for hiding this comment

yongyan-gh May 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schlaman-ms left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shaopeng-gh left a comment

Choose a reason for hiding this comment

shaopeng-gh May 4, 2023 • edited Loading

Choose a reason for hiding this comment

michaelcfanning commented Apr 30, 2023 •

edited

Loading

yongyan-gh May 1, 2023 •

edited

Loading

shaopeng-gh May 4, 2023 •

edited

Loading