# Top 10 C\# projects errors found in 2016 To measure the efficiency of our analyzer, and also to promote the methodology of static analysis, we regularly analyze open source projects for bugs and write articles about the results\. 2016 was no exception\. This year is especially important as it is the year of the "growth" of the C\# analyzer\. PVS\-Studio has obtained a large number of new C\# diagnostics, an improved virtual values mechanism \(symbolic execution\) and much more\. Based on the results of our teamwork, I compiled a kind of chart of the most interesting bugs, found in various C\# projects in 2016\. ![0480_Top_10_CSharp_Bugs_2016/image1.png](https://import.viva64.com/docx/blog/0480_Top_10_CSharp_Bugs_2016/image1.png) **Tenth place: when a minute doesn't always have 60 seconds** I'll start the chart with a bug from the Orchard CMS project\. The description of the error can be found in this [article](https://pvs-studio.com/en/blog/posts/csharp/0456/)\. In general, the full list of all the articles we checked is [here](https://pvs-studio.com/en/blog/inspections/)\. [V3118](https://pvs-studio.com/en/docs/warnings/v3118/) Seconds component of TimeSpan is used, which does not represent full time interval\. Possibly 'TotalSeconds' value was intended instead\. AssetUploader\.cs 182 ```cpp void IBackgroundTask.Sweep() { .... // Don't flood the database with progress updates; // Limit it to every 5 seconds. if ((_clock.UtcNow - lastUpdateUtc).Seconds >= 5) { .... } } ``` The developer mistakenly used _Seconds_ instead of _TotalSeconds_ in this case\. Thus, we will get not the full number of seconds between the dates _\_clock\.UtcNow_ and _lastUpdateUtc, _as the developer expected, but only the _Seconds_ component of the interval\. For example, for the time interval of 1 minute 4 seconds it will be not 64 seconds, but 4 seconds\. Incredible, but even experienced developers make such mistakes\. **Ninth place: the expression is always true** The following error is from the [article](https://pvs-studio.com/en/blog/posts/csharp/0440/) about the analysis of GitExtensions\. [V3022](https://pvs-studio.com/en/docs/warnings/v3022/) Expression 'string\.IsNullOrEmpty\(rev1\) \|\| string\.IsNullOrEmpty\(rev2\)' is always true\. GitUI FormFormatPatch\.cs 155 ```cpp string rev1 = ""; string rev2 = ""; var revisions = RevisionGrid.GetSelectedRevisions(); if (revisions.Count > 0) { rev1 = ....; rev2 = ....; .... } else if (string.IsNullOrEmpty(rev1) || string.IsNullOrEmpty(rev2)) // <= { MessageBox.Show(....); return; } ``` Note the _else_ keyword\. Most likely, this is not the right place for it\. Inattention during refactoring, or just banal tiredness of a programmer, and we are getting a radical change in the logic of the program, which leads to unpredictable behavior\. It's great that a static analyzer is never tired\. **Eighth place: a possible typo** In the [article](https://pvs-studio.com/en/blog/posts/csharp/0412/) about the analysis of FlashDevelop source code, we see a peculiar error, caused by a typo\. [V3056](https://pvs-studio.com/en/docs/warnings/v3056/) Consider reviewing the correctness of 'a1' item's usage\. LzmaEncoder\.cs 225 ```cpp public void SetPrices(....) { UInt32 a0 = _choice.GetPrice0(); UInt32 a1 = _choice.GetPrice1(); UInt32 b0 = a1 + _choice2.GetPrice0(); // <= UInt32 b1 = a1 + _choice2.GetPrice1(); .... } ``` I agree with the analyzer, as well as the author of the article\. It feels like _a0 _should be used instead of the _a1_ variable in the marked line\. In any case, it wouldn't hurt to give more meaningful names to variables\. **Seventh place: logic error** This [article](https://pvs-studio.com/en/blog/posts/csharp/0461/) was written based on the second check of the Umbraco project\. An example of an interesting, in my opinion, error from it\. [V3022](https://pvs-studio.com/en/docs/warnings/v3022/) Expression 'name \!\= "Min" \|\| name \!\= "Max"' is always true\. Probably the '&&' operator should be used here\. DynamicPublishedContentList\.cs 415 ```cpp private object Aggregate(....) { .... if (name != "Min" || name != "Max") { throw new ArgumentException( "Can only use aggregate min or max methods on properties which are datetime"); } .... } ``` An exception of the ArgumentException type will be thrown for any value of _name_ variable\. It's because of the erroneous use of the \|\| operator instead of && in the condition\. **Sixth place: incorrect loop condition** The [article](https://pvs-studio.com/en/blog/posts/csharp/0410/) about the check of Accord\.Net has a description of several amusing bugs\. I have chosen two, one of which is related to a typo again\. [V3015](https://pvs-studio.com/en/docs/warnings/v3015/) It is likely that the wrong variable is being compared inside the 'for' operator\. Consider reviewing 'i' Accord\.Audio SampleConverter\.cs 611 ```cpp public static void Convert(float[][] from, short[][] to) { for (int i = 0; i < from.Length; i++) for (int j = 0; i < from[0].Length; j++) to[i][j] = (short)(from[i][j] * (32767f)); } ``` There is an error in the condition of the second _for_ loop, whose counter is the _j_ variable\. Using the variable names i and j for the counters is some sort of classic of programming\. Unfortunately, these variables are very similar looking, so the developers often make mistakes in such code\. I do not think that in this case it is worth any recommendation to use more meaningful names\. Nobody will do that anyway\. So, here is another recommendation: use static analyzers\! **Fifth** **place:** **using** **a** **bitwise** **operator** **instead** **of** **a** **logical** **operator** One more interesting and quite a wide\-spread error from the [article](https://pvs-studio.com/en/blog/posts/csharp/0410/) about the analysis of Accord\.Net\. [V3093](https://pvs-studio.com/en/docs/warnings/v3093/) The '&' operator evaluates both operands\. Perhaps a short\-circuit '&&' operator should be used instead\. Accord\.Math JaggedSingularValueDecompositionF\.cs 461 ```cpp public JaggedSingularValueDecompositionF(....) { .... if ((k < nct) & (s[k] != 0.0)) .... } ``` It is obvious that even if the first condition is true, the erroneous use of & instead of && will lead to the check of the second condition, which in turn, will cause array index out of bounds\. **Fourth place: quotation mark and\.\.\. a quotation mark again** The fourth place is taken by the Xamarin\.Forms project; the full article about its check is [here](https://pvs-studio.com/en/blog/posts/csharp/0400/)\. [V3038](https://pvs-studio.com/en/docs/warnings/v3038/) The first argument of 'Replace' function is equal to the second argument\. ICSharpCode\.Decompiler ReflectionDisassembler\.cs 349 ```cpp void WriteSecurityDeclarationArgument(CustomAttributeNamedArgument na) { .... output.Write("string('{0}')", NRefactory.CSharp .TextWriterTokenWriter .ConvertString( (string)na.Argument.Value).Replace("'", "\'")); .... } ``` In this case, the quotation mark will be replaced with\.\.\. a quotation mark\. I do not think that this is what the developer wanted\. The analyzer did a great job\! **Third** **place:** **ThreadStatic** In third place, is the Mono project that was also rich in amusing bugs\. Here is the [article](https://pvs-studio.com/en/blog/posts/csharp/0431/) about its check\. One of these bugs is a real rarity\. [V3089](https://pvs-studio.com/en/docs/warnings/v3089/) Initializer of a field marked by \[ThreadStatic\] attribute will be called once on the first accessing thread\. The field will have default value on different threads\. System\.Data\.Linq\-net\_4\_x Profiler\.cs 16 ```cpp static class Profiler { [ThreadStatic] private static Stopwatch timer = new Stopwatch(); .... } ``` In a nutshell: the field marked by the _ThreadStatic_ attribute is initialized incorrectly\. In the [documentation](https://pvs-studio.com/en/docs/warnings/v3089/) of the diagnostic you may see a detailed description of the situation, and some tips on how to avoid such errors\. It's the perfect example of an error that is not that easy to find and fix using the usual methods\. **Second place: Copy\-Paste \- classic\!** One of the classical, to my mind, errors of the Copy\-Paste type which was found in the already mentioned Mono project \(_see the full [article](https://pvs-studio.com/en/blog/posts/csharp/0431/)\)\._ [V3012](https://pvs-studio.com/en/docs/warnings/v3012/) The '?:' operator, regardless of its conditional expression, always returns one and the same value: Color\.FromArgb \(150, 179, 225\)\. ProfessionalColorTable\.cs 258 Here is a short code fragment, where this bug was found: ```cpp button_pressed_highlight = use_system_colors ? Color.FromArgb (150, 179, 225) : Color.FromArgb (150, 179, 225); ``` You may ask: "Is it really such a huge mistake?" "Does this obvious mistake really deserve second place in the chart?" The thing is that this code fragment was purposefully formatted to make it more visible\. And now imagine that you have the task of finding an error like this without any tools\. So, those with weak nerves, we would ask not to look at this; here is a screenshot of a full code fragment with an error \(click on the image to enlarge\): ![0480_Top_10_CSharp_Bugs_2016/image2.png](https://import.viva64.com/docx/blog/0480_Top_10_CSharp_Bugs_2016/image2.png) Apparently, this mistake can be done by any programmer, regardless of the qualification\. Successfully finding such errors demonstrates the full power of static analysis\. **First** **place:** **PVS\-Studio** No, it's not an illusion\. "PVS\-Studio" really is written here\. It took first place in our chart\. Not because it is a great static analyzer, but because our team has ordinary people, who make simple human errors when writing code\. This was the main topic of an [article](https://pvs-studio.com/en/blog/posts/csharp/0382/) we wrote previously\. With the help of PVS\-Studio, we detected two errors in the code of PVS\-Studio itself\. [V3022](https://pvs-studio.com/en/docs/warnings/v3022/) Expression 'RowsCount \> 100000' is always false\. ProcessingEngine\.cs 559 [V3022](https://pvs-studio.com/en/docs/warnings/v3022/) Expression 'RowsCount \> 200000' is always false\. ProcessingEngine\.cs 561 ```cpp public void ProcessFiles(....) { .... int RowsCount = DynamicErrorListControl.Instance.Plog.NumberOfRows; if (RowsCount > 20000) DatatableUpdateInterval = 30000; //30s else if (RowsCount > 100000) DatatableUpdateInterval = 60000; //1min else if (RowsCount > 200000) DatatableUpdateInterval = 120000; //2min .... } ``` The result of this code fragment \(on condition that _RowsCount_ _\>_ _20000_\) will always be a value _DatatableUpdateInterval_ equal to 30000\. Fortunately, we have already done some work in this sphere\. Thanks to widely used incremental analysis in our team, the articles "bugs in PVS\-Studio found by PVS\-Studio" are very unlikely to appear\. **Conclusion** I should note that anyone can make a personal chart of bugs found using a [free](https://pvs-studio.com/en/blog/posts/0457/) version of PVS\-Studio static analyzer for checking individual projects\. [Download](https://pvs-studio.com/en/pvs-studio/) and try PVS\-Studio\. To purchase a commercial license, please [contact](https://pvs-studio.com/en/about-feedback/) us via email\. You can also write to us to get a temporary license key for a comprehensive investigation of PVS\-Studio, if you want to avoid the [limitations](https://pvs-studio.com/en/docs/manual/0009/) of the demo version\.