Skip to content

Avoid NullReferenceException if "params" property is null. #25

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Oct 10, 2017
Merged

Avoid NullReferenceException if "params" property is null. #25

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8879f27
Fix for OmniSharp/csharp-language-server-protocol#24
tintoy Oct 8, 2017
4b96423
Changes to address feedback for OmniSharp/csharp-language-server-prot…
tintoy Oct 9, 2017
697c22f
Don't serialise "params" property of a request if it's null (OmniShar…
tintoy Oct 9, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/JsonRpc/Reciever.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ protected virtual Renor GetRenor(JToken @object)

object requestId = null;
bool hasRequestId;
if (hasRequestId = request.TryGetValue("id", out var id))
if (hasRequestId = request.TryGetValue("id", out var id) && requestId != null)
Copy link
Member

@david-driscoll david-driscoll Oct 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This shouldn't change, requestId is allowed to be provided as null. This indicates a request vs notification.

Copy link
Collaborator Author

@tintoy tintoy Oct 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case, shouldn't hasRequestId be false if id == null?

{
var idString = id.Type == JTokenType.String ? (string)id : null;
var idLong = id.Type == JTokenType.Integer ? (long?)id : null;
Expand All @@ -77,7 +77,7 @@ protected virtual Renor GetRenor(JToken @object)
return new InvalidRequest(requestId, "Method not set");
}

var hasParams = request.TryGetValue("params", out var @params);
var hasParams = request.TryGetValue("params", out var @params) && @params != null;
Copy link
Member

@david-driscoll david-driscoll Oct 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Revert this change.

Copy link
Collaborator Author

@tintoy tintoy Oct 9, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But isn't this the source of the NRE? If hasParams is true but @params is null, then the line below will attempt to check @params.Type and crash, won't it?

                  // @params can be null even though hasParams is true
 if (hasParams && @params.Type != JTokenType.Array

Copy link
Collaborator Author

@tintoy tintoy Oct 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nvm - saw your other comment :)

if (hasParams && @params.Type != JTokenType.Array && @params.Type != JTokenType.Object)
Copy link
Member

@david-driscoll david-driscoll Oct 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should change this line to better align to the spec. params if specified must be an array or object.

Swap with:
if (hasParams && @params?.Type != JTokenType.Array && @params?.Type != JTokenType.Object)

Copy link
Collaborator Author

@tintoy tintoy Oct 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, ok, you want to use the ?. operator. Sorry, fair enough - I missed that when viewing on my phone, I get you now. Will fix it up shortly :)

{
return new InvalidRequest(requestId, "Invalid params");
Expand All @@ -95,4 +95,4 @@ protected virtual Renor GetRenor(JToken @object)
}
}
}
}
}