Стряхнём пыль с глобуса: проверяем проект NASA World Wind

Иногда полезно оглянуться и посмотреть, как мог помочь анализатор в старых проектах, и каких ошибок можно своевременно избежать, если использовать анализатор регулярно. В этот раз выбор пал на проект NASA World Wind, который до 2007 года разрабатывался на языке C#.

NASA World Wind — это интерактивный глобус, позволяющий увидеть любое место на Земле. Для работы проект использует базу публичных снимков со спутника Landsat и проект моделирования рельефа Shuttle Radar Topography Mission. Первые версии проекта создавались на языке С#. Позже проект продолжил своё развитие на языке Java. Последняя выпущенная на C# версия — 1.4. Хотя C# версия уже много лет как заброшена, это не помешает нам проверить проект и оценить качество кода, разработчиком которого является NASA Ames Research Center.

Зачем мы проверили старый проект? Нам давно предлагали проверить что-то из проектов NASA и вот мы случайно набрели на этот проект. Да, эта проверка не принесёт никакой пользы проекту. Но такой цели в этот раз мы и не ставили. Мы просто хотели в очередной раз продемонстрировать пользу, которую может приносить статический анализатор кода PVS-Studio при разработке, в том числе и компании NASA.

Кстати, это уже не первый случай проверки «исторических» проектов. Возможно, Вам также будет интересно познакомиться со следующими статьями:

• Занимательная археология. Или PVS-Studio проверяет Microsoft Word 1.1a
• К тридцатилетию первого C++ компилятора: ищем ошибки в Cfront

Проект NASA World Wind хорошо показывает возможности анализатора PVS-Studio. Как станет видно из статьи, при написании кода, похоже, активно использовались механизмы Copy-Paste. Многие ошибки расплодились и часто дублируются в коде. Некоторые ошибки проекта являются показательными и хорошо иллюстрируют принципы работы диагностик анализатора.

Для анализа проекта использовался анализатор PVS-Studio версии 6.06.

Плотность ошибок

После проверки проекта анализатор выдал 120 предупреждений первого уровня и 158 предупреждение второго уровня. После изучения сообщений я считаю, что код стоит исправить в 122 местах. Таким образом, процент ложных срабатываний составил 56%. Это значит, что каждое второе сообщение указывает на ошибку или явно плохой код, нуждающийся в исправлении.

Теперь рассчитаем плотность ошибок. Всего в проекте, с учётом комментариев, 474240 строк кода в 943 файлах. Среди них 122 проблемных мест. Получаем, что анализатор в среднем находит 0,25 ошибки на 1000 строк кода. Это говорит о высоком качестве кода.

Эффект последней строки

public Point3d (Point3d P) 
{
   X = P.X;
   Y = P.Y;
   X = P.Z;  // <=
}

Предупреждение:

• V3008 The 'X' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 40, 38. Point3d.cs 40

Здесь мы видим классическую ошибку в конструкторе копирования. При присваивании трёхмерных координат объекта вместо задания переменой Z было дважды перезаписано значение переменной X. Вполне очевидно, что эта ошибка была допущена в результате использования «Copy-Paste методики». Шанс допустить ошибку в последней строке при копировании кода гораздо выше. Подробней об этом феномене можно почитать в статье Андрея Карпова "Эффект последней строки". Для исправления этого конструктора требуется заменить переменную в последней строке.

public Point3d (Point3d P)
{
   X = P.X;
   Y = P.Y;
   Z = P.Z;  
}

Это не единственная ошибка в проекте, которая подтверждает этот эффект. В продолжении статьи встретится ещё несколько примеров, доказывающих его.

Ещё несколько подозрительных мест, где сработала диагностика V3008:

• V3008 The 'this._imagePath' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 270, 263. ImageLayer.cs 270
• V3008 The 'm_PolygonFill' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 1623, 1618. ShapeFileLayer.cs 1623

Логическая ошибка или коварная опечатка?

private static void WebUpdate(....)
{
  ....
  if (ver != version)          // <=
   {
      ....
   }
   else if (ver != version)    // <=
   {
      ....
   }
}

Предупреждение:

• V3003 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 2111, 2197. KMLImporter.cs 2111

Фрагмент занимается автоматическим обновлением плагина в проекте. При несоответствии версии он скачивает нужный плагин. Если это внутренний плагин, программа отказывается обновлять его автоматически и просто выдаёт сообщение о новой версии. Но условный оператор, расположенный после else содержит выражение, которое противоречило условию входа в оператор else. Код достаточно противоречивый, и точно определить в чём ошибка могут только разработчики, знающие как именно должна работать функция. На данный момент я могу лишь предположить, что else должен был относиться совсем не к этому условному оператору. Или же, судя по комментарию рядом с условным оператором, можно сделать вывод, что оператор else расположен на своём месте, а у второго оператора if должно быть иное условие.

Ошибка копирования

public GpsSetup(....)
{
 ....
  if (m_gpsIcon!=null)
  {
   ....
   labelTitle.Text = "Set options for " +
                     m_gpsIcon.m_RenderInfo.sDescription;
  }
  else
  if (m_gpsTrackLine != null)
  {
   ....
   labelTitle.Text = "Set options for " + 
                     m_gpsIcon.m_RenderInfo.sDescription; // <=
  }
 ....
}

Предупреждение:

• V3080 Possible null dereference. Consider inspecting 'm_gpsIcon'. GpsTrackerPlugin.SourceSetup.cs 68

В приведённом фрагменте ошибка, связанная с неаккуратным копированием кода. В последнем операторе условия перепутана переменная. В итоге произошла ошибка с доступом по нулевой ссылке. Согласно предыдущим проверкам, переменная m_gpsIcon, использованная в последней строке, гарантировано равна null. Если сработает условие m_gpsTrackLine!=null, программа завершится с ошибкой. Могу предположить, что корректный код должен выглядеть следующим образом:

if (m_gpsTrackLine != null)
{
  ....
  labelTitle.Text = "Set options for " + 
                     m_gpsTrackLine.m_sDescription;
}

Невыполняемое условие

public bool LoadSettings(....)
{
 ....
 if (bSet)
 {
  while(true)
   {
     line = sr.ReadLine();
     if (line==null || line.StartsWith("END UI CONTROLS"))
        break;
     ....
     if (line.StartsWith("comboBoxAPRSInternetServer=")) // <=
        ....
     else
     ....
     if (line.StartsWith("checkBoxNoDelay="))            // <= 
        ....
     else
     if (line.StartsWith("checkBoxNoDelay="))            // <=
        ....
     ....
     else
     if (line.StartsWith("comboBoxAPRSInternetServer=")) // <=
     ....
   }
  ....
 }
 ....
}

Предупреждения:

• V3003 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 4503, 4607. GPSTracker.cs 4503
• V3003 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 4527, 4530. GPSTracker.cs 4527

Ещё один пример ошибки, возникающий из-за больших фрагментов кода, созданных с помощью копирования кода. Приведённая функция предназначена для обработки входящих команд и представляет из себя очень длинный блок кода, состоящий из однотипных условных операторов. Код никогда не дойдёт до повторной проверки. В такой ошибке нет ничего страшного, но вполне может быть, что вместо дубликата должна была располагаться какая-то другая команда. Раз нужная команда не будет обработана, значит код ведёт себя совсем не так, как предполагает программист.

Анализатор указал сразу на два места в этом огромном дереве условных операторов. В первом месте двойная проверка line.StartsWith(«checkBoxNoDelay=») расположена рядом, и её можно было бы заметить при внимательном изучении кода, хотя, учитывая количество кода — это достаточно сложный процесс, который занял бы много времени.

Второе место скрывается от глаз разработчиков гораздо лучше. Первая проверка line.StartsWith(«comboBoxAPRSInternetServer=») расположена в середине дерева, а вторая проверка фактически завершает его, и между ними находится порядка 100 строк кода. Этот случай хорошо демонстрирует, что иногда статический анализ кода может быть даже эффективней, чем обзор кода. Регулярное использование анализатора позволяет выявлять подобные ошибки на ранних стадиях и избавляет программистов от мучительной отладки и чтения длинных функций.

Ошибка в имени переменной

public int CompareTo(object obj)
{
  RenderableObject robj = obj as RenderableObject;
  if(obj == null)   // <=
    return 1;
  return this.m_renderPriority.CompareTo(robj.RenderPriority);
}

Предупреждение:

• V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'obj', 'robj'. RenderableObject.cs 199

Опечатка в имени переменной привела к потенциальному использованию нулевой ссылки. Вместо проверки объекта производного класса robj проверяется базовый объект obj. В случае, если он не соответствует типу RenderableObject, программа завершится с ошибкой. Для исправления требуется изменить имя переменой в выражении условного оператора на robj.

Доступ по нулевой ссылке

public override void Render(DrawArgs drawArgs)
{
  ....
  if(this.linePoints.Length > 1)     // <=  
   {
     ....
     if(this.linePoints != null)     // <=
      {
        ....
      }
   }
  ....
}

Предупреждения:

• V3022 Expression 'this.linePoints != null' is always true. PathLine.cs 346
• V3095 The 'this.linePoints' object was used before it was verified against null. Check lines: 332, 346. PathLine.cs 332

На этот код указывают сразу две различных диагностики. В коде нарушен порядок действий по проверке и доступе к ссылке. Сначала вычисляют количество элементов внутри объекта, а уже потом проверяют существует ли объект вообще. Возможно, объект this.linePoints может никогда не получить значение null, но тогда проверка во внутреннем условии также не нужна. Логично изменить код следующим образом:

if(this.linePoints != null && this.linePoints.Length > 1)  
{
  ....
}

Всегда ложное условие

private bool checkSurfaceImageChange()
{
  ....
  SurfaceImage currentSurfaceImage = 
  m_ParentWorldSurfaceRenderer.SurfaceImages[i] as SurfaceImage;
                    
  if(currentSurfaceImage.LastUpdate > m_LastUpdate || 
      currentSurfaceImage.Opacity != 
      currentSurfaceImage.ParentRenderable.Opacity)
     {
      if(currentSurfaceImage == null ||               // <=
         currentSurfaceImage.ImageTexture == null || 
         currentSurfaceImage.ImageTexture.Disposed || 
         !currentSurfaceImage.Enabled || ....)
         {
           continue;
         }
         else
         {
           return true;
         }
      }
  ....
}

Предупреждение:

• V3063 A part of conditional expression is always false: currentSurfaceImage == null. SurfaceTile.cs 1069

Эта ошибка похожа на описанную в предыдущем разделе. Здесь ссылка на объект присваивается переменной непосредственно перед условными операторами. Скорее всего, ссылка на объект не может быть нулевой, а проверка во внутреннем условии не требуется. Если это не так, проверку надо перенести во внешний условный оператор. В нём уже осуществляется работа со свойствами объекта currentSurfaceImage. Если ссылка будет нулевой, ошибка возникнет раньше, чем будет выполнена проверка.

Похожие места:

• V3063 A part of conditional expression is always true: iWildIndex==-1. GPSTrackerPlugin.APRS.cs 87
• V3063 A part of conditional expression is always true: iWildIndex==-1. GPSTrackerPlugin.NMEA.cs 169
• V3063 A part of conditional expression is always false: newvalue == null. SchemaTypes.cs 860

Блуждающая скобка

public void threadStartFile()
{
 ....
  if (File.Exists(sFileName))
   {
    ....
    if (gpsSource.bTrackAtOnce!=true && ....)
    {
      if (!gpsSource.bWaypoints)
      {
         m_GpsTracker.m_gpsTrackerPlugin.pluginShowFixInfo("");
         ....                    
                            // <=
      gpsSource.sFileNameSession=sFileName;
      ....
      }
      else
      {
      ....
        }
      }
      else
      {
       ....                             
      }
   }                        // <=
 ....
}

Предупреждение:

• V3033 It is possible that this 'else' branch must apply to the previous 'if' statement. GPSTrackerPlugin.File.cs 314

Анализатор обнаружил несоответствие форматирования и логики работы кода. При близком рассмотрении оказалось, что во внутреннем if была забыта закрывающая скобка. Недостающая скобка обнаружилась после завершения блока второго оператора else. В итоге проект смог скомпилироваться, несмотря на неаккуратный код. Компилятор может сообщать только, если else перестанет принадлежать условному оператору. В данном случае просто произошёл сдвиг else на другой оператор if. Так как закрывающая скобка находилась в неправильном месте, была нарушена работа двух условных операторов, а также форматирование операторов else. Могу предположить, как должен выглядеть код после исправления положения скобки и ошибок форматирования:

public void threadStartFile()
{
 ....
 if (File.Exists(sFileName))
  {
   ....
   if (gpsSource.bTrackAtOnce!=true && ....)
   {
     if (!gpsSource.bWaypoints)
     {
        m_GpsTracker.m_gpsTrackerPlugin.pluginShowFixInfo("");
        ....                    
     }
     gpsSource.sFileNameSession=sFileName;
     ....
   }
   else
   {
     ....
   }
 }
 else
 {
   ....                             
 }
 ....
}

Неиспользованный параметр

public bool Diagonal(CPoint2D vertex1, CPoint2D vertex2)
{
 ....
 for (int i= 0; i<nNumOfVertices; i++) 
  {
    ....
    //Diagonal line:
    double x1=vertex1.X;
    double y1=vertex1.Y;
    double x2=vertex1.X;
    double y2=vertex1.Y;
    ....
  }
 ....
}

Предупреждение:

• V3065 Parameter 'vertex2' is not utilized inside method's body. CPolygon.cs 227

В функцию поступают координаты двух точек линии, но в результате опечатки значения обеих точек берутся только из переменной vertex1. Для исправления ошибки требуется изменить код следующим образом:

double x2=vertex2.X;
double y2=vertex2.Y;

Помимо опечатки во время присваивания, замечена ещё одна странность. Зачем присваивать фиксированное значение в каждой итерации цикла? Внутри цикла значение не меняется и гораздо логичнее сделать это присваивание один раз до его начала.

Перезапись параметра функции

void ShowInfo(.... , float fDistance )
{
  ....
  if (m_fTotalDistance>=0F)
   {
     string sUnit=(m_fTotalDistance>=1F)?"km":"m";
     fDistance = (m_fTotalDistance < 1F) ? 
                 (m_fTotalDistance * 1000F) : 
                  m_fTotalDistance;
     sInfo += "Track Distance: " +
              Convert.ToString(
               decimal.Round(
                Convert.ToDecimal(fDistance),3)) +
              sUnit + 
              "n";
   }
  ....
}

Предупреждение:

• V3061 Parameter 'fDistance' is always rewritten in method body before being used. GPSTrackerPlugin.WorldWind.cs 1667

Поступающий в функцию параметр fDistance перезаписывается непосредственно перед использованием. Настоящее значение переменной теряется, а вместо него используется значение свойства класса m_fTotalDistance. Больше нигде в функции переменная не встречается, поэтому изменять значение fDistance не имеет смысла. Судя по другим фрагментам функции можно предположить, что тут перепутаны местами переменные, и на самом деле этот фрагмент должен выглядеть так:

if (fDistance>=0F)
{
 string sUnit=(fDistance>=1F)?"km":"m";
 m_fTotalDistance = (fDistance < 1F) ? 
                    (fDistance * 1000F) : fDistance;
 sInfo += "Track Distance: " + 
          Convert.ToString(
           decimal.Round(
            Convert.ToDecimal(m_fTotalDistance),3)) +
          sUnit + 
          "n";
}

Неверная проверка NaN

public override bool PerformSelectionAction(DrawArgs drawArgs)
{
  ....
  if(icon.OnClickZoomAltitude != double.NaN || 
     icon.OnClickZoomHeading != double.NaN || 
     icon.OnClickZoomTilt != double.NaN)
     {
       ....
     }
  ....
}

Предупреждение:

• V3076 Comparison of 'icon.OnClickZoomAltitude' with 'double.NaN' is meaningless. Use 'double.IsNaN()' method instead. Icon.cs 389

Согласно документации нельзя сравнивать два значения double.NaN с помощью оператора !=. Результат такого сравнения всегда будет равен true. Для корректной проверки следует использовать метод double.IsNaN(). Соответственно, код примет следующий вид:

if(!double.IsNaN(icon.OnClickZoomAltitude) || 
   !double.IsNaN(icon.OnClickZoomHeading) || 
   !double.IsNaN(icon.OnClickZoomTilt)) ....

Анализатор обнаружил много мест с использованием таких некорректных проверок:

• V3076 Comparison of 'icon.OnClickZoomHeading' with 'double.NaN' is meaningless. Use 'double.IsNaN()' method instead. Icon.cs 389
• V3076 Comparison of 'icon.OnClickZoomTilt' with 'double.NaN' is meaningless. Use 'double.IsNaN()' method instead. Icon.cs 389
• V3076 Comparison of 'm_ShapeTileArgs.ScaleMin' with 'double.NaN' is meaningless. Use 'double.IsNaN()' method instead. ShapeFileLayer.cs 642
• V3076 Comparison of 'm_ShapeTileArgs.ScaleMax' with 'double.NaN' is meaningless. Use 'double.IsNaN()' method instead. ShapeFileLayer.cs 642
• V3076 Comparison of 'm_ShapeTileArgs.ScaleMin' with 'double.NaN' is meaningless. Use 'double.IsNaN()' method instead. ShapeFileLayer.cs 645
• V3076 Comparison of 'm_ShapeTileArgs.ScaleMax' with 'double.NaN' is meaningless. Use 'double.IsNaN()' method instead. ShapeFileLayer.cs 650
• V3076 Comparison of 'm_ShapeTileArgs.ScaleMin' with 'double.NaN' is meaningless. Use 'double.IsNaN()' method instead. ShapeFileLayer.cs 677
• V3076 Comparison of 'm_ShapeTileArgs.ScaleMax' with 'double.NaN' is meaningless. Use 'double.IsNaN()' method instead. ShapeFileLayer.cs 681
• V3076 Comparison of 'm_ScalarFilterMin' with 'double.NaN' is meaningless. Use 'double.IsNaN()' method instead. ShapeFileLayer.cs 886
• V3076 Comparison of 'm_ScalarFilterMax' with 'double.NaN' is meaningless. Use 'double.IsNaN()' method instead. ShapeFileLayer.cs 894
• V3076 Comparison of 'm_ShapeTileArgs.ScaleMin' with 'double.NaN' is meaningless. Use 'double.IsNaN()' method instead. ShapeFileLayer.cs 1038
• V3076 Comparison of 'm_ShapeTileArgs.ScaleMax' with 'double.NaN' is meaningless. Use 'double.IsNaN()' method instead. ShapeFileLayer.cs 1038
• V3076 Comparison of 'm_ShapeTileArgs.ScaleMin' with 'double.NaN' is meaningless. Use 'double.IsNaN()' method instead. ShapeFileLayer.cs 1041
• V3076 Comparison of 'm_ShapeTileArgs.ScaleMax' with 'double.NaN' is meaningless. Use 'double.IsNaN()' method instead. ShapeFileLayer.cs 1046
• V3076 Comparison of 'm_ShapeTileArgs.ScaleMin' with 'double.NaN' is meaningless. Use 'double.IsNaN()' method instead. ShapeFileLayer.cs 1073
• V3076 Comparison of 'm_ShapeTileArgs.ScaleMax' with 'double.NaN' is meaningless. Use 'double.IsNaN()' method instead. ShapeFileLayer.cs 1077
• V3076 Comparison of 'm_ScalarFilterMin' with 'double.NaN' is meaningless. Use 'double.IsNaN()' method instead. ShapeFileLayer.cs 1259
• V3076 Comparison of 'm_ScalarFilterMax' with 'double.NaN' is meaningless. Use 'double.IsNaN()' method instead. ShapeFileLayer.cs 1267

Игнорирование результата функции

private static void addExtendedInformation(....)
{
 ....
 if(toolBarImage.Length > 0 && 
    !Path.IsPathRooted(toolBarImage))
      Path.Combine(...., toolBarImage);    // <=
 ....
}

Предупреждение:

• V3010 The return value of function 'Combine' is required to be utilized. ConfigurationLoader.cs 943

Вызов функции Path.Combine без обработки результата не имеет смысла. В данном случае функция служит для формирования пути к объекту на основании абсолютного пути до исполняемого файла и относительного пути до изображения. Отсутствие обработки значения указывает на явную ошибку. Функция Path.Combine используется во многих местах программы. На основе этого можно сделать вывод, что код надо исправить следующим образом:

toolBarImage=Path.Combine(...., toolBarImage);

Ошибка была скопирована и в другие места проекта:

• V3010 The return value of function 'Combine' is required to be utilized. ConfigurationLoader.cs 1361
• V3010 The return value of function 'Combine' is required to be utilized. ConfigurationLoader.cs 1566
• V3010 The return value of function 'Combine' is required to be utilized. ConfigurationLoader.cs 1687
• V3010 The return value of function 'Replace' is required to be utilized. WorldWind.cs 2455

Пропущенное значение Enum

public enum MenuAnchor
{
  Top,
  Bottom,
  Left,
  Right
}

public bool OnMouseMove(MouseEventArgs e)
{
  ....
  if(this._visibleState == VisibleState.Visible)
   {
     ....
     switch(m_anchor)
     {
       case MenuAnchor.Top: ....
       case MenuAnchor.Bottom: ....    
       case MenuAnchor.Right: ....
     }
   }
  ....
}

Предупреждение:

• V3002 The switch statement does not cover all values of the 'MenuAnchor' enum: Left. Menu.cs 1681

Код, видимо, проверяет, находится ли курсор над ToolBar в данный момент, или нет. Из кода видно, что отсутствует обработка элемента MenuAnchor.Left. Точно знать, что подразумевали разработчики при написании данного фрагмента невозможно. Возможно, его обработка не требовалась, но я считаю — надо обратить внимание на этот фрагмент.

Бессмысленное присваивание

protected virtual void CreateElevatedMesh()
{
  ....
  if (minimumElevation > maximumElevation)
  {
    // Compensate for negative vertical exaggeration
    minimumElevation = maximumElevation;
    maximumElevation = minimumElevation;
  }
  ....
}

Предупреждение:

• V3037 An odd sequence of assignments of this kind: A = B; B = A;. Check lines: 625, 624. QuadTile.cs 625

Здесь просто избыточный код. Наличие строки maximumElevation = minimumElevation не имеет смысла, так как на момент присваивания обе переменные имеют одинаковые значения в результате выполнения предыдущей операции. Можно, конечно, предположить, что разработчики хотели поменять значения переменных, но сделали это некорректно. Это сомнительное предположение, так как и комментарий, и то, что переменная maximumElevation больше не используется, доказывают обратное.

Многократное присваивание

public static bool SearchForAddress(....)
{
  double num1;
  long2 = num1 = 0;
  long1 = num1 = num1;  // <=
  lat2 = num1 = num1;   // <=
  lat1 = num1;
  ....
}

Предупреждения:

• V3005 The 'num1' variable is assigned to itself. PlaceFinder.cs 2011
• V3005 The 'num1' variable is assigned to itself. PlaceFinder.cs 2012

Опять же эффект Copy-Paste, который часто встречается в проекте. Опасности этот фрагмент не несёт, но присваивание значения трижды одной и той же переменой смотрится странно. На мой взгляд, было бы нагляднее инициализировать переменную num1 непосредственно при её объявлении, а уж после — присваивать значение num1 раздельно каждой переменной.

Небезопасный вызов обработчика события

private static void m_timer_Elapsed(....)
{
  ....
  if (Elapsed != null)
     Elapsed(sender, e);
}

Предупреждение:

• V3083 Unsafe invocation of event 'Elapsed', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. TimeKeeper.cs 78

В многопоточном приложении такой вызов события не является безопасным. Вполне может произойти ситуация, когда между проверкой на null и непосредственно вызовом обработчика произойдёт отписка от события в другом потоке. Если это был единственный обработчик, то это приведёт к использованию нулевой ссылки. Для безопасного вызова события рекомендуется использовать временную переменную, тогда событие будет вызвано корректно в любом случае. О других способах исправления этой ошибки можно почитать на странице диагностики V3083.

Кстати, это очень коварный вид ошибки, так как проблемы будут возникать крайне редко, а воспроизвести их повторно будет вообще почти невозможно!

Прочие небезопасные вызовы приведу списком:

• V3083 Unsafe invocation of event 'Navigate', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. InternalWebBrowser.cs 65
• V3083 Unsafe invocation of event 'Close', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. InternalWebBrowser.cs 73
• V3083 Unsafe invocation of event 'OnMouseEnterEvent', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. WavingFlagLayer.cs 672
• V3083 Unsafe invocation of event 'OnMouseLeaveEvent', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. WavingFlagLayer.cs 691
• V3083 Unsafe invocation of event 'OnMouseUpEvent', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. WavingFlagLayer.cs 1105
• V3083 Unsafe invocation of event, NullReferenceException is possible. Consider assigning event to a local variable before invoking it. TreeNodeWidget.cs 325
• V3083 Unsafe invocation of event 'OnVisibleChanged', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. FormWidget.cs 721
• V3083 Unsafe invocation of event 'OnResizeEvent', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. FormWidget.cs 1656
• V3083 Unsafe invocation of event 'OnMouseEnterEvent', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. PictureBox.cs 351
• V3083 Unsafe invocation of event 'OnMouseLeaveEvent', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. PictureBox.cs 362
• V3083 Unsafe invocation of event 'OnMouseUpEvent', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. PictureBox.cs 590
• V3083 Unsafe invocation of event 'OnMouseEnterEvent', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. WorldWind.Widgets.PictureBox.cs 282
• V3083 Unsafe invocation of event 'OnMouseLeaveEvent', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. WorldWind.Widgets.PictureBox.cs 293
• V3083 Unsafe invocation of event 'OnMouseUpEvent', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. WorldWind.Widgets.PictureBox.cs 511
• V3083 Unsafe invocation of event 'OnVisibleChanged', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. WorldWindow.Widgets.Form.cs 184
• V3083 Unsafe invocation of event 'StatusChanged', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. ScriptPlayer.cs 57
• V3083 Unsafe invocation of event 'Navigate', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. HtmlEditor.cs 608
• V3083 Unsafe invocation of event 'ReadyStateChanged', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. HtmlEditor.cs 578
• V3083 Unsafe invocation of event 'UpdateUI', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. HtmlEditor.cs 568
• V3083 Unsafe invocation of event 'HtmlKeyPress', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. HtmlEditor.cs 587
• V3083 Unsafe invocation of event 'HtmlEvent', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. HtmlEditor.cs 600
• V3083 Unsafe invocation of event 'BeforeNavigate', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. HtmlEditor.cs 635
• V3083 Unsafe invocation of event 'BeforeShortcut', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. HtmlEditor.cs 626
• V3083 Unsafe invocation of event 'BeforePaste', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. HtmlEditor.cs 644
• V3083 Unsafe invocation of event 'ContentChanged', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. HtmlEditor.cs 615

Небезопасный объект для блокировки

private int APRSParse(....)
{
  int iRet=-1;
  try
  {
   lock("ExternDllAccess")
     {
       //Call the APRS DLL
       iRet=APRSParseLinePosStat(sString, 
                                 ref aprsPosition, 
                                 ref aprsStatus);
     }
  }
  catch(Exception)
  {
    iRet=-1;
  }
 return iRet;
}

Предупреждение:

• V3090 Unsafe locking on an object of type 'String'. GPSTrackerPlugin.APRS.cs 256

Использование в качестве объекта для блокировки текстовой строки не является безопасным. Доступ к таким объектам можно получить из любого места программы. В результате может произойти взаимная блокировка, так как в обоих случаях при анализе строки будет получена ссылка на один и тот же объект в памяти.

Ещё несколько мест обнаруженных анализатором:

• V3090 Unsafe locking on an object of type 'String'. GPSTrackerPlugin.File.cs 226
• V3090 Unsafe locking on an object of type 'String'. GPSTrackerPlugin.File.cs 244
• V3090 Unsafe locking on an object of type 'String'. GPSTrackerPlugin.File.cs 370
• V3090 Unsafe locking on an object of type 'String'. GPSTrackerPlugin.File.cs 416
• V3090 Unsafe locking on an object of type 'String'. GPSTrackerPlugin.File.cs 448
• V3090 Unsafe locking on an object of type 'String'. GPSTrackerPlugin.File.cs 723
• V3090 Unsafe locking on an object of type 'String'. GPSTrackerPlugin.WorldWind.cs 339
• V3090 Unsafe locking on an object of type 'String'. GPSTrackerPlugin.WorldWind.cs 394
• V3090 Unsafe locking on an object of type 'String'. GPSTrackerPlugin.WorldWind.cs 468
• V3090 Unsafe locking on an object of type 'String'. GPSTrackerPlugin.WorldWind.cs 538
• V3090 Unsafe locking on an object of type 'String'. GPSTracker.cs 3853
• V3090 Unsafe locking on an object of type 'String'. GPSTracker.cs 6787
• V3090 Unsafe locking on a type. All instances of a type will have the same 'Type' object. JHU_Globals.cs 73
• V3090 Unsafe locking on a type. All instances of a type will have the same 'Type' object. JHU_Globals.cs 222
• V3090 Unsafe locking on a type. All instances of a type will have the same 'Type' object. JHU_Log.cs 145

Использование & вместо &&

public static String GetDocumentSource(....)
{
 ....
 int iSize = 2048;
 byte[] bytedata = new byte[2048];
 int iBOMLength = 0;

 while (true)
 {
  iSize = memstream.Read(bytedata, 0, bytedata.Length);
  if (iSize > 0)
   {
    if (!IsUnicodeDetermined)
     {
        ....
        if ((bytedata[0] == 0xEF) & 
            (bytedata[1] == 0xBB) & 
            (bytedata[2] == 0xBF)) //UTF8
            {
              IsUTF8 = true;
              IsBOMPresent = true;
            }

        if (!IsUTF16LE & !IsUTF16BE & !IsUTF8)
          {
            if ((bytedata[1] == 0) & 
                (bytedata[3] == 0) & 
                (bytedata[5] == 0) & 
                (bytedata[7] == 0))
                {
                  IsUTF16LE = true; //best guess
                }
          }
    ....
    }
  }
  ....
}

Предупреждения:

• V3093 The '&' operator evaluates both operands. Perhaps a short-circuit '&&' operator should be used instead. utils.cs 280
• V3093 The '&' operator evaluates both operands. Perhaps a short-circuit '&&' operator should be used instead. utils.cs 291

Сложно сказать, может привести этот код к ошибке или нет. Однако, выглядит он опасным, так как оператор & вычисляет оба операнда перед выполнением. Вместо него стоит использовать оператор &&. Если вдруг окажется, что программа, например, прочитала только один символ, все равно будет происходить обращение к элементам bytedata[2], bytedata[3] и так далее, что может привести к неожиданным и неприятным последствиям.

Лишний код

protected IWidgetCollection m_ChildWidgets = 
                     new WidgetCollection();

public interface IWidgetCollection
{
  ....
  IWidget this[int index] {get;set;}
  ....
}

public void Render(DrawArgs drawArgs)
{
 ....
 for(int index = m_ChildWidgets.Count-1; index>=0; index--)
  {
   IWidget currentChildWidget = 
            m_ChildWidgets[index] as IWidget; // <=
   ....
  }
 ....
}       

Предупреждение:

• V3051 An excessive type cast. The object is already of the 'IWidget' type. PanelWidget.cs 749

Это, конечно, не является ошибкой, но явно лишний код. Приведение объекта к типу, которым он уже является, не несёт пользы. Если убрать as, то ничего не изменится. А в случае, если тип не совпадает, то код просто не скомпилируется. Данное сообщение встречается слишком много раз, и я думаю, лишний код в столь многих местах надо исправить. Остальные обнаруженные места:

• V3051 An excessive type cast. The object is already of the 'IWidget' type. FormWidget.cs 1174
• V3051 An excessive type cast. The object is already of the 'IWidget' type. RootWidget.cs 80
• V3051 An excessive type cast. The object is already of the 'IWidget' type. RootWidget.cs 219
• V3051 An excessive type cast. The object is already of the 'IWidget' type. RootWidget.cs 244
• V3051 An excessive type cast. The object is already of the 'IWidget' type. RootWidget.cs 269
• V3051 An excessive type cast. The object is already of the 'IWidget' type. RootWidget.cs 294
• V3051 An excessive type cast. The object is already of the 'IWidget' type. RootWidget.cs 313
• V3051 An excessive type cast. The object is already of the 'IWidget' type. RootWidget.cs 337
• V3051 An excessive type cast. The object is already of the 'IWidget' type. RootWidget.cs 362
• V3051 An excessive type cast. The object is already of the 'IWidget' type. RootWidget.cs 387
• V3051 An excessive type cast. The object is already of the 'IWidget' type. RootWidget.cs 412
• V3051 An excessive type cast. The object is already of the 'IWidget' type. WorldWind.Widgets.RootWidget.cs 24
• V3051 An excessive type cast. The object is already of the 'IWidget' type. WorldWind.Widgets.RootWidget.cs 148
• V3051 An excessive type cast. The object is already of the 'IWidget' type. WorldWind.Widgets.RootWidget.cs 167
• V3051 An excessive type cast. The object is already of the 'IWidget' type. WorldWind.Widgets.RootWidget.cs 186
• V3051 An excessive type cast. The object is already of the 'IWidget' type. WorldWind.Widgets.RootWidget.cs 204
• V3051 An excessive type cast. The object is already of the 'IWidget' type. WorldWind.Widgets.RootWidget.cs 222
• V3051 An excessive type cast. The object is already of the 'IWidget' type. WorldWind.Widgets.RootWidget.cs 246
• V3051 An excessive type cast. The object is already of the 'IWidget' type. WorldWindow.Widgets.Form.cs 429
• V3051 An excessive type cast. The object is already of the 'IWidget' type. JHU_FormWidget.cs 1132
• V3051 An excessive type cast. The object is already of the 'IWidget' type. JHU_RootWidget.cs 80
• V3051 An excessive type cast. The object is already of the 'IWidget' type. JHU_RootWidget.cs 215
• V3051 An excessive type cast. The object is already of the 'IWidget' type. JHU_RootWidget.cs 240
• V3051 An excessive type cast. The object is already of the 'IWidget' type. JHU_RootWidget.cs 265
• V3051 An excessive type cast. The object is already of the 'IWidget' type. JHU_RootWidget.cs 290
• V3051 An excessive type cast. The object is already of the 'IWidget' type. JHU_RootWidget.cs 315
• V3051 An excessive type cast. The object is already of the 'IWidget' type. JHU_RootWidget.cs 340
• V3051 An excessive type cast. The object is already of the 'IWidget' type. JHU_RootWidget.cs 365
• V3051 An excessive type cast. The object is already of the 'IWidget' type. JHU_RootWidget.cs 390
• V3051 An excessive type cast. The object is already of the 'IWidget' type. JHU_PanelWidget.cs 744

Странный код

public void LoadUrl(String url)
{
  ....
  if (!isCreated)
   {
    Debug.WriteLine("Doc not created" + 
                    iLoadAttempts.ToString());
    if (iLoadAttempts < 2)
     {
       this.bLoadUrlWhenReady = true;
       return;
     }
     else
     {
       throw new HtmlEditorException("Document not created");
     }
   }

  if (!isCreated) return; // <=
  ....
}

Предупреждение:

• V3021 There are two 'if' statements with identical conditional expressions. The first 'if' statement contains method return. This means that the second 'if' statement is senseless HtmlEditor.cs 1480

Тут мы видим пример достаточно странного фрагмента кода. В нём явно есть ошибка, но понять в чём именно она заключается достаточно сложно. Код пытается загрузить страницу, и в случае неудачной загрузки начинает проверки. Если количество попыток загрузки меньше двух, то переходит к следующей попытке, а если нет — выдаёт ошибку. Однако, после выдачи ошибки стоит ещё один принудительный выход из функции. Возможно, это какая-то подстраховка, а может это просто лишний код. К сожалению, определить, что именно не так в этом фрагменте, могут только его разработчики.

Заключение

Как уже не раз говорилось, активное использование копирования кода приводит к частым ошибкам, и этого достаточно сложно избежать. Однако, копирование кода — это слишком удобно, и отказаться от него на практике нет никакой возможности. К счастью, как видите, анализатор PVS-Studio может помочь предотвратить множество ошибок, связанных с копирование кода и опечатками. Предлагаю не откладывать и попробовать его на своём проекте: скачать.