WinForms: erreurs, Holmes

Nous aimons rechercher des bogues dans les projets Microsoft. Pourquoi? C'est simple: leurs projets sont généralement faciles à vérifier (le travail peut être effectué immédiatement dans l'environnement Visual Studio, pour lequel PVS-Studio dispose d'un plug-in pratique) et ils contiennent peu d'erreurs. Par conséquent, l'algorithme de travail habituel est le suivant: rechercher et télécharger un projet ouvert à partir de MS; vérifiez-le; choisissez des erreurs intéressantes; assurez-vous qu'ils sont peu nombreux; écrire un article sans oublier de féliciter les développeurs. Super! Gagnant-gagnant-gagnant: cela a pris un peu de temps, le manuel est heureux de voir de nouveaux documents sur le blog, et le karma est en parfait état. Mais cette fois, quelque chose s'est mal passé. Voyons ce qui a été trouvé dans le code source de Windows Forms et si Microsoft devrait être loué cette fois.

Présentation

Début décembre 2018, Microsoft a annoncé la sortie de .NET Core 3 Preview 1. Un peu plus tôt (approximativement à partir de la mi-octobre), GitHub a commencé un travail actif sur la publication du code source de Windows Forms , la plate-forme d'interface utilisateur .NET Core pour créer des applications de bureau Windows. Vous pouvez voir les statistiques de validation ici . Maintenant, n'importe qui peut télécharger le code source de WinForms pour examen.

J'ai également téléchargé les sources pour y rechercher des erreurs à l'aide de PVS-Studio. La vérification n'a pas causé de difficultés. Requis: Visual Studio 2019, aperçu du SDK .NET Core 3.0, PVS-Studio. Et maintenant, le journal d'avertissement de l'analyseur est reçu.

Après avoir reçu le journal PVS-Studio, je le trie généralement par ordre croissant de numéros de diagnostic (la fenêtre avec le journal des messages PVS-Studio dans Visual Studio propose différentes options pour trier et filtrer la liste). Cela vous permet de travailler avec des groupes d'erreurs du même type, ce qui simplifie considérablement l'analyse du code source. Je marque des erreurs intéressantes dans la liste avec un astérisque, et seulement ensuite, après avoir analysé l'intégralité du journal, j'écris des fragments de code et fais une description. Et comme il y a généralement peu d'erreurs, je les mélange, en essayant de placer les plus intéressantes au début et à la fin de l'article. Mais cette fois, les erreurs se sont avérées un peu importantes (oh, il n'a pas été possible de sauver l'intrigue pendant longtemps), et je les apporterai par ordre de numéros de diagnostic.

Qu'a-t-on trouvé? 833 avertissements du niveau de confiance élevé et moyen (249 et 584, respectivement) ont été émis pour 540 000 lignes de code (à l'exclusion des lignes vides) dans des fichiers de 1670 cs. Et oui, selon la tradition, je n'ai pas vérifié les tests et n'ai pas pris en compte les avertissements du niveau de confiance faible (215 ont été émis). Selon mes observations précédentes, il y a trop d'avertissements pour le projet de la part de MS. Mais tous les avertissements ne sont pas des erreurs.

Pour ce projet, le nombre de faux positifs était d'environ 30%. Dans environ 20% des cas, je ne pouvais tout simplement pas tirer une conclusion exacte quant à savoir si c'était une erreur ou non, car je ne connaissais pas bien le code. Eh bien, au moins 20% des erreurs que j'ai manquées peuvent être attribuées au facteur humain: hâte, fatigue, etc. À propos, l'effet inverse est également possible: j'ai regardé à travers certains déclencheurs du même type, dont le nombre pourrait atteindre 70-80, à travers un qui pourrait occasionnellement augmenter le nombre d'erreurs que je considérais comme réelles.

Dans tous les cas, 30% des avertissements indiquent des erreurs réelles, ce qui est un pourcentage très élevé, étant donné que l'analyseur n'a pas été préconfiguré.

Donc, le nombre d'erreurs que j'ai pu détecter était d'environ 240, ce qui est dans les limites des statistiques. Je répète, à mon avis, pour un projet de MS, ce n'est pas le résultat le plus remarquable (bien que ce ne soit que 0,44 erreur pour 1000 lignes de code), et il y a probablement plus d'erreurs réelles dans le code WinForms. Je propose de parler des raisons à la fin de l'article, mais voyons maintenant les erreurs les plus intéressantes.

Erreurs

PVS-Studio: V3003 L'utilisation du modèle 'if (A) {...} else if (A) {...}' a été détectée. Il y a une probabilité de présence d'erreur logique. Vérifiez les lignes: 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 { .... } .... } 

Dans les blocs if et else if , la même condition est vérifiée. Cela ressemble à du copier-coller. Est-ce une erreur? Si vous regardez la déclaration de la méthode IsHighContrastHighlighted , il y a un doute:

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

La méthode peut probablement renvoyer différentes valeurs lors d'appels successifs. Et ce qui est fait dans la méthode d'appel semble étrange, bien sûr, mais il a droit à la vie. Cependant, je conseillerais aux auteurs de jeter un œil à ce morceau de code. Juste au cas où. Et pourtant, c'est un bon exemple de la difficulté de tirer des conclusions lors de l'analyse de code inconnu.

PVS-Studio: V3004 L' instruction 'then' est équivalente à l'instruction 'else'. 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; // <= } .... } .... } 

Et ici, l'erreur de copier-coller est définitivement commise. Quelle que soit la condition, la variable selCharOffset obtiendra toujours la même valeur.

Il y avait deux autres erreurs similaires dans le code WinForms:

• V3004 L'instruction 'then' est équivalente à l'instruction 'else'. SplitContainer.cs 1700
• V3004 L'instruction 'then' est équivalente à l'instruction 'else'. ToolstripProfessionalRenderer.cs 371

PVS-Studio: V3008 La variable se voit attribuer des valeurs deux fois de suite. C'est peut-être une erreur. Vérifiez les lignes: 681, 680. ProfessionalColorTable.cs 681

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

La méthode remplit le dictionnaire rgbTable . L'analyseur a pointé un morceau de code où deux valeurs différentes sont écrites séquentiellement sur la même clé. Et tout irait bien, mais il y avait 16 autres emplacements de ce type dans cette méthode, ce qui ne ressemble plus à une seule erreur. Mais pourquoi faire ça, pour moi ça reste un mystère. Je n'ai trouvé aucun signe de code généré automatiquement. Dans l'éditeur, cela ressemble à ceci:

Je donnerai les dix premières opérations avec une liste:

1. V3008 La variable se voit attribuer des valeurs deux fois de suite. C'est peut-être une erreur. Vérifiez les lignes: 785, 784. ProfessionalColorTable.cs 785
2. V3008 La variable se voit attribuer des valeurs deux fois de suite. C'est peut-être une erreur. Vérifiez les lignes: 787, 786. ProfessionalColorTable.cs 787
3. V3008 La variable se voit attribuer des valeurs deux fois de suite. C'est peut-être une erreur. Vérifiez les lignes: 789, 788. ProfessionalColorTable.cs 789
4. V3008 La variable se voit attribuer des valeurs deux fois de suite. C'est peut-être une erreur. Vérifiez les lignes: 791, 790. ProfessionalColorTable.cs 791
5. V3008 La variable se voit attribuer des valeurs deux fois de suite. C'est peut-être une erreur. Vérifiez les lignes: 797, 796. ProfessionalColorTable.cs 797
6. V3008 La variable se voit attribuer des valeurs deux fois de suite. C'est peut-être une erreur. Vérifiez les lignes: 799, 798. ProfessionalColorTable.cs 799
7. V3008 La variable se voit attribuer des valeurs deux fois de suite. C'est peut-être une erreur. Vérifiez les lignes: 807, 806. ProfessionalColorTable.cs 807
8. V3008 La variable se voit attribuer des valeurs deux fois de suite. C'est peut-être une erreur. Vérifiez les lignes: 815, 814. ProfessionalColorTable.cs 815
9. V3008 La variable se voit attribuer des valeurs deux fois de suite. C'est peut-être une erreur. Vérifiez les lignes: 817, 816. ProfessionalColorTable.cs 817
10. V3008 La variable se voit attribuer des valeurs deux fois de suite. C'est peut-être une erreur. Vérifiez les lignes: 823, 822. ProfessionalColorTable.cs 823

PVS-Studio: V3011 Deux conditions opposées ont été rencontrées. La deuxième condition est toujours fausse. Vérifiez les lignes: 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; } } 

L' instruction de retour ne sera jamais exécutée. Très probablement, la condition myGridTable! = Null dans le bloc if externe a été ajoutée ultérieurement lors du refactoring. Et maintenant, vérifier myGridTable == null est inutile. Pour améliorer la qualité du code, supprimez cette coche.

PVS-Studio: V3019 Il est possible qu'une variable incorrecte soit comparée à null après la conversion de type à l'aide du mot clé 'as'. Vérifiez les variables «gauche», «cscLeft». TypeCodeDomSerializer.cs 611

PVS-Studio: V3019 Il est possible qu'une variable incorrecte soit comparée à null après la conversion de type à l'aide du mot clé 'as'. Vérifiez les variables «droite», «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; // <= } 

L'analyseur a immédiatement émis deux avertissements pour la méthode Compare . Quel est le problème? C'est que les valeurs de cscLeft et cscRight ne sont pas vérifiées pour l'égalité nulle . Ils peuvent obtenir cette valeur après une conversion infructueuse vers le type OrderedCodeStatementCollection . Ensuite, une exception sera levée dans la dernière instruction de retour . Une telle situation est possible lorsque tous les contrôles de gauche et de droite passent et n'aboutissent pas à une sortie préalable de la méthode.

Pour corriger le code, cscLeft / cscRight doit être utilisé partout au lieu de gauche / droite .

PVS-Studio: V3020 Une «pause» inconditionnelle dans une boucle. 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; } } .... } 

Ce fragment fait plutôt référence au code "avec odeur". Il n'y a pas d'erreur ici. Mais des questions se posent quant à l'organisation du cycle foreach . Pourquoi est-il nécessaire ici - c'est clair: en raison de la nécessité d'extraire les éléments de la collection, passés sous ICollection . Mais pourquoi dans la boucle, initialement conçue pour une seule itération (une condition préalable est la présence d'un seul élément dans la collection de composants ), un filet de sécurité supplémentaire sous forme de rupture était nécessaire? Probablement, la réponse peut être envisagée: "Elle s'est développée historiquement". Le code a l'air moche.

PVS-Studio: V3022 L' expression 'ocxState! = Null' est toujours vraie. 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; } .... } } 

En raison d'une erreur logique, un «code mort» a été formé dans ce fragment. Les expressions du bloc else ne seront jamais exécutées.

PVS-Studio: V3027 La variable 'e' a été utilisée dans l'expression logique avant d'être vérifiée par rapport à null dans la même expression logique. ImageEditor.cs 99

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

La variable e dans la condition est d'abord utilisée, puis vérifiée pour l'inégalité nulle . Salut, NullReferenceException .

Une autre erreur similaire:

PVS-Studio: V3027 La variable 'dropDownItem' a été utilisée dans l'expression logique avant d'être vérifiée par rapport à null dans la même expression logique. 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) { .... } .... } 

Une situation similaire à la précédente, uniquement avec la variable dropDownItem . Je pense que de telles erreurs apparaissent à la suite d'une inattention lors du refactoring. Probablement une partie de la condition ! (DropDownItem.Owner est ToolStripDropDownMenu) a été ajoutée au code plus tard.

PVS-Studio: contrôle récurrent V3030 . La condition 'columnCount> 0' a déjà été vérifiée à la ligne 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); } .... } .... } 

Une erreur qui peut sembler inoffensive. En effet, eh bien, une vérification supplémentaire est effectuée, ce qui n'affecte pas la logique du travail. Et parfois, ils le font même lorsque vous devez revérifier l'état d'un composant visuel, par exemple, en obtenant le nombre d'entrées dans la liste. Seulement dans ce cas, ils revérifient la variable locale columnCount . C'est très suspect. Soit ils voulaient vérifier une autre variable, soit dans l'un des contrôles, ils utilisent la mauvaise condition.

PVS-Studio: V3061 Le paramètre 'lprcClipRect' est toujours réécrit dans le corps de la méthode avant d'être utilisé. 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; } 

Erreur évidente. Oui, le paramètre lprcClipRect est vraiment initialisé avec une nouvelle valeur, sans l'utiliser en aucune façon. Mais qu'est-ce que cela donne? Je pense que quelque part dans le code appelant, le lien passé par ce paramètre restera inchangé, bien qu'il ait été conçu différemment. En effet, regardez travailler avec d'autres variables dans cette méthode. Même son nom (le préfixe «Get») indique qu'une certaine initialisation sera effectuée à l'intérieur de la méthode via les paramètres passés. Et il en est ainsi. Les deux premiers paramètres ( ppFrame et ppDoc ) sont passés avec le modificateur out et obtiennent de nouvelles valeurs. Les liens lprcPosRect et lpFrameInfo permettent d'accéder aux champs de la classe et de les initialiser. Et seul lprcClipRect sort de la liste générale. Très probablement, le modificateur out ou ref est requis pour ce paramètre.

PVS-Studio: V3066 Ordre incorrect possible des arguments passés à la méthode 'AdjustCellBorderStyle': 'isFirstDisplayedRow' et 'isFirstDisplayedColumn'. DataGridViewComboBoxCell.cs 1934

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

L'analyseur soupçonnait que les deux derniers arguments étaient mélangés. Jetons un coup d'œil à la déclaration de la méthode AdjustCellBorderStyle :

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

On dirait une erreur. Oui, souvent certains arguments sont intentionnellement passés dans l'ordre inverse, par exemple, afin d'échanger certaines variables. Mais je ne pense pas que ce soit exactement le cas. Rien dans les méthodes appelantes ou appelées ne dit un tel modèle d'utilisation. Tout d'abord, les variables de type booléen sont confuses. Deuxièmement, les noms des méthodes sont également courants: pas de «Swap» ou «Reverse». De plus, il n'est pas si difficile de se tromper comme ça. Les gens perçoivent souvent différemment l'ordre de la paire ligne / colonne. Pour moi, par exemple, juste «ligne / colonne» est familier. Mais pour l'auteur de la méthode appelée AdjustCellBorderStyle , évidemment, l'ordre le plus familier est «colonne / ligne».

PVS-Studio: V3070 La variable non initialisée 'LANG_USER_DEFAULT' est utilisée lors de l'initialisation de la variable 'LOCALE_USER_DEFAULT'. 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); .... } 

Une erreur rare. L'ordre d'initialisation des champs de classe est mélangé. Pour calculer la valeur du champ LOCALE_USER_DEFAULT , utilisez le champ LANG_USER_DEFAULT , qui n'est pas encore initialisé et a une valeur de 0. Par ailleurs, la variable LANG_USER_DEFAULT n'est utilisée nulle part ailleurs dans le code. Je n'étais pas trop paresseux et j'ai écrit un petit programme de console qui simule la situation. Au lieu des valeurs de certaines constantes utilisées dans le code WinForms, j'ai substitué leurs valeurs réelles:

 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); } } 

À la suite du lancement, les éléments suivants seront affichés sur la console: 0. Maintenant, nous échangeons la déclaration des champs LOCALE_USER_DEFAULT et LANG_USER_DEFAULT . Le résultat du programme sous cette forme: 1024. Je pense qu'il n'y a rien de plus à commenter ici.

PVS-Studio: V3080 Déréférence nulle possible. Envisagez d'inspecter les «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); // <= .... } .... } 

Le code qui doit "tomber" est suffisamment stable, car vous pouvez entrer dans la branche else juste au moment où la référence ces est nulle .

Un autre exemple similaire:

PVS-Studio: V3080 Déréférence nulle possible. Pensez à inspecter 'comboBox'. ComboBox.cs 6610

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

Le code paradoxal. Apparemment, ils ont mélangé le test en écrivant if (comboBox! = Null) au lieu de if (comboBox == null) . Et donc - nous recevrons la prochaine NullReferenceException .

Nous avons examiné deux erreurs assez évidentes V3080 , où vous pouvez suivre visuellement la situation de l'utilisation possible de références nulles dans la méthode. Mais les diagnostics du V3080 sont beaucoup plus intelligents et peuvent rechercher des erreurs similaires pour les chaînes d'appels de méthode. Il n'y a pas si longtemps, nous avons considérablement renforcé les mécanismes de flux de données et d'analyse interprocédurale. Vous pouvez en savoir plus à ce sujet dans l'article " Types de référence nullables en C # 8.0 et analyse statique ". Et voici une erreur similaire trouvée dans WinForms:

PVS-Studio: V3080 Déréférence nulle possible à l'intérieur de la méthode sur 'reader.NameTable'. Pensez à inspecter le 1er 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 { .... } .... } 

Regardez ce qui se passe avec la variable contentReader dans le corps de la méthode. Après l'initialisation avec une référence nulle, suite à l'une des vérifications, le lien sera initialisé. Cependant, une série de vérifications ne se termine pas par un bloc else . Cela signifie que dans certains cas rares (ou en raison d'une refactorisation à l'avenir), le lien peut toujours rester nul. Ensuite, il sera transmis à la méthode SetupNameTable , où il est utilisé sans aucune vérification:

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

Il s'agit d'un code potentiellement dangereux.

Et encore une erreur, où l'analyseur a dû passer par une chaîne d'appels pour identifier un problème:

PVS-Studio: V3080 Déréférence nulle possible. Pensez à inspecter la «disposition». DockAndAnchorLayout.cs 156

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

L'analyseur prétend qu'il est possible d'obtenir une référence nulle à partir de la méthode GetAnchorInfo , qui lèvera une exception lors du calcul de la valeur gauche . Passons en revue toute la chaîne des appels et vérifions si c'est le cas:

 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; } } 

En effet, dans certains cas, la méthode GetObject qui ferme la chaîne d'appel retournera null , qui sans aucun contrôle supplémentaire sera passé à la méthode appelante. Probablement, la méthode GetAnchorDestination devrait prévoir une telle situation.

Dans le code WinForms, il y avait pas mal d'erreurs de ce type, plus de 70 . Ils sont tous similaires et je ne donnerai pas leur description dans l'article.

PVS-Studio: V3091 Analyse empirique. Il est possible qu'une faute de frappe soit présente à l'intérieur du littéral de chaîne: "ShowCheckMargin". Le mot «ShowCheckMargin» est suspect. PropertyNames.cs 136

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

Un bon exemple d'une erreur qui n'est pas si facile à détecter. Lors de l'initialisation des champs de classe, ils utilisent la même valeur, bien que l'auteur du code ne le pense évidemment pas (le copier-coller est à blâmer). L'analyseur a tiré cette conclusion en comparant les noms des variables et les valeurs des chaînes attribuées. Je n'ai donné que les lignes d'erreur, mais regardez à quoi cela ressemble dans l'éditeur de code:

C'est la détection de telles erreurs qui démontre la puissance et le soin infini des outils d'analyse statique.

PVS-Studio: V3095 L'objet 'currentForm' a été utilisé avant d'être vérifié par rapport à null. Vérifiez les lignes: 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 && ....) .... } 

Classique La variable currentForm est utilisée sans aucun contrôle. Mais plus loin dans le code, il y a sa vérification de l' égalité nulle . Dans ce cas, je peux vous conseiller d'être plus prudent lorsque vous travaillez avec des types de référence, ainsi que d'utiliser des analyseurs statiques :).

Une autre erreur similaire:

PVS-Studio: V3095 L'objet 'backgroundBrush' a été utilisé avant d'être vérifié par rapport à null. Vérifiez les lignes: 2331, 2334. DataGrid.cs 2331

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

Dans le code WinForms, j'ai rencontré plus de 60 erreurs de ce type. À mon avis, tous sont assez critiques et nécessitent l'attention des développeurs. Mais dans l'article, il n'est pas si intéressant d'en parler, je me limiterai donc aux deux mentionnés ci-dessus.

PVS-Studio: V3125 L'objet '_propInfo' a été utilisé et a été vérifié par rapport à null dans différentes branches d'exécution. Vérifiez les lignes: 996, 982. Binding.cs 996

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

Pour compléter l'image - également une sorte de classique, bug V3125 . La situation inverse. Au début, une référence potentiellement nulle est utilisée en toute sécurité, vérifiant la valeur null , mais ensuite, ils ne le font plus dans le code.

Et une autre erreur similaire:

PVS-Studio: V3125 L'objet 'propriétaire' a été utilisé après avoir été vérifié par rapport à null. Vérifiez les lignes: 64, 60. FlatButtonAppearance.cs 64

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

La beauté Mais c'est du point de vue d'un chercheur extérieur. En effet, en plus de ces deux V3125 , l'analyseur a trouvé plus de 50 modèles similaires dans le code WinForms. Les développeurs ont du travail à faire.

Et enfin - une erreur assez intéressante, à mon avis.

PVS-Studio: V3137 La variable 'hCurrentFont' est affectée mais n'est pas utilisée à la fin de la fonction. 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; } } .... } 

Voyons ce que l'analyseur a alerté et pourquoi le fait qu'une variable se voit attribuer une valeur, mais qu'elle ne sera pas utilisée dans la méthode à l'avenir, peut indiquer un problème.

Une classe partielle est déclarée dans le fichier DeviceContext2.cs . La méthode DisposeFont est utilisée pour libérer des ressources après avoir travaillé avec des graphiques: le contexte de l'appareil et les polices. Pour une meilleure compréhension, j'ai fourni l'intégralité de la méthode DisposeFont . Faites attention à la variable locale hCurrentFont . Le problème est que la déclaration de cette variable dans une méthode masque le champ de classe du même nom. J'ai trouvé deux méthodes de la classe DeviceContext où un champ appelé hCurrentFont est utilisé :

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

Jetez un œil à la méthode ResetFont . La dernière ligne indique exactement ce que fait la méthode DisposeFont dans le bloc if imbriqué (l'analyseur pointe vers cet endroit). Et ce champ hCurrentFont du même nom a été déclaré dans une autre partie de la classe partielle dans le fichier DeviceContext.cs :

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

Ainsi, une erreur évidente a été commise. Une autre question est sa criticité. Maintenant, à la suite de la méthode DisposeFont travaillant dans la section marquée avec le commentaire «sélectionner la police initiale de retour», le champ hCurrentFont ne se verra pas attribuer une valeur initiale. Je pense que seuls les auteurs de code peuvent donner un verdict exact.

Conclusions

Donc, cette fois, je dois «gronder» un peu la SP. Dans WinForms, il y avait beaucoup d'erreurs qui nécessitent une attention particulière de la part des développeurs. Cela est peut-être dû à une certaine précipitation avec laquelle MS travaille sur .NET Core 3 et ses composants, y compris WinForms. À mon avis, le code WinForms est toujours "humide", mais j'espère que la situation va bientôt changer pour le mieux.

La deuxième raison d'un grand nombre d'erreurs est peut-être que notre analyseur s'est encore amélioré pour les rechercher :).

Soit dit en passant, un article de mon collègue Sergey Vasiliev sera bientôt publié dans lequel il recherche et trouve pas mal de problèmes dans le code des bibliothèques .NET Core. J'espère que son travail contribuera également à améliorer les performances de la plateforme .NET, car nous essayons toujours de communiquer les résultats de l'analyse de leurs projets aux développeurs.

Eh bien, pour ceux qui veulent améliorer leurs produits par eux-mêmes ou mener des recherches pour trouver des erreurs dans les projets d'autres personnes, je suggère de télécharger et d'essayer PVS-Studio.

Tout code propre!

Si vous souhaitez partager cet article avec un public anglophone, veuillez utiliser le lien vers la traduction: Sergey Khrenov. WinForms: erreurs, Holmes