New Year's check .NET Core Libraries (CoreFX)

About a year ago, Microsoft released open source source code for projects such as CoreCLR and CoreFX. Until recently, the last project was not interesting to us, because it was written in C #, not C ++. But with the release of the new version of PVS-Studio 6.00, which supports projects in the C # programming language, I decided to go back to CoreFX and write an article.

Introduction

The .NET Core is a modular implementation of libraries and a runtime environment that includes a subset of the .NET Framework. The .NET Core consists of a set of libraries called “CoreFX” and a small optimized working environment “CoreCLR”.

.NET Core is distributed open source, which is available on GitHub:

• .NET Core Libraries (CoreFX)
• .NET Core Common Language Runtime (CoreCLR)

These are large Microsoft products that contain high-quality source code, but you can still find suspicious parts of the code.
')
You can read about CoreCLR verification in the article " PVS-Studio: 25 Suspicious Fragments of Code from CoreCLR ".

The CoreFX project, which will be discussed in the article, was tested using the PVS-Studio 6.00 static analyzer, which now supports C #!

Test results

Preparing an article about checking an open project , we provide in it information far from all warnings issued by the static analyzer. Therefore, we recommend that the authors of the project independently perform an analysis and study all the messages issued by the analyzer.

The most dangerous places found

V3027 The variable "start.BaseMapping" was used in the same logical expression. Mappings.cs 598

internal void SetSequence() { if (TypeDesc.IsRoot) return; StructMapping start = this; // find first mapping that does not have the sequence set while (!start.BaseMapping.IsSequence && //<== start.BaseMapping != null && //<==??? !start.BaseMapping.TypeDesc.IsRoot) start = start.BaseMapping; .... } 

There is a serious logical error in the code! In the body of the loop, an object named 'start' is changed at each iteration, and the loop is executed while the object is in a certain state. BUT, the verification of the condition “start.BaseMapping! = Null” is performed after referring to “start.BaseMapping.IsSequence”, and this may lead to dereference of the zero reference.

V3019 Possibly incorrectly variable conversion compared to null after type conversion using 'as' keyword. Check variables 'comparand', 'comparedCredentialKey'. CredentialCache.cs 4007

 public override bool Equals(object comparand) { CredentialHostKey comparedCredentialKey = comparand as CredentialHostKey; if (comparand == null) { // This covers also the compared == null case return false; } bool equals = string.Equals(AuthenticationType, comparedCredentialKey.AuthenticationType, .... .... } 

An object can come in a function of any type or null. If null comes, this case will be processed correctly. If this is an object of this type that cannot be converted to the “CredentialHostKey” type, then an error will occur when accessing “comparedCredentialKey.AuthenticationType”, since The "comparedCredentialKey" variable can be null.

Most likely, they wanted to write this:

 CredentialHostKey comparedCredentialKey = comparand as CredentialHostKey; if (comparedCredentialKey == null) { return false; } 

Similar place in the code:

• V3019 Possibly incorrectly variable conversion compared to null after type conversion using 'as' keyword. Check variables 'comparand', 'comparedCredentialKey'. CredentialCache.cs 497

V3008 The 'HResult' variable is assigned values ​​twice successively. Perhaps this is a mistake. Check lines: 169, 166. WebSocketException.cs 169

 private void SetErrorCodeOnError(int nativeError) { if (!Succeeded(nativeError)) { HResult = nativeError; } HResult = nativeError; //<==??? } 

For some reason, regardless of the condition, the variable “HResult” always takes the same value. Most likely the function should be implemented in another way.

V3008 The 'ResPrec' variable is assigned values ​​twice successively. Perhaps this is a mistake. Check lines: 1735, 1731. SQLDecimal.cs 1735

 public static SqlDecimal operator /(SqlDecimal x, SqlDecimal y) { int ResPrec; .... ResPrec = ResScale + x.m_bPrec + y.m_bPrec + 1; //<== MinScale = Math.Min(ResScale, s_cNumeDivScaleMin); ResInteger = Math.Min(ResInteger, s_NUMERIC_MAX_PRECISION); ResPrec = ResInteger + ResScale; //<== if (ResPrec > s_NUMERIC_MAX_PRECISION) ResPrec = s_NUMERIC_MAX_PRECISION; .... } 

It is very suspicious that the value of the variable “ResPrec” is calculated using a certain formula, but then it is simply triturated by another value.

V3020 An unconditional 'return' within a loop. Enumerable.cs 517

 public override bool MoveNext() { switch (state) { case 1: _enumerator = _source.GetEnumerator(); state = 2; goto case 2; case 2: while (_enumerator.MoveNext()) { current = _selector(_enumerator.Current); return true; } Dispose(); break; } return false; } 

Strangely, in the body of the “while” loop, the function exits without any condition. Perhaps there is an error in the code.

Another similar cycle:

• V3020 An unconditional 'return' within a loop. JsonDataContract.cs 128

V3008 The 'prefix' variable is assigned values ​​twice successively. Perhaps this is a mistake. Check lines: 953, 952. XmlSerializationWriter.cs 953

 protected void WriteAttribute(string localName, string ns, ....) { .... string prefix = localName.Substring(0, colon); prefix = _w.LookupPrefix(ns); _w.WriteStartAttribute(prefix, localName.Substring(colon + 1), ns); .... } 

In the 'prefix' variable, the substring from the 'localName' is stored with a long “colon”, then this value is ground to another. Further along the code, you can see that the remaining substring from 'localName' is used, and the first part was lost. Very doubtful code.

V3030 Recurring check. The 'baseTableRowCounts == null' condition was already verified in line 68. MetadataAggregator.cs 70

 private MetadataAggregator(....) { .... if (baseTableRowCounts == null) //<== { if (baseReader == null) { throw new ArgumentNullException("deltaReaders"); } if (baseReader.GetTableRowCount(TableIndex.EncMap) != 0) { throw new ArgumentException("....", "baseReader"); } CalculateBaseCounts(baseReader, out baseTableRowCounts, //<== out baseHeapSizes); } else { if (baseTableRowCounts == null) //<==??? { throw new ArgumentNullException("baseTableRowCounts"); } .... } .... } 

The analyzer detected a condition that has already been tested. If you look at the code snippet, then the last check in the 'else' - “baseTableRowCounts == null” does not make sense. But above the code you can see that if the variable “baseTableRowCounts” is null, then its value is attempted to be changed by calling the function CalculateBaseCounts (). After this function, most likely, there is not enough additional checking “baseTableRowCounts == null”. Those. Most likely the code should look like this:

 private MetadataAggregator(....) { .... if (baseTableRowCounts == null) { if (baseReader == null) { throw new ArgumentNullException("deltaReaders"); } if (baseReader.GetTableRowCount(TableIndex.EncMap) != 0) { throw new ArgumentException("....", "baseReader"); } CalculateBaseCounts(baseReader, out baseTableRowCounts, out baseHeapSizes); if (baseTableRowCounts == null) { throw new ArgumentNullException("baseTableRowCounts"); } } else { .... } .... } 

The remaining warnings

V3022 Expression 'readercount> = 0' is always true. Unsigned type value is always> = 0. ReaderWriterLockSlim.cs 977

 private void ExitAndWakeUpAppropriateWaitersPreferringWriters() { .... uint readercount = GetNumReaders(); .... if (readercount == 1 && _numWriteUpgradeWaiters > 0) { .... } else if (readercount == 0 && _numWriteWaiters > 0) { ExitMyLock(); _writeEvent.Set(); } else if (readercount >= 0) { .... } else ExitMyLock(); .... } 

The “readercount” variable is of unsigned type, so the condition “readercount> = 0” does not make sense. Most likely, before it was a sign type, but then for the ExitMyLOck () function in the last 'else' there was at least some chance of being executed. Now this code never gets control. It is necessary to rewrite this place.

V3014 It is likely that a variable is being incremented inside the 'for' operator. Consider reviewing 'i'. RegexCharClass.cs 1094

 private void Canonicalize() { .... for (i = 1, j = 0; ; i++) { for (last = _rangelist[j]._last; ; i++) { if (i == _rangelist.Count || last == LastChar) { done = true; break; } if ((CurrentRange = _rangelist[i])._first > last + 1) break; if (last < CurrentRange._last) last = CurrentRange._last; } _rangelist[j] = new SingleRange(_rangelist[j]._first, last); j++; if (done) break; if (j < i) _rangelist[j] = _rangelist[i]; } _rangelist.RemoveRange(j, _rangelist.Count - j); .... } 

The analyzer detected a change in the counter of one cycle in another. It is difficult to say whether there is an error in this function, but it is not written very clearly. It is possible somewhere to make a mistake in the index when accessing an array, since in such a code it is difficult to follow the change of one counter in several cycles.

V3004 The 'then' statement is equivalent to the 'else' statement. XmlSerializationWriterILGen.cs 1213

 private void WriteMember(...., TypeDesc memberTypeDesc, ....) { .... if (memberTypeDesc.IsArray) { LocalBuilder localI = ilg.DeclareOrGetLocal(typeof(Int32), iVar); ilg.For(localI, 0, ilg.GetLocal(aVar)); } else { LocalBuilder localI = ilg.DeclareOrGetLocal(typeof(Int32), iVar); ilg.For(localI, 0, ilg.GetLocal(aVar)); } .... } 

A condition that does not affect anything, because one code will always be executed. Classic copy-paste.

V3004 The 'then' statement is equivalent to the 'else' statement. SqlUtil.cs 93

 internal static void ContinueTask(....) { .... if (connectionToDoom != null || connectionToAbort != null) { try { onSuccess(); } catch (Exception e) { completion.SetException(e); } } else { // no connection to doom - reliability section not required try { onSuccess(); } catch (Exception e) { completion.SetException(e); } } .... } 

Here, too, there is something a lot of the same code in the condition, although the commentary says that the situations are different.

Conclusion

Here is another project from Microsoft. For such a volume, the project has a fairly high-quality code, but programmers can still make mistakes. The article is an overview and contains far from all analyzer warnings that were received in the report.

Two very important circumstances contribute to quality code:

1. Regular static analysis of the project, not a one-time;
2. Viewing analyzer warnings by corresponding code snippets.

We hope you enjoyed this article. We promise to continue to delight our readers with checks of interesting open projects in C / C ++ and C #.

Thanks for attention. And bezbezhnogo you code in the New Year!


If you want to share this article with an English-speaking audience, then please use the link to the translation: Svyatoslav Razmyslov. Christmas Analysis of .NET Core Libraries (CoreFX) .