Fix null reference when markdown content is empty by adityapatwardhan · Pull Request #7463 · PowerShell/PowerShell

Aditya Patwardhan (adityapatwardhan) · 2018-08-06T19:37:33Z

PR Summary

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

PR Checklist

Ilya (iSazonov)

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

Ilya (iSazonov) · 2018-08-07T03:55:44Z

        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.

Ilya (iSazonov) · 2018-08-07T03:57:00Z

+            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)

Ilya (iSazonov) · 2018-08-07T04:00:08Z

        {
-            // 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?

Ilya (iSazonov) · 2018-08-07T04:06:17Z

            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 Ilya (@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.

Aditya Patwardhan (adityapatwardhan) · 2018-08-13T21:57:58Z

Rob Holt (@rjmholt) Ilya (@iSazonov) Addressed the comments please have another look.

Rob Holt (rjmholt) · 2018-08-13T23:50:12Z

+            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

Rob Holt (rjmholt) · 2018-08-13T23:54:26Z

+            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.

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

Aditya Patwardhan (adityapatwardhan) · 2018-08-14T00:09:46Z

Rob Holt (@rjmholt) Updated.

Rob Holt (rjmholt)

LGTM!

Ilya (iSazonov) · 2018-08-15T03:50:28Z

+                        // 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.

Ilya (iSazonov) · 2018-08-16T05:31:09Z

Dongbo Wang (@daxian-dbw) Steve Lee (@SteveL-MSFT) The PR is ready to merge if you haven't comments.

Fix null reference when markdown content is empty

775fd60

Aditya Patwardhan (adityapatwardhan) requested review from Steve Lee (SteveL-MSFT) and Dongbo Wang (daxian-dbw) August 6, 2018 19:40

Ilya (iSazonov) reviewed Aug 7, 2018

View reviewed changes

Address code review comments

09fedf0

Aditya Patwardhan (adityapatwardhan) assigned Ilya (iSazonov) Aug 13, 2018

Rob Holt (rjmholt) reviewed Aug 13, 2018

View reviewed changes

Address code review comments

309bf73

Fix build

6446ebe

Rob Holt (rjmholt) approved these changes Aug 14, 2018

View reviewed changes

Ilya (iSazonov) reviewed Aug 15, 2018

View reviewed changes

Ilya (iSazonov) approved these changes Aug 16, 2018

View reviewed changes

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

Aditya Patwardhan (adityapatwardhan) deleted the FixMarkdownNullReference branch August 28, 2018 17:09

Travis Plunk (TravisEz13) added this to the v6.1.0 milestone Aug 28, 2018

Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request Aug 6, 2026

Fix null reference when markdown content is empty (PowerShell#7463)

70c3a89

Uh oh!

Conversation

Aditya Patwardhan (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!

Ilya (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!

Aditya Patwardhan (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!

Aditya Patwardhan (adityapatwardhan) commented Aug 14, 2018

Uh oh!

Rob Holt (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!

Ilya (iSazonov) commented Aug 16, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Aditya Patwardhan (adityapatwardhan) commented Aug 6, 2018 •

edited

Loading