Fix null reference when markdown content is empty #7463

adityapatwardhan · 2018-08-06T19:37:33Z

Fix #7453

PR Summary

Fix cases where markdown content can be empty and we get a null reference exception.

PR Checklist

iSazonov

I wonder that we have to check so many null references. Seems we need review all the code (not in the PR).

iSazonov · 2018-08-07T03:55:44Z

src/Microsoft.PowerShell.MarkdownRender/FencedCodeBlockRenderer.cs

        protected override void Write(VT100Renderer renderer, FencedCodeBlock obj)
        {
-            foreach (var codeLine in obj.Lines.Lines)
+            if (obj?.Lines != null && obj.Lines.Lines != null)


If we use "?" we could do

if (obj?.Lines?.Lines != null)

Updated to obj?.Lines.Lines != null

obj.Lines is of type StringLineGroup which cannot be null.

iSazonov · 2018-08-07T03:57:00Z

src/Microsoft.PowerShell.MarkdownRender/FencedCodeBlockRenderer.cs

+            if (obj?.Lines != null && obj.Lines.Lines != null)
            {
-                if (!string.IsNullOrWhiteSpace(codeLine.ToString()))
+                foreach (var codeLine in obj?.Lines.Lines)


We have checked obj != null in previous line and could remove "?"

foreach (var codeLine in obj.Lines.Lines)

iSazonov · 2018-08-07T04:00:08Z

src/Microsoft.PowerShell.MarkdownRender/HeaderBlockRenderer.cs

        {
-            // Format header and then add blank line to improve readability.
-            switch (obj.Level)
+            string headerText = obj.Inline?.FirstChild?.ToString();


Should we check obj != null too?

iSazonov · 2018-08-07T04:06:17Z

src/Microsoft.PowerShell.MarkdownRender/LinkInlineRenderer.cs

            else
            {
-                renderer.Write(renderer.EscapeSequences.FormatLink(obj.FirstChild.ToString(), obj.Url));
+                renderer.Write(renderer.EscapeSequences.FormatLink(obj.FirstChild?.ToString(), obj.Url));


We check obj.FirstChild != null in both code path in the "if-else" - seems we should move the check to new "if":

if (obj.FirstChild != null) { if (obj.IsImage) { renderer.Write(renderer.EscapeSequences.FormatImage(obj.FirstChild.ToString())); } else { renderer.Write(renderer.EscapeSequences.FormatLink(obj.FirstChild.ToString(), obj.Url)); } }

The question marks here would have different semantics of course, but I imagine FormatLink will thrown an NRE if its first argument is null.

So I would do similar to @iSazonov (just a style change):

if (obj.FirstChild == null) { return; } if (obj.IsImage) { renderer.Write(renderer.EscapeSequences.FormatImage(obj.FirstChild.ToString())); } else { renderer.Write(renderer.EscapeSequences.FormatLink(obj.FirstChild.ToString(), obj.Url)); }

We support having empty alt-text for images and empty link text. So updated accordingly.

adityapatwardhan · 2018-08-13T21:57:58Z

@rjmholt @iSazonov Addressed the comments please have another look.

rjmholt · 2018-08-13T23:50:12Z

src/Microsoft.PowerShell.MarkdownRender/FencedCodeBlockRenderer.cs

+            if (obj?.Lines.Lines != null)
            {
-                if (!string.IsNullOrWhiteSpace(codeLine.ToString()))
+                foreach (var codeLine in obj.Lines.Lines)


string instead of var would be more obvious here

rjmholt · 2018-08-13T23:54:26Z

src/Microsoft.PowerShell.MarkdownRender/LinkInlineRenderer.cs

+            string text = obj?.FirstChild?.ToString();
+
            // Format link as image or link.
            if (obj.IsImage)


If obj is null, this will cause an NRE. Would the logic be preserved like this:

string text = obj?.FirstChild?.ToString(); if (text == null) { return; } // Golden path code...

?

obj can never be null, hence removed ? after obj.

We allow text to be null hence the above logic holds.

if (obj.IsImage) - can cause an NRE.

@iSazonov obj is guaranteed to be not null. If it is null the renderer is not called.

adityapatwardhan · 2018-08-14T00:09:46Z

@rjmholt Updated.

rjmholt

LGTM!

iSazonov · 2018-08-15T03:50:28Z

src/Microsoft.PowerShell.MarkdownRender/FencedCodeBlockRenderer.cs

+                        // This specifically helps for parameters help content.
+                        if (string.Equals(obj.Info, "yaml", StringComparison.OrdinalIgnoreCase))
+                        {
+                            renderer.Write("\t").WriteLine(codeLine.ToString());


We call codeLine.ToString()) twice above - should we use a variable?
And in line 34 too.

One is in if and other is in else, so only one of them is called. And this change is out of scope for this PR.

iSazonov · 2018-08-16T05:31:09Z

@daxian-dbw @SteveL-MSFT The PR is ready to merge if you haven't comments.

Fix null reference when markdown content is empty

775fd60

adityapatwardhan requested review from SteveL-MSFT and daxian-dbw August 6, 2018 19:40

iSazonov reviewed Aug 7, 2018

View reviewed changes

Address code review comments

09fedf0

adityapatwardhan assigned iSazonov Aug 13, 2018

rjmholt reviewed Aug 13, 2018

View reviewed changes

Address code review comments

309bf73

Fix build

6446ebe

rjmholt approved these changes Aug 14, 2018

View reviewed changes

iSazonov reviewed Aug 15, 2018

View reviewed changes

iSazonov approved these changes Aug 16, 2018

View reviewed changes

iSazonov merged commit aec954e into PowerShell:master Aug 27, 2018

adityapatwardhan deleted the FixMarkdownNullReference branch August 28, 2018 17:09

TravisEz13 added this to the v6.1.0 milestone Aug 28, 2018

Fix null reference when markdown content is empty #7463

Fix null reference when markdown content is empty #7463

Uh oh!

Conversation

adityapatwardhan commented Aug 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Checklist

Uh oh!

iSazonov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adityapatwardhan commented Aug 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adityapatwardhan commented Aug 14, 2018

Uh oh!

rjmholt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iSazonov commented Aug 16, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

adityapatwardhan commented Aug 6, 2018 •

edited

Loading