WinForms: Errors, Holmes

We love looking for bugs in Microsoft projects. Why? It's simple: their projects are usually easy to check (work can be done immediately in the Visual Studio environment, for which PVS-Studio has a convenient plug-in) and they contain few errors. Therefore, the usual algorithm of work is as follows: find and download an open project from MS; check him; choose interesting mistakes; make sure that they are few; write an article without forgetting to praise the developers. Sumptuously! Win-win-win: it took a little time, the manual is glad to see new materials on the blog, and karma is in perfect order. But this time something went wrong. Let's see what interesting was found in the source code of Windows Forms and whether Microsoft should be praised this time.



Introduction



In early December 2018, Microsoft announced the release of .NET Core 3 Preview 1. A little earlier (approximately from mid-October), GitHub began active work on the publication of the source code for Windows Forms , the .NET Core UI platform for creating Windows desktop applications. You can see the commit statistics here . Now anyone can download the WinForms source code for review.



I also downloaded the sources to look for errors there using PVS-Studio. Check did not cause difficulties. Required: Visual Studio 2019, .NET Core 3.0 SDK Preview, PVS-Studio. And here is the analyzer warning log received.



Having received the PVS-Studio log, I usually sort it in ascending order of diagnostic numbers (the window with the PVS-Studio message log in Visual Studio has various options for sorting and filtering the list). This allows you to work with groups of errors of the same type, which greatly simplifies the analysis of the source code. I mark interesting errors in the list with an asterisk, and only then, after analyzing the entire log, I write out code fragments and make a description. And since there are usually few errors, I mix them up, trying to place the most interesting ones at the beginning and end of the article. But this time, the errors turned out to be a bit much (oh, it was not possible to save the intrigue for a long time), and I will give them in order of diagnostic numbers.



What was found? 833 warnings of the High and Medium confidence levels (249 and 584, respectively) were issued for 540,000 lines of code (excluding empty ones) in 1670 cs files. And yes, according to tradition, I did not check the tests and did not consider the warnings of the Low confidence level (215 were issued). According to my previous observations, there are too many warnings for the project from MS. But not all warnings are errors.



For this project, the number of false positives was about 30%. In about 20% of cases, I just could not make an exact conclusion about whether this is a mistake or not, as I am not well acquainted with the code. Well, at least 20% of the errors I missed can be attributed to the human factor: haste, fatigue, etc. By the way, the opposite effect is also possible: I looked through some of the same type of triggering, the number of which could reach 70-80, through one that occasionally could increase the number of errors that I considered real.



In any case, 30% of warnings indicate real errors, which is a very large percentage, given that the analyzer has not been pre-configured.



So, the number of errors that I was able to detect was about 240, which is within the limits of the statistics. I repeat, in my opinion, for a project from MS this is not the most outstanding result (although this will be only 0.44 errors per 1000 lines of code), and there are probably more real errors in WinForms code. I propose to talk about the reasons at the end of the article, but now let's see the most interesting errors.



Mistakes



PVS-Studio: V3003 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 213, 224. ButtonStandardAdapter.cs 213

void PaintWorker(PaintEventArgs e, bool up, CheckState state) { up = up && state == CheckState.Unchecked; .... if (up & IsHighContrastHighlighted()) { .... } else if (up & IsHighContrastHighlighted()) { .... } else { .... } .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

In if and else if blocks, the same condition is checked. It looks like copy-paste. Is this a mistake? If you look at the declaration of the IsHighContrastHighlighted method, there is doubt:

 protected bool IsHighContrastHighlighted() { return SystemInformation.HighContrast && Application.RenderWithVisualStyles && (Control.Focused || Control.MouseIsOver || (Control.IsDefault && Control.Enabled)); }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

The method can probably return different values ​​in successive calls. And what is done in the calling method looks strange, of course, but has the right to life. However, I would advise the authors to take a look at this piece of code. Just in case. And yet, this is a good example of how difficult it is to draw conclusions when analyzing unfamiliar code.



PVS-Studio: V3004 The 'then' statement is equivalent to the 'else' statement. RichTextBox.cs 1018

 public int SelectionCharOffset { get { int selCharOffset = 0; .... NativeMethods.CHARFORMATA cf = GetCharFormat(true); // if the effects member contains valid info if ((cf.dwMask & RichTextBoxConstants.CFM_OFFSET) != 0) { selCharOffset = cf.yOffset; // <= } else { // The selection contains characters of different offsets, // so we just return the offset of the first character. selCharOffset = cf.yOffset; // <= } .... } .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

And here the copy-paste error is definitely made. Regardless of the condition, the selCharOffset variable will always get the same value.



There were two more similar errors in WinForms code:

• V3004 The 'then' statement is equivalent to the 'else' statement. SplitContainer.cs 1700
• V3004 The 'then' statement is equivalent to the 'else' statement. ToolstripProfessionalRenderer.cs 371

PVS-Studio: V3008 The variable is assigned values ​​twice successively. Perhaps this is a mistake. Check lines: 681, 680. ProfessionalColorTable.cs 681

 internal void InitSystemColors(ref Dictionary<KnownColors, Color> rgbTable) { .... rgbTable[ProfessionalColorTable.KnownColors.msocbvcrCBBdrOuterDocked] = buttonFace; rgbTable[ProfessionalColorTable.KnownColors.msocbvcrCBBdrOuterDocked] = buttonShadow; .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

The method populates the rgbTable dictionary. The analyzer pointed to a code fragment where two different values ​​are written successively on the same key. And everything would be fine, but there were 16 more such places in this method. This no longer looks like a single error. But why do this, for me it remains a mystery. I did not find signs of autogenerated code. In the editor, it looks like this:

I will give the first ten operations with a list:

1. V3008 The variable is assigned values ​​twice successively. Perhaps this is a mistake. Check lines: 785, 784. ProfessionalColorTable.cs 785
2. V3008 The variable is assigned values ​​twice successively. Perhaps this is a mistake. Check lines: 787, 786. ProfessionalColorTable.cs 787
3. V3008 The variable is assigned values ​​twice successively. Perhaps this is a mistake. Check lines: 789, 788. ProfessionalColorTable.cs 789
4. V3008 The variable is assigned values ​​twice successively. Perhaps this is a mistake. Check lines: 791, 790. ProfessionalColorTable.cs 791
5. V3008 The variable is assigned values ​​twice successively. Perhaps this is a mistake. Check lines: 797, 796. ProfessionalColorTable.cs 797
6. V3008 The variable is assigned values ​​twice successively. Perhaps this is a mistake. Check lines: 799, 798. ProfessionalColorTable.cs 799
7. V3008 The variable is assigned values ​​twice successively. Perhaps this is a mistake. Check lines: 807, 806. ProfessionalColorTable.cs 807
8. V3008 The variable is assigned values ​​twice successively. Perhaps this is a mistake. Check lines: 815, 814. ProfessionalColorTable.cs 815
9. V3008 The variable is assigned values ​​twice successively. Perhaps this is a mistake. Check lines: 817, 816. ProfessionalColorTable.cs 817
10. V3008 The variable is assigned values ​​twice successively. Perhaps this is a mistake. Check lines: 823, 822. ProfessionalColorTable.cs 823

PVS-Studio: V3011 Two opposite conditions were encountered. The second condition is always false. Check lines: 5242, 5240. DataGrid.cs 5242

 private void CheckHierarchyState() { if (checkHierarchy && listManager != null && myGridTable != null) { if (myGridTable == null) // <= { // there was nothing to check return; } for (int j = 0; j < myGridTable.GridColumnStyles.Count; j++) { DataGridColumnStyle gridColumn = myGridTable.GridColumnStyles[j]; } checkHierarchy = false; } }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

The return statement will never be executed. Most likely, the condition myGridTable! = Null in the external if block was added later during refactoring. And now checking myGridTable == null is pointless. To improve the quality of the code, remove this check.



PVS-Studio: V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'left', 'cscLeft'. TypeCodeDomSerializer.cs 611



PVS-Studio: V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'right', 'cscRight'. TypeCodeDomSerializer.cs 615

 public int Compare(object left, object right) { OrderedCodeStatementCollection cscLeft = left as OrderedCodeStatementCollection; OrderedCodeStatementCollection cscRight = right as OrderedCodeStatementCollection; if (left == null) { return 1; } else if (right == null) { return -1; } else if (right == left) { return 0; } return cscLeft.Order - cscRight.Order; // <= }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

The analyzer immediately issued two warnings for the Compare method. What is the problem? It is that the values ​​of cscLeft and cscRight are not checked for null equality. They can get this value after unsuccessful casting to the type OrderedCodeStatementCollection . Then an exception will be thrown in the last return statement . Such a situation is possible when all checks for left and right pass and do not lead to a preliminary exit from the method.



To fix the code, cscLeft / cscRight should be used everywhere instead of left / right .



PVS-Studio: V3020 An unconditional 'break' within a loop. SelectionService.cs 421

 void ISelectionService.SetSelectedComponents( ICollection components, SelectionTypes selectionType) { .... // Handle the click case object requestedPrimary = null; int primaryIndex; if (fPrimary && 1 == components.Count) { foreach (object o in components) { requestedPrimary = o; if (o == null) { throw new ArgumentNullException(nameof(components)); } break; } } .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

This fragment refers, rather, to the "with smell" code. There is no mistake here. But questions arise to the way the foreach cycle is organized. Why is it needed here - it is clear: due to the need to extract the elements of the collection, passed as ICollection . But why in the loop, originally designed for a single iteration (a precondition is the presence of a single element in the components collection), an additional safety net in the form of break was required? Probably, the answer can be considered: "It has historically developed." The code looks ugly.



PVS-Studio: V3022 Expression 'ocxState! = Null' is always true. AxHost.cs 2186

 public State OcxState { .... set { .... if (value == null) { return; } .... ocxState = value; if (ocxState != null) // <= { axState[manualUpdate] = ocxState._GetManualUpdate(); licenseKey = ocxState._GetLicenseKey(); } else { axState[manualUpdate] = false; licenseKey = null; } .... } }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

Due to a logical error, a “dead code” was formed in this fragment. Expressions in the else block will never be executed.



PVS-Studio: V3027 The variable 'e' was utilized in the logical expression before it was verified against null in the same logical expression. ImageEditor.cs 99

 public override object EditValue(....) { .... ImageEditor e = ....; Type myClass = GetType(); if (!myClass.Equals(e.GetType()) && e != null && myClass.IsInstanceOfType(e)) { .... } .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

The variable e in the condition is first used, and then checked for null . Hi, NullReferenceException .



Another similar error:



PVS-Studio: V3027 The variable 'dropDownItem' was utilized in the logical expression before it was verified against null in the same logical expression. ToolStripMenuItemDesigner.cs 1351

 internal void EnterInSituEdit(ToolStripItem toolItem) { .... ToolStripDropDownItem dropDownItem = toolItem as ToolStripDropDownItem; if (!(dropDownItem.Owner is ToolStripDropDownMenu) && dropDownItem != null && dropDownItem.Bounds.Width < commitedEditorNode.Bounds.Width) { .... } .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

A situation similar to the previous one, only with the variable dropDownItem . I think that such errors appear as a result of inattention during refactoring. Probably part of the condition ! (DropDownItem.Owner is ToolStripDropDownMenu) was added to the code later.



PVS-Studio: V3030 Recurring check. The 'columnCount> 0' condition was already verified in line 3900. ListView.cs 3903

 internal ColumnHeader InsertColumn( int index, ColumnHeader ch, bool refreshSubItems) { .... // Add the column to our internal array int columnCount = (columnHeaders == null ? 0 : columnHeaders.Length); if (columnCount > 0) { ColumnHeader[] newHeaders = new ColumnHeader[columnCount + 1]; if (columnCount > 0) { System.Array.Copy(columnHeaders, 0, newHeaders, 0, columnCount); } .... } .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

A mistake that may seem harmless. Indeed, well, an extra check is performed, which does not affect the logic of work. And sometimes they even do this when you need to re-check the status of a visual component, for example, by getting the number of entries in the list. Only in this case, they double-check the local variable columnCount . This is very suspicious. Either they wanted to check another variable, or in one of the checks they use the wrong condition.



PVS-Studio: V3061 Parameter 'lprcClipRect' is always rewritten in method body before being used. WebBrowserSiteBase.cs 281

 int UnsafeNativeMethods.IOleInPlaceSite.GetWindowContext( out UnsafeNativeMethods.IOleInPlaceFrame ppFrame, out UnsafeNativeMethods.IOleInPlaceUIWindow ppDoc, NativeMethods.COMRECT lprcPosRect, NativeMethods.COMRECT lprcClipRect, NativeMethods.tagOIFI lpFrameInfo) { ppDoc = null; ppFrame = Host.GetParentContainer(); lprcPosRect.left = Host.Bounds.X; lprcPosRect.top = Host.Bounds.Y; .... lprcClipRect = WebBrowserHelper.GetClipRect(); // <= if (lpFrameInfo != null) { lpFrameInfo.cb = Marshal.SizeOf<NativeMethods.tagOIFI>(); lpFrameInfo.fMDIApp = false; .... } return NativeMethods.S_OK; }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

Unobvious mistake. Yes, the lprcClipRect parameter is really initialized with a new value, without using it in any way. But what does this result in the end? I think that somewhere in the calling code, the link passed through this parameter will remain unchanged, although it was intended differently. Indeed, look at working with other variables in this method. Even its name (the “Get” prefix) hints that some initialization will be made inside the method through the passed parameters. And so it is. The first two parameters ( ppFrame and ppDoc ) are passed with the out modifier and get new values. Links lprcPosRect and lpFrameInfo are used to access the fields of the class and to initialize them. And only lprcClipRect gets out of the general list. Most likely, the out or ref modifier is required for this parameter.



PVS-Studio: V3066 Possible incorrect order of arguments passed to 'AdjustCellBorderStyle' method: 'isFirstDisplayedRow' and 'isFirstDisplayedColumn'. DataGridViewComboBoxCell.cs 1934

 protected override void OnMouseMove(DataGridViewCellMouseEventArgs e) { .... dgvabsEffective = AdjustCellBorderStyle( DataGridView.AdvancedCellBorderStyle, dgvabsPlaceholder, singleVerticalBorderAdded, singleHorizontalBorderAdded, isFirstDisplayedRow, // <= isFirstDisplayedColumn); // <= .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

The analyzer suspected that the last two arguments were mixed up. Let's take a look at the declaration of the AdjustCellBorderStyle method:

 public virtual DataGridViewAdvancedBorderStyle AdjustCellBorderStyle( DataGridViewAdvancedBorderStyledataGridViewAdvancedBorderStyleInput, DataGridViewAdvancedBorderStyle dataGridViewAdvancedBorderStylePlaceholder, bool singleVerticalBorderAdded, bool singleHorizontalBorderAdded, bool isFirstDisplayedColumn, bool isFirstDisplayedRow) { .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

Looks like a mistake. Yes, often some arguments are deliberately passed in the reverse order, for example, to swap some variables. But I do not think that this is exactly the case. Nothing in the calling or called methods says such a usage pattern. First, bool type variables are confused. Secondly, the names of the methods are also common: no “Swap” or “Reverse”. Moreover, it is not so difficult to make a mistake like that. People often perceive the order of the row / column pair differently. For me, for example, just “row / column” is familiar. But for the author of the called AdjustCellBorderStyle method, obviously, the more familiar order is “column / row”.



PVS-Studio: V3070 Uninitialized variable 'LANG_USER_DEFAULT' is used when initializing the 'LOCALE_USER_DEFAULT' variable. NativeMethods.cs 890

 internal static class NativeMethods { .... public static readonly int LOCALE_USER_DEFAULT = MAKELCID(LANG_USER_DEFAULT); public static readonly int LANG_USER_DEFAULT = MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT); .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

A rare mistake. Confused the initialization order of the class fields. To calculate the value of the LOCALE_USER_DEFAULT field, use the LANG_USER_DEFAULT field, which is not yet initialized and has a value of 0. By the way, the LANG_USER_DEFAULT variable is not used anywhere else in the code. I was not too lazy and wrote a small console program that simulates the situation. Instead of the values ​​of some constants that are used in WinForms code, I substituted their actual values:

 internal static class NativeMethods { public static readonly int LOCALE_USER_DEFAULT = MAKELCID(LANG_USER_DEFAULT); public static readonly int LANG_USER_DEFAULT = MAKELANGID(0x00, 0x01); public static int MAKELANGID(int primary, int sub) { return ((((ushort)(sub)) << 10) | (ushort)(primary)); } public static int MAKELCID(int lgid) { return MAKELCID(lgid, 0x0); } public static int MAKELCID(int lgid, int sort) { return ((0xFFFF & lgid) | (((0x000f) & sort) << 16)); } } class Program { static void Main() { System.Console.WriteLine(NativeMethods.LOCALE_USER_DEFAULT); } }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

As a result of the launch, the following will be displayed on the console: 0. Now we swap the declaration of the LOCALE_USER_DEFAULT and LANG_USER_DEFAULT fields . The result of the program in this form: 1024. I think there is nothing more to comment on here.



PVS-Studio: V3080 Possible null dereference. Consider inspecting 'ces'. CodeDomSerializerBase.cs 562

 protected void DeserializeStatement( IDesignerSerializationManager manager, CodeStatement statement) { .... CodeExpressionStatement ces = statement as CodeExpressionStatement; if (ces != null) { .... } else { .... DeserializeExpression(manager, null, ces.Expression); // <= .... } .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

The code that should “fall” is stable enough, because you can get into the else branch just when the ces reference is null .



Another similar example:



PVS-Studio: V3080 Possible null dereference. Consider inspecting 'comboBox'. ComboBox.cs 6610

 public void ValidateOwnerDrawRegions(ComboBox comboBox, ....) { .... if (comboBox != null) { return; } Rectangle topOwnerDrawArea = new Rectangle(0, 0, comboBox.Width, innerBorder.Top); .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

The paradoxical code. Apparently, they mixed up the test by writing if (comboBox! = Null) instead of if (comboBox == null) . And so - we will receive the next NullReferenceException .



We examined two fairly obvious V3080 errors, where you can visually track the situation of the possible use of a null reference within the method. But V3080 diagnostics are much more intelligent and can look for similar errors for chains of method calls. Not so long ago, we significantly strengthened the mechanisms of dataflow and interprocedural analysis. You can read about this in the article " Nullable Reference Types in C # 8.0 and Static Analysis ". And here is a similar error found in WinForms:



PVS-Studio: V3080 Possible null dereference inside method at 'reader.NameTable'. Consider inspecting the 1st argument: contentReader. ResXResourceReader.cs 267

 private void EnsureResData() { .... XmlTextReader contentReader = null; try { if (fileContents != null) { contentReader = new XmlTextReader(....); } else if (reader != null) { contentReader = new XmlTextReader(....); } else if (fileName != null || stream != null) { .... contentReader = new XmlTextReader(....); } SetupNameTable(contentReader); // <= .... } finally { .... } .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

Look at what happens with the contentReader variable in the body of the method. After initialization with a null reference, as a result of one of the checks, the link will be initialized. However, a series of checks does not end with an else block. This means that in some rare case (or due to refactoring in the future), the link can still remain zero. Next, it will be passed to the SetupNameTable method, where it is used without any verification:

 private void SetupNameTable(XmlReader reader) { reader.NameTable.Add(ResXResourceWriter.TypeStr); reader.NameTable.Add(ResXResourceWriter.NameStr); .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

This is potentially unsafe code.



And one more error, where the analyzer had to go through a chain of calls to identify a problem:



PVS-Studio: V3080 Possible null dereference. Consider inspecting 'layout'. DockAndAnchorLayout.cs 156

 private static Rectangle GetAnchorDestination( IArrangedElement element, Rectangle displayRect, bool measureOnly) { .... AnchorInfo layout = GetAnchorInfo(element); int left = layout.Left + displayRect.X; .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

The analyzer claims that it is possible to get a null reference from the GetAnchorInfo method, which will throw an exception when calculating the left value. Let's go through the whole chain of calls and check if this is so:

 private static AnchorInfo GetAnchorInfo(IArrangedElement element) { return (AnchorInfo)element.Properties.GetObject(s_layoutInfoProperty); } public object GetObject(int key) => GetObject(key, out _); public object GetObject(int key, out bool found) { short keyIndex = SplitKey(key, out short element); if (!LocateObjectEntry(keyIndex, out int index)) { found = false; return null; } // We have found the relevant entry. See if // the bitmask indicates the value is used. if (((1 << element) & s_objEntries[index].Mask) == 0) { found = false; return null; } found = true; switch (element) { case 0: return s_objEntries[index].Value1; .... default: Debug.Fail("Invalid element obtained from LocateObjectEntry"); return null; } }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

Indeed, in some cases, the GetObject method that closes the call chain will return null , which without any additional checks will be passed to the calling method. Probably, the GetAnchorDestination method should provide for such a situation.



In WinForms code there were quite a few such errors, more than 70 . They are all similar and I will not give their description in the article.



PVS-Studio: V3091 Empirical analysis. It is possible that a typo is present inside the string literal: "ShowCheckMargin". The 'ShowCheckMargin' word is suspicious. PropertyNames.cs 136

 internal class PropertyNames { .... public static readonly string ShowImageMargin = "ShowCheckMargin"; ... public static readonly string ShowCheckMargin = "ShowCheckMargin"; .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

A good example of an error that is not so easy to detect. When initializing the fields of a class, they use the same value, although the author of the code clearly did not think so (copy-paste is to blame). The analyzer made this conclusion by comparing the names of the variables and the values ​​of the assigned strings. I gave only the error lines, but look at how it looks in the code editor:

It is the detection of such errors that demonstrates the power and infinite care of static analysis tools.



PVS-Studio: V3095 The 'currentForm' object was used before it was verified against null. Check lines: 3386, 3404. Application.cs 3386

 private void RunMessageLoopInner(int reason, ApplicationContext context) { .... hwndOwner = new HandleRef( null, UnsafeNativeMethods.GetWindowLong( new HandleRef(currentForm, currentForm.Handle), // <= NativeMethods.GWL_HWNDPARENT)); .... if (currentForm != null && ....) .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

Classic. The currentForm variable is used without any checks. But further in the code there is its check for null equality. In this case, I can advise you to be more careful when working with reference types, as well as use static analyzers :).



Another similar error:



PVS-Studio: V3095 The 'backgroundBrush' object was used before it was verified against null. Check lines: 2331, 2334. DataGrid.cs 2331

 public Color BackgroundColor { .... set { .... if (!value.Equals(backgroundBrush.Color)) // <= { if (backgroundBrush != null && BackgroundBrush != DefaultBackgroundBrush) .... } } }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

In WinForms code, I have encountered more than 60 such errors. In my opinion, all of them are quite critical and require the attention of developers. But in the article it’s not so interesting to talk about them, so I will limit myself to the two considered above.



PVS-Studio: V3125 The '_propInfo' object was used and was verified against null in different execution branches. Check lines: 996, 982. Binding.cs 996

 private void SetPropValue(object value) { .... if (....) { if .... else if (_propInfo != null) .... } else { _propInfo.SetValue(_control, value); } .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

To complete the picture - also a kind of classic, bug V3125 . The reverse situation. First, a potentially null reference is used safely, checking for null equality, but then they don’t do this in the code anymore.



And another similar error:



PVS-Studio: V3125 The 'owner' object was used after it was verified against null. Check lines: 64, 60. FlatButtonAppearance.cs 64

 public int BorderSize { .... set { .... if (owner != null && owner.ParentInternal != null) { LayoutTransaction.DoLayoutIf(....); } owner.Invalidate(); // <= .... } }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

Beauty. But this is from the point of view of an outside researcher. Indeed, in addition to these two V3125s , the analyzer found more than 50 similar patterns in WinForms code. Developers have work to do.



And finally - a rather interesting error, in my opinion.



PVS-Studio: V3137 The 'hCurrentFont' variable is assigned but is not used by the end of the function. DeviceContext2.cs 241

 sealed partial class DeviceContext : .... { WindowsFont selectedFont; .... internal void DisposeFont(bool disposing) { if (disposing) { DeviceContexts.RemoveDeviceContext(this); } if (selectedFont != null && selectedFont.Hfont != IntPtr.Zero) { IntPtr hCurrentFont = IntUnsafeNativeMethods.GetCurrentObject( new HandleRef(this, hDC), IntNativeMethods.OBJ_FONT); if (hCurrentFont == selectedFont.Hfont) { // select initial font back in IntUnsafeNativeMethods.SelectObject(new HandleRef(this, Hdc), new HandleRef(null, hInitialFont)); hCurrentFont = hInitialFont; // <= } selectedFont.Dispose(disposing); selectedFont = null; } } .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

Let's see what the analyzer alerted, and why the fact that a variable is assigned a value, but it is not used in the method in the future, may indicate a problem.



A partial class is declared in the file DeviceContext2.cs . The DisposeFont method is used to free resources after working with graphics: device context and fonts. For a better understanding, I presented the entire DisposeFont method. Pay attention to the local variable hCurrentFont . The problem is that declaring this variable in a method hides the class field of the same name. I found two methods of the DeviceContext class where a field called hCurrentFont is used :

 public IntPtr SelectFont(WindowsFont font) { .... hCurrentFont = font.Hfont; .... } public void ResetFont() { .... hCurrentFont = hInitialFont; }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

Take a look at the ResetFont method. The last line there is exactly what the DisposeFont method does in the nested if block (the analyzer points to this place). And this hCurrentFont field of the same name was declared in another part of the partial class in the DeviceContext.cs file:

 sealed partial class DeviceContext : .... { .... IntPtr hInitialFont; .... IntPtr hCurrentFont; // <= .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

Thus, an obvious mistake has been made. Another question is its criticality. Now, as a result of the DisposeFont method working in the section marked with the comment “select initial font back in”, the hCurrentFont field will not be assigned some initial value. I think that only code authors can give an exact verdict.



findings



So, this time I have to “scold” MS a bit.In WinForms there were a lot of errors that require close attention of developers. Perhaps this is due to some rush with which MS are working on .NET Core 3 and components, including WinForms. In my opinion, WinForms code is still "damp", but I hope that the situation will soon change for the better.



The second reason for a large number of errors may be that our analyzer just got better to look for them :).



By the way, an article by my colleague Sergey Vasiliev will be published soon in which he searches and finds quite a few problems in the code of .NET Core libraries. I hope that his work will also contribute to improving the performance of the .NET platform, because we always try to communicate the results of the analysis of their projects to developers.



Well, for those who want to improve their products on their own or conduct research to find errors in other people's projects, I suggest downloading and trying PVS-Studio.



All clean code!

If you want to share this article with an English-speaking audience, then please use the link to the translation: Sergey Khrenov. WinForms: Errors, Holmes