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.

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

Bugfix for null dereferencing. Issue found by SonarQube C# Code Analysis. #9

Merged
merged 1 commit into from Jun 18, 2015

Conversation

Projects
None yet
3 participants
@ghost

ghost commented Jun 17, 2015

SonarQube C# Code Analysis provides on-the-fly feedback to developers on any new bug or quality issue injected into C# code. This package is based on and benefits from the .NET Compiler Platform ("Roslyn") and the related code analysis API to provide a fully-integrated user experience in Visual Studio 2015.

https://www.nuget.org/packages/SonarQube.CSharp.CodeAnalysis
https://visualstudiogallery.msdn.microsoft.com/148ea243-c018-4138-9222-4f0e81270f6a
https://redirect.sonarsource.com/6

@shanselman

This comment has been minimized.

Show comment
Hide comment
@shanselman

shanselman Jun 17, 2015

Owner

&& is a short-circuit and, so if x && y and x is false then y isn't evaluated, no? https://msdn.microsoft.com/en-us/library/2a723cdk.aspx

Owner

shanselman commented Jun 17, 2015

&& is a short-circuit and, so if x && y and x is false then y isn't evaluated, no? https://msdn.microsoft.com/en-us/library/2a723cdk.aspx

@issafram

This comment has been minimized.

Show comment
Hide comment
@issafram

issafram Jun 18, 2015

@shanselman, I believe the difference between the PR and what is currently in source is that yes you will short-circuit with &&.
So the logic in the if statement will never execute for when the pingbackService is not null but also empty (length of 0). I believe that is what @tvsonar is attempting to fix here.

Side note, got here from your blog. Good stuff.

issafram commented Jun 18, 2015

@shanselman, I believe the difference between the PR and what is currently in source is that yes you will short-circuit with &&.
So the logic in the if statement will never execute for when the pingbackService is not null but also empty (length of 0). I believe that is what @tvsonar is attempting to fix here.

Side note, got here from your blog. Good stuff.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Jun 18, 2015

@shanselman you are right that && short-circuits if the first part is false. But:

The original condition was pingbackService == null && pingbackService.Length == 0. Consider the case when pingbackService is null. In this case we get to this condition: true && null.Length == 0. The first part is true, so the second part will be evaluated. And it will throw a NullReferenceException.

http://dist.sonarsource.com/csharp/rulesdoc/0.9.0-RC/S1697.html

ghost commented Jun 18, 2015

@shanselman you are right that && short-circuits if the first part is false. But:

The original condition was pingbackService == null && pingbackService.Length == 0. Consider the case when pingbackService is null. In this case we get to this condition: true && null.Length == 0. The first part is true, so the second part will be evaluated. And it will throw a NullReferenceException.

http://dist.sonarsource.com/csharp/rulesdoc/0.9.0-RC/S1697.html

@shanselman

This comment has been minimized.

Show comment
Hide comment
@shanselman

shanselman Jun 18, 2015

Owner

@tvsonar Ah, I stand corrected, then it should really be != null, not || yes?

Owner

shanselman commented Jun 18, 2015

@tvsonar Ah, I stand corrected, then it should really be != null, not || yes?

@issafram

This comment has been minimized.

Show comment
Hide comment
@issafram

issafram Jun 18, 2015

@shanselman, No I don't believe so.

The code block has a comment right before the code:

// if we don't get the header
// try and autodetect the auto pingback info

Then it tries to set pingbackService in the logic inside the if statement.

The || is correct because it first checks for null. If it isn't, it then checks if the length is zero.
If either condition is true, it will then use the regex split logic to set the pingbackService variable.

issafram commented Jun 18, 2015

@shanselman, No I don't believe so.

The code block has a comment right before the code:

// if we don't get the header
// try and autodetect the auto pingback info

Then it tries to set pingbackService in the logic inside the if statement.

The || is correct because it first checks for null. If it isn't, it then checks if the length is zero.
If either condition is true, it will then use the regex split logic to set the pingbackService variable.

shanselman added a commit that referenced this pull request Jun 18, 2015

Merge pull request #9 from tvsonar/null-test-bugfix
Bugfix for null dereferencing. Issue found by SonarQube C# Code Analysis.

@shanselman shanselman merged commit 77fd0ac into shanselman:master Jun 18, 2015

1 check passed

continuous-integration/appveyor AppVeyor build succeeded
Details

@ghost ghost deleted the null-test-bugfix branch Jun 19, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment