Иногда полезно оглянуться и посмотреть, как мог помочь анализатор в старых проектах, и каких ошибок можно своевременно избежать, если использовать анализатор регулярно. В этот раз выбор пал на проект 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.
Кстати, это уже не первый случай проверки "исторических" проектов. Возможно, Вам также будет интересно познакомиться со следующими статьями:
Проект 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; // <=
}Предупреждение:
Здесь мы видим классическую ошибку в конструкторе копирования. При присваивании трёхмерных координат объекта вместо задания переменой Z было дважды перезаписано значение переменной X. Вполне очевидно, что эта ошибка была допущена в результате использования "Copy-Paste методики". Шанс допустить ошибку в последней строке при копировании кода гораздо выше. Подробней об этом феномене можно почитать в статье Андрея Карпова "Эффект последней строки". Для исправления этого конструктора требуется заменить переменную в последней строке.
public Point3d (Point3d P)
{
X = P.X;
Y = P.Y;
Z = P.Z;
}Это не единственная ошибка в проекте, которая подтверждает этот эффект. В продолжении статьи встретится ещё несколько примеров, доказывающих его.
Ещё несколько подозрительных мест, где сработала диагностика V3008:
private static void WebUpdate(....)
{
....
if (ver != version) // <=
{
....
}
else if (ver != version) // <=
{
....
}
}Предупреждение:
Фрагмент занимается автоматическим обновлением плагина в проекте. При несоответствии версии он скачивает нужный плагин. Если это внутренний плагин, программа отказывается обновлять его автоматически и просто выдаёт сообщение о новой версии. Но условный оператор, расположенный после 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; // <=
}
....
}Предупреждение:
В приведённом фрагменте ошибка, связанная с неаккуратным копированием кода. В последнем операторе условия перепутана переменная. В итоге произошла ошибка с доступом по нулевой ссылке. Согласно предыдущим проверкам, переменная 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=")) // <=
....
}
....
}
....
}Предупреждения:
Ещё один пример ошибки, возникающий из-за больших фрагментов кода, созданных с помощью копирования кода. Приведённая функция предназначена для обработки входящих команд и представляет из себя очень длинный блок кода, состоящий из однотипных условных операторов. Код никогда не дойдёт до повторной проверки. В такой ошибке нет ничего страшного, но вполне может быть, что вместо дубликата должна была располагаться какая-то другая команда. Раз нужная команда не будет обработана, значит код ведёт себя совсем не так, как предполагает программист.
Анализатор указал сразу на два места в этом огромном дереве условных операторов. В первом месте двойная проверка 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);
}Предупреждение:
Опечатка в имени переменной привела к потенциальному использованию нулевой ссылки. Вместо проверки объекта производного класса robj проверяется базовый объект obj. В случае, если он не соответствует типу RenderableObject, программа завершится с ошибкой. Для исправления требуется изменить имя переменой в выражении условного оператора на robj.
public override void Render(DrawArgs drawArgs)
{
....
if(this.linePoints.Length > 1) // <=
{
....
if(this.linePoints != null) // <=
{
....
}
}
....
}Предупреждения:
На этот код указывают сразу две различных диагностики. В коде нарушен порядок действий по проверке и доступе к ссылке. Сначала вычисляют количество элементов внутри объекта, а уже потом проверяют существует ли объект вообще. Возможно, объект 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;
}
}
....
}Предупреждение:
Эта ошибка похожа на описанную в предыдущем разделе. Здесь ссылка на объект присваивается переменной непосредственно перед условными операторами. Скорее всего, ссылка на объект не может быть нулевой, а проверка во внутреннем условии не требуется. Если это не так, проверку надо перенести во внешний условный оператор. В нём уже осуществляется работа со свойствами объекта currentSurfaceImage. Если ссылка будет нулевой, ошибка возникнет раньше, чем будет выполнена проверка.
Похожие места:
public void threadStartFile()
{
....
if (File.Exists(sFileName))
{
....
if (gpsSource.bTrackAtOnce!=true && ....)
{
if (!gpsSource.bWaypoints)
{
m_GpsTracker.m_gpsTrackerPlugin.pluginShowFixInfo("");
....
// <=
gpsSource.sFileNameSession=sFileName;
....
}
else
{
....
}
}
else
{
....
}
} // <=
....
}Предупреждение:
Анализатор обнаружил несоответствие форматирования и логики работы кода. При близком рассмотрении оказалось, что во внутреннем 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;
....
}
....
}Предупреждение:
В функцию поступают координаты двух точек линии, но в результате опечатки значения обеих точек берутся только из переменной 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";
}
....
}Предупреждение:
Поступающий в функцию параметр 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";
}public override bool PerformSelectionAction(DrawArgs drawArgs)
{
....
if(icon.OnClickZoomAltitude != double.NaN ||
icon.OnClickZoomHeading != double.NaN ||
icon.OnClickZoomTilt != double.NaN)
{
....
}
....
}Предупреждение:
Согласно документации, нельзя сравнивать два значения double.NaN с помощью оператора !=. Результат такого сравнения всегда будет равен true. Для корректной проверки следует использовать метод double.IsNaN(). Соответственно, код примет следующий вид:
if(!double.IsNaN(icon.OnClickZoomAltitude) ||
!double.IsNaN(icon.OnClickZoomHeading) ||
!double.IsNaN(icon.OnClickZoomTilt)) ....Анализатор обнаружил много мест с использованием таких некорректных проверок:
private static void addExtendedInformation(....)
{
....
if(toolBarImage.Length > 0 &&
!Path.IsPathRooted(toolBarImage))
Path.Combine(...., toolBarImage); // <=
....
}Предупреждение:
Вызов функции Path.Combine без обработки результата не имеет смысла. В данном случае функция служит для формирования пути к объекту на основании абсолютного пути до исполняемого файла и относительного пути до изображения. Отсутствие обработки значения указывает на явную ошибку. Функция Path.Combine используется во многих местах программы. На основе этого можно сделать вывод, что код надо исправить следующим образом:
toolBarImage=Path.Combine(...., toolBarImage);Ошибка была скопирована и в другие места проекта:
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: ....
}
}
....
}Предупреждение:
Код, видимо, проверяет, находится ли курсор над ToolBar в данный момент, или нет. Из кода видно, что отсутствует обработка элемента MenuAnchor.Left. Точно знать, что подразумевали разработчики при написании данного фрагмента невозможно. Возможно, его обработка не требовалась, но я считаю - надо обратить внимание на этот фрагмент.
protected virtual void CreateElevatedMesh()
{
....
if (minimumElevation > maximumElevation)
{
// Compensate for negative vertical exaggeration
minimumElevation = maximumElevation;
maximumElevation = minimumElevation;
}
....
}Предупреждение:
Здесь просто избыточный код. Наличие строки maximumElevation = minimumElevation не имеет смысла, так как на момент присваивания обе переменные имеют одинаковые значения в результате выполнения предыдущей операции. Можно, конечно, предположить, что разработчики хотели поменять значения переменных, но сделали это некорректно. Это сомнительное предположение, так как и комментарий, и то, что переменная maximumElevation больше не используется, доказывают обратное.
public static bool SearchForAddress(....)
{
double num1;
long2 = num1 = 0;
long1 = num1 = num1; // <=
lat2 = num1 = num1; // <=
lat1 = num1;
....
}Предупреждения:
Опять же эффект Copy-Paste, который часто встречается в проекте. Опасности этот фрагмент не несёт, но присваивание значения трижды одной и той же переменой смотрится странно. На мой взгляд, было бы нагляднее инициализировать переменную num1 непосредственно при её объявлении, а уж после - присваивать значение num1 раздельно каждой переменной.
private static void m_timer_Elapsed(....)
{
....
if (Elapsed != null)
Elapsed(sender, e);
}Предупреждение:
В многопоточном приложении такой вызов события не является безопасным. Вполне может произойти ситуация, когда между проверкой на null и непосредственно вызовом обработчика произойдёт отписка от события в другом потоке. Если это был единственный обработчик, то это приведёт к использованию нулевой ссылки. Для безопасного вызова события рекомендуется использовать временную переменную, тогда событие будет вызвано корректно в любом случае. О других способах исправления этой ошибки можно почитать на странице диагностики V3083.
Кстати, это очень коварный вид ошибки, так как проблемы будут возникать крайне редко, а воспроизвести их повторно будет вообще почти невозможно!
Прочие небезопасные вызовы приведу списком:
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;
}Предупреждение:
Использование в качестве объекта для блокировки текстовой строки не является безопасным. Доступ к таким объектам можно получить из любого места программы. В результате может произойти взаимная блокировка, так как в обоих случаях при анализе строки будет получена ссылка на один и тот же объект в памяти.
Ещё несколько мест обнаруженных анализатором:
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
}
}
....
}
}
....
}Предупреждения:
Сложно сказать, может привести этот код к ошибке или нет. Однако, выглядит он опасным, так как оператор & вычисляет оба операнда перед выполнением. Вместо него стоит использовать оператор &&. Если вдруг окажется, что программа, например, прочитала только один символ, все равно будет происходить обращение к элементам 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; // <=
....
}
....
}Предупреждение:
Это, конечно, не является ошибкой, но явно лишний код. Приведение объекта к типу, которым он уже является, не несёт пользы. Если убрать as, то ничего не изменится. А в случае, если тип не совпадает, то код просто не скомпилируется. Данное сообщение встречается слишком много раз, и я думаю, лишний код в столь многих местах надо исправить. Остальные обнаруженные места:
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; // <=
....
}Предупреждение:
Тут мы видим пример достаточно странного фрагмента кода. В нём явно есть ошибка, но понять в чём именно она заключается достаточно сложно. Код пытается загрузить страницу, и в случае неудачной загрузки начинает проверки. Если количество попыток загрузки меньше двух, то переходит к следующей попытке, а если нет - выдаёт ошибку. Однако, после выдачи ошибки стоит ещё один принудительный выход из функции. Возможно, это какая-то подстраховка, а может это просто лишний код. К сожалению, определить, что именно не так в этом фрагменте, могут только его разработчики.
Как уже не раз говорилось, активное использование копирования кода приводит к частым ошибкам, и этого достаточно сложно избежать. Однако, копирование кода - это слишком удобно, и отказаться от него на практике нет никакой возможности. К счастью, как видите, анализатор PVS-Studio может помочь предотвратить множество ошибок, связанных с копирование кода и опечатками. Предлагаю не откладывать и попробовать его на своём проекте: скачать.