microsoft / sarif-sdk

.NET code and supporting files for working with the 'Static Analysis Results Interchange Format' (SARIF, see https://github.com/oasis-tcs/sarif-spec)
Other
192 stars 90 forks source link

rewrite --insert "GitBlameInformation" throws NullReferenceException #2683

Closed AvremelM closed 1 year ago

AvremelM commented 1 year ago

Issue:

NullReferenceException thrown when trying to insert blame information via sarif rewrite in.sarif --insert "GitBlameInformation"

Stacktrace
System.NullReferenceException: Object reference not set to an instance of an object.
   at Microsoft.CodeAnalysis.Sarif.Visitors.SarifTransformerUtilities.ParseBlameInformation(String blameText) in [...]\sarif-sdk\src\Sarif\Visitors\SarifTransformerUtilities.cs:line 329
   at Microsoft.CodeAnalysis.Sarif.Visitors.InsertOptionalDataVisitor.VisitResult(Result node) in [...]\sarif-sdk\src\Sarif\Visitors\InsertOptionalDataVisitor.cs:line 215
   at Microsoft.CodeAnalysis.Sarif.SarifRewritingVisitor.VisitActual(ISarifNode node) in [...]\sarif-sdk\src\Sarif\Autogenerated\SarifRewritingVisitor.cs:line 120
   at Microsoft.CodeAnalysis.Sarif.SarifRewritingVisitor.Visit(ISarifNode node) in [...]\sarif-sdk\src\Sarif\Autogenerated\SarifRewritingVisitor.cs:line 28
   at Microsoft.CodeAnalysis.Sarif.SarifRewritingVisitor.VisitNullChecked[T](T node) in [...]\sarif-sdk\src\Sarif\Autogenerated\SarifRewritingVisitor.cs:line 167
   at Microsoft.CodeAnalysis.Sarif.SarifRewritingVisitor.VisitRun(Run node) in [...]\sarif-sdk\src\Sarif\Autogenerated\SarifRewritingVisitor.cs:line 1117
   at Microsoft.CodeAnalysis.Sarif.Visitors.InsertOptionalDataVisitor.VisitRun(Run node) in [...]\sarif-sdk\src\Sarif\Visitors\InsertOptionalDataVisitor.cs:line 96
   at Microsoft.CodeAnalysis.Sarif.SarifRewritingVisitor.VisitActual(ISarifNode node) in [...]\sarif-sdk\src\Sarif\Autogenerated\SarifRewritingVisitor.cs:line 124
   at Microsoft.CodeAnalysis.Sarif.SarifRewritingVisitor.Visit(ISarifNode node) in [...]\sarif-sdk\src\Sarif\Autogenerated\SarifRewritingVisitor.cs:line 28
   at Microsoft.CodeAnalysis.Sarif.SarifRewritingVisitor.VisitNullChecked[T](T node) in [...]\sarif-sdk\src\Sarif\Autogenerated\SarifRewritingVisitor.cs:line 167
   at Microsoft.CodeAnalysis.Sarif.SarifRewritingVisitor.VisitSarifLog(SarifLog node) in [...]\sarif-sdk\src\Sarif\Autogenerated\SarifRewritingVisitor.cs:line 1211
   at Microsoft.CodeAnalysis.Sarif.Multitool.RewriteCommand.Run(RewriteOptions options) in [...]\sarif-sdk\src\Sarif.Multitool.Library\RewriteCommand.cs:line 61

Details

Issue seems to be two-fold:

  1. GitHelper.GetSimpleGitCommandOutput(string repoPath, ...) requires the repoPath argument to be the repository root (here), but GitHelper.GetBlame(...) passes in the full directory path to the file here. So GetBlame(...) will return null for any file that isn't in the repo's root.
  2. SarifTransformerUtilities.ParseBlameInformation(...) doesn't handle null results from _gitHelper.GetBlame(filePath) here

Solution

Simplest solution seems to be to modify GetBlame(...) from

return GetSimpleGitCommandOutput(
    Path.GetDirectoryName(filePath),
    args: $"blame -f --porcelain {Path.GetFileName(filePath)}",
    trimLines: false);
return GetSimpleGitCommandOutput(
    GetTopLevel(filePath),
    args: $"blame -f --porcelain {filePath}",
    trimLines: false);

(Modifying GetSimpleGitCommandOutput() to remove the IsRepositoryRoot() check would also work, but obviously that may negatively affect other callers)

~If it helps, I'm happy to open a PR.~

AvremelM commented 1 year ago

Fixed in #2686