PVS-Studio: fixed vulnerability CWE-476 (NULL Pointer Dereference) #16807
Conversation
|
@IvanKishchenko, It will cover your contributions to all Microsoft-managed open source projects. |
|
@IvanKishchenko, thanks for signing the contribution license agreement. We will now validate the agreement and then the pull request. |
|
Thanks, @IvanKishchenko. Most of these changes look fine, but at least one not: they broke ~2500 tests. Can you please fix? |
|
Dear, @stephentoub . Now I am working on fixing these. |
| @@ -43,7 +43,7 @@ internal class CaseInsensitiveAscii : IEqualityComparer, IComparer | |||
| public int GetHashCode(object myObject) | |||
| { | |||
| string myString = myObject as string; | |||
| if (myObject == null) | |||
| if (myString == null) | |||
There was a problem hiding this comment.
It is better to use object.ReferenceEquals to avoid calling the equality operator
There was a problem hiding this comment.
Roslyn is smart enough to avoid the string equality operator when comparing null, so object.ReferenceEquals isn't necessary.
There was a problem hiding this comment.
Theoretically before I made any changes, this code should have been failing with NullReferenceException.
But in fact it breaks tests.
I think that this piece of code should be reconsidered because it does not make any sense and NullReferenceException can be thrown in more simple way.
| @@ -106,7 +106,7 @@ private void RecordType(AggregateType type, Symbol sym) | |||
| // If it is first, record it. | |||
| if (_swtFirst == null) | |||
| { | |||
| _swtFirst.Set(sym, type); | |||
| _swtFirst.Set(sym, type); // NullReferenceException possible! | |||
There was a problem hiding this comment.
_swtFirst actually is never null since it is initialized only once in constructor.
The wierd thing here is that SymWithType overloads the equality operator so that it equals to null when one of its field is null.
|
All unit tests are successful. |
| @@ -103,7 +103,7 @@ internal CompilerInfo(CompilerParameters compilerParams, string codeDomProviderT | |||
| public override bool Equals(object o) | |||
| { | |||
| CompilerInfo other = o as CompilerInfo; | |||
| if (o == null) | |||
| if (other == null) | |||
There was a problem hiding this comment.
The fact that all of our tests were passing before this change suggests that we're missing some test coverage. To go along with your fixes, can you add tests that would have failed before the changes?
There was a problem hiding this comment.
I would be glad to do this, but I doubt that my manager would approve these actions. Most likely he will say something "We are a small company and cannot develop Microsoft projects for free" :) If you wish, I could give you contacts of our CEO and you could discuss variants of paid collaboration. We really could fix all the warnings and write tests, we do such tasks. Here is the example of how we did it for Unreal Engine.
There was a problem hiding this comment.
In general, we consider src/tests to be a unit, such that we ask contributors changing src to ensure that the tests for the thing being changed are appropriate. Doing so helps to ensure that the fix is actually a fix (and not a regression, as it was in the case of one of your changes you backed out... imagine how bad things could have been if we didn't have those tests and we merged the change). We can skip doing so this time for the few remaining modifications, but for any future contributions please do so. Thanks for the interest in contributing!
…otnet/corefx#16807) * PVS-Studio: fixed vulnerability CWE-476 (NULL Pointer Dereference) * Undo changes for understand what is breaking tests. Commit migrated from dotnet/corefx@44eb1db
We have found and fixed a vulnerability CWE-476 (NULL Pointer Dereference) using PVS-Studio tool.
Analyzer warning: V3080 and V3019.
PVS-Studio is a static code analyzer for C, C++ and C#.