Мы используем куки, чтобы пользоваться сайтом
было удобно.
Уже стало традицией, что программисты, пополняющие команду PVS-Studio, начинают свою деятельность с написания статьи про анализ проекта с открытым исходным кодом. В этот раз таким проверенным проектом станет Telerik UI for UWP.
PVS-Studio - это инструмент для выявления ошибок и потенциальных уязвимостей в исходном коде программ, написанных на языках C, C++, C# и Java. Работает на Windows, Linux и macOS.
Анализатор предоставляет различные сценарии использования:
Telerik UI for UWP - это набор компонентов пользовательского интерфейса для универсальной платформы Windows (UWP). Исходный код проекта можно найти на GitHub. Набор включает в себя более 20 компонентов, позволяющих визуализировать данные в виде графиков, создавать списки и таблицы, использовать карту для демонстрации данных, привязанных к местоположению.
Предупреждение PVS-Studio: V3013 It is odd that the body of 'OnMinValuePropertyChanged' function is fully equivalent to the body of 'OnMaxValuePropertyChanged' function. RadGauge.cs 446
private static void OnMinValuePropertyChanged(
DependencyObject sender,
DependencyPropertyChangedEventArgs args)
{
double newVal = (double)args.NewValue;
ValidateValue(newVal);
RadGauge gauge = sender as RadGauge;
if (gauge.panel != null)
{
gauge.panel.UpdateOnMinMaxValueChange();
}
if(AutomationPeer.ListenerExists(AutomationEvents.PropertyChanged))
{
var peer = FrameworkElementAutomationPeer.FromElement(gauge)
as RadGaugeAutomationPeer;
if (peer != null)
{
peer.RaiseMinimumPropertyChangedEvent((double)args.OldValue,
(double)args.NewValue);
}
}
}
private static void OnMaxValuePropertyChanged(
DependencyObject sender,
DependencyPropertyChangedEventArgs args)
{
double newVal = (double)args.NewValue;
ValidateValue(newVal);
RadGauge gauge = sender as RadGauge;
if (gauge.panel != null)
{
gauge.panel.UpdateOnMinMaxValueChange();
}
if (AutomationPeer.ListenerExists(AutomationEvents.PropertyChanged))
{
var peer = FrameworkElementAutomationPeer.FromElement(gauge)
as RadGaugeAutomationPeer;
if (peer != null)
{
peer.RaiseMinimumPropertyChangedEvent((double)args.OldValue,
(double)args.NewValue);
}
}
}Анализатор обнаружил два метода, OnMinValuePropertyChanged и OnMaxValuePropertyChanged, выполняющих одинаковые действия. У меня есть сильные подозрения, что в этот код закралась ошибка. Обратите внимание, что и в методе OnMinValuePropertyChanged, и в методе OnMaxValuePropertyChanged, используется RaiseMinimumPropertyChangedEvent. При этом в классе RadGaugeAutomationPeer можно встретить как метод для "Minimum", так и для "Maximum":
internal void RaiseMaximumPropertyChangedEvent(double oldValue, double newValue)
{
this.RaisePropertyChangedEvent(
RangeValuePatternIdentifiers.MaximumProperty,
oldValue,
newValue);
}
internal void RaiseMinimumPropertyChangedEvent(double oldValue, double newValue)
{
this.RaisePropertyChangedEvent(
RangeValuePatternIdentifiers.MinimumProperty,
oldValue,
newValue);
}Метод RaiseMaximumPropertyChangedEvent в коде не используется ни разу, а вот RaiseMinimumPropertyChangedEvent - дважды. И, знаете, работоспособность метода OnMaxValuePropertyChanged вызывает вопросы... Я думаю, что предполагалось написать следующее:
private static void OnMaxValuePropertyChanged(
DependencyObject sender,
DependencyPropertyChangedEventArgs args)
{
....
peer.RaiseMaximumPropertyChangedEvent((double)args.OldValue,
(double)args.NewValue);
....
}Но даже так код выглядит не очень аккуратно из-за большого количества повторяющихся элементов. Он сложен для восприятия, а повторяющиеся строки притупляют внимание программиста, и проводить code review в таких условиях становится сложнее. Зато с проверкой такого кода отлично справляются инструменты статического анализа (но это не значит, что можно обойтись без рефакторинга и особенно без сокращения количества повторяющихся строк).
По рассмотренному и следующему фрагменту кода можно предположить, что авторы не прочь позаниматься copy-paste. Впрочем, как и все мы... :)
Предупреждение PVS-Studio: V3001 There are identical sub-expressions 'element.RenderSize == emptySize' to the left and to the right of the '||' operator. TiltInteractionEffect.cs 181
private static bool IsPointInElementBounds(FrameworkElement element,
Point position)
{
Size emptySize = new Size(0, 0);
if (element.RenderSize == emptySize ||
element.RenderSize == emptySize)
{
return false;
}
return new Rect(....).Contains(position);
}Анализатор обнаружил ошибочный фрагмент кода, в котором справа и слева от оператора '||' в условии оператора if используются одинаковые подвыражения. Второе подвыражение явно должно было выглядеть иначе. Возможно, на месте второго RenderSize должен стоять DesiredSize. Или второго подвыражения вообще не должно быть. В любом случае, этот фрагмент кода требует исправления.
Предупреждение PVS-Studio: V3001 There are identical sub-expressions 'text[0] == '-'' to the left and to the right of the '||' operator. RadNumericBox.cs 1057
private void ValidateText()
{
string text = this.textBox.Text;
....
if (text.Length == 1 && (text[0] == '-' || text[0] == '-'))
{
if (this.isNegative)
{
this.isNegative = false;
}
else
{
this.SetText(string.Empty);
}
return;
}
....
}Тут разработчик записывает текст, введенный в поле текстбокса, в переменную. Затем первый символ строки сравнивается дважды с одним и тем же символом '-', что является подозрительным решением. Очевидно, валидация текста в этой функции работает не так, как изначально задумывалось.
Предупреждение PVS-Studio: V3001 There are identical sub-expressions 'newValue.HasValue' to the left and to the right of the '&&' operator. DateTimePicker.cs 576
private static void OnValueChanged(object sender,
DependencyPropertyChangedEventArgs args)
{
DateTimePicker picker = sender as DateTimePicker;
var newValue = (DateTime?)args.NewValue;
if (newValue.HasValue && newValue != null) // <=
....
}Выражение newValue.HasValue возвращает true, если newValue содержит какое-либо значение, и выражение newValue != null делает то же самое. Анализатор обращает на это внимание, а что делать - убрать одно из подвыражений или заменить его на другое (если проверяться должно было что-то ещё), решать уже разработчикам.
Предупреждение PVS-Studio: V3125 The 'CurrentAttachedMenu' object was used after it was verified against null. Check lines: 98, 96. PopupService.cs 98
internal static class PopupService
{
....
private static void Overlay_PointerPressed(....)
{
if (CurrentAttachedMenu == null ||
!CurrentAttachedMenu.hitTestService.
HitTest(e.GetCurrentPoint(CurrentAttachedMenu).Position).Any())
{
CurrentAttachedMenu.IsOpen = false;
HideOverlay();
}
}
}Если переменная CurrentAttachedMenu окажется равна null, то вычисление выражения CurrentAttachedMenu.IsOpen приведёт к возникновению исключения. С первого взгляда кажется, что это простая опечатка и имелось в виду не сравнение с null, а обратная операция - '!='. Но тогда возникнет исключение в условии оператора if, если переменная CurrentAttachedMenu будет иметь значение null.
Далее в коде встретилось ещё 37 таких же предупреждений, часть из которых по всей видимости свидетельствует об ошибках. Но описывать их в рамках одной статьи - все же перебор, поэтому оставлю их без внимания.
Предупреждение PVS-Studio: V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'dragDropElement', 'uiDragDropElement'. DragDrop.cs 91
internal static void StartDrag(....)
{
var dragDropElement = sender as IDragDropElement;
....
UIElement uiDragDropElement = dragDropElement as UIElement;
....
if (dragDropElement == null ||
!dragDropElement.CanStartDrag(trigger, initializeContext))
{
return;
}
....
}Тут же весьма вероятно, что автор перепутал переменные. На неравенство null проверяется не полученная в результате приведения ссылка, а исходная (dragDropElement). Скорее всего проверить предполагалось всё же ссылку uiDragDropElement. Догадку подтверждает и то, что программист далее использовал uiDragDropElement без проверок на значение null.
Предупреждение PVS-Studio: V3030 Recurring check. The '!showIndicatorWhenNoData' condition was already verified in line 139. RadDataBoundListBox.PullToRefresh.cs 141
internal void HandlePullToRefreshItemStateChanged(object item, ItemState state)
{
....
bool showIndicatorWhenNoData = this.ShowPullToRefreshWhenNoData;
if (this.realizedItems.Count == 0 && !showIndicatorWhenNoData)
{
if (state == ItemState.Recycled && !showIndicatorWhenNoData)
{
this.StopPullToRefreshLoading(false);
this.HidePullToRefreshIndicator();
}
return;
}
....
}Анализатор нашел участок кода, в котором в двух условиях повторно проверяется одна и та же переменная showIndicatorWhenNoData. Возможно, проверка просто избыточна, но также есть вероятность, что одно из дублирующихся подвыражений вообще должно быть другим.
Предупреждение PVS-Studio: V3031 An excessive check can be simplified. The '||' operator is surrounded by opposite expressions. SelectedItemCollection.cs 77
internal class SelectedItemCollection : ObservableCollection<object>
{
....
private bool CanInsertItem(object item)
{
return this.suspendLevel == 0 && this.AllowSelect &&
((!this.AllowMultipleSelect && this.Count == 0)
|| this.AllowMultipleSelect);
}
}Этот участок кода формально не является ошибочным. Анализатор намекает на некоторую избыточность кода в условии. Однако стоит помнить, что лишний код иногда является следствием ошибки, например, когда вместо одной переменной несколько раз проверили другую.
Можно немного это условие сократить и убрать лишний код. Например, вот так:
internal class SelectedItemCollection : ObservableCollection<object>
{
....
private bool CanInsertItem(object item)
{
return this.suspendLevel == 0 && this.AllowSelect &&
(this.AllowMultipleSelect || this.Count == 0);
}
}Другие подобные сообщения:
Рассмотрим еще один участок кода, на который анализатор выдал следующее:
Предупреждения PVS-Studio:
internal static class DragDrop
{
....
double leftMargin = 0d;
double topMargin = 0d;
if (frameworkElementSource != null)
{
leftMargin = frameworkElementSource.Margin.Left; // <=
topMargin = frameworkElementSource.Margin.Top; // <=
}
if (dragDropElement == null ||
!dragDropElement.CanStartDrag(trigger, initializeContext))
{
return;
}
var context = dragDropElement
.DragStarting(trigger, initializeContext);
if (context == null)
{
return;
}
var startDragPosition = e
.GetCurrentPoint(context.DragSurface.RootElement).Position;
var relativeStartDragPosition = e
.GetCurrentPoint(uiDragDropElement).Position;
var dragPositionMode = DragDrop
.GetDragPositionMode(uiDragDropElement);
AddOperation(new DragDropOperation(
context,
dragDropElement,
dragPositionMode,
e.Pointer,
startDragPosition,
relativeStartDragPosition));
}Анализатор сообщает о том, что переменным leftMargin и topMargin были присвоены значения, но после эти переменные не используются вплоть до конца метода. Вероятно, здесь нет ошибки, но такой код выглядит подозрительно. Это может быть следствием опечатки или неудачного рефакторинга.
Такая же проблема была найдена в другом месте: V3137 The 'currentColumnLength' variable is assigned but is not used by the end of the function. WrapLayout.cs 824
Предупреждение PVS-Studio: V3061 Parameter 'index' is always rewritten in method body before being used. DataEngine.cs 1443
private static Tuple<Group, int> FindGroupAndItemIndex(.... int index, ....)
{
if (exhaustiveSearch)
{
....
}
else
{
var aggregateRowGroup = rowRootGroup;
var rowGroupNames = valueProvider.GetRowGroupNames(item);
foreach (var groupName in rowGroupNames)
{
Group group;
if (aggregateRowGroup.TryGetGroup(groupName, out group))
{
aggregateRowGroup = group;
}
}
index = aggregateRowGroup.IndexOf(item, // <=
valueProvider.GetSortComparer());
return Tuple.Create(aggregateRowGroup, index);
}
}Параметр index метода FindGroupAndItemIndex перезаписывается прежде, чем используется. Вероятнее всего, это указывает на ошибку программиста.
Предупреждение PVS-Studio: V3083 Unsafe invocation of event 'Completed', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. ActionBase.cs 32
internal abstract class ActionBase
{
....
protected virtual void OnCompleted()
{
this.IsCompleted = true;
if (this.Completed != null)
{
this.Completed(this, EventArgs.Empty);
}
}
}Программист допустил в этом методе потенциально небезопасный вызов обработчика события, что может привести к возникновению исключения типа NullReferenceException. Исключение будет сгенерировано при условии, если между проверкой на null и вызовом обработчиков события подписчиков у этого события не останется.
В коде встречается ещё 49 похожих проблем. Все их в этой статье смотреть будет не интересно, да и авторы легко смогут самостоятельно их найти при помощи PVS-Studio, поэтому перейдем к другим ошибкам.
Предупреждение PVS-Studio: V3145 Unsafe dereference of a WeakReference target, consider inspecting info.Target. The object could have been garbage collected between checking 'IsAlive' and accessing the 'Target' property. FadeAnimation.cs 84
public class RadFadeAnimation : RadAnimation
{
....
protected internal override void
ApplyAnimationValues(PlayAnimationInfo info)
{
....
if (info.Target.Opacity != opacity) // <=
{
info.Target.Opacity = opacity;
}
....
}
....
}Анализатор предупреждает об опасности возникновения исключения типа NullReferenceException при обращении к свойству info.Target.Opacity. Для того, чтобы лучше понять суть проблемы, нужно посмотреть на фрагменты класса PlayAnimationInfo, в частности - на свойство Target.
public class PlayAnimationInfo
{
....
private WeakReference target;
....
public PlayAnimationInfo(Storyboard storyboard,
RadAnimation animation,
UIElement target)
{
....
this.target = new WeakReference(target);
....
}
....
public UIElement Target
{
get
{
if (this.target.IsAlive)
{
return this.target.Target as UIElement;
}
return null;
}
}
....
}Вообще, чем дальше копать этот код, тем больше потенциальных проблем в нём можно найти. Давайте рассмотрим, пожалуй, наиболее интересную - ту, на которую анализатор выдал предупреждение. Дело в том, что даже если исполнение пойдёт по then-ветви оператора if, это не гарантирует того, что будет возвращена ненулевая ссылка. Безотносительно разговоров про приведение - тут всё считаем допустимым за счёт инициализации объекта в конструкторе.
Как такое возможно? Дело в том, что если между проверкой IsAlive и обращением к Target будет выполнена сборка мусора, под которую попадёт объект, на который ссылается WeakReference, this.target.Target вернёт значение null. То есть проверка IsAlive не даёт гарантий того, что при последующем обращении к Target объект всё ещё доступен.
Кстати, ситуацию return null; отлавливает другое диагностическое правило: V3080 Possible null dereference. Consider inspecting 'info.Target'. FadeAnimation.cs 84
Подобные проблемы в коде встретились ещё несколько раз:
Перейдём к следующему примеру.
Предупреждение PVS-Studio: V3066 Possible incorrect order of arguments passed to 'NotifyCollectionChangedEventArgs' constructor: 'oldItem' and 'newItem'. CheckedItemsCollection.cs 470
public class CheckedItemsCollection<T> : IList<T>,
INotifyCollectionChanged
{
....
private NotifyCollectionChangedEventArgs GenerateArgs(....)
{
switch (action)
{
case NotifyCollectionChangedAction.Add:
....
case NotifyCollectionChangedAction.Remove:
....
case NotifyCollectionChangedAction.Replace:
return new NotifyCollectionChangedEventArgs(
action, oldItem, newItem, changeIndex); // <=
default:
return new NotifyCollectionChangedEventArgs(action);
}
}
}Чтобы понять, что означает это предупреждение от анализатора, стоит посмотреть на параметры конструктора NotifyCollectionChangedEventArgs:
public NotifyCollectionChangedEventArgs(
NotifyCollectionChangedAction action,
object newItem,
object oldItem,
int index);Анализатор предупреждает, что в выражении
return new NotifyCollectionChangedEventArgs(
action,
oldItem,
newItem,
changeIndex);поменяли местами переменные oldItem и newItem. В определении конструктора они перечислены в другом порядке. Сознательно это было сделано или нет – остается только догадываться.
Предупреждение PVS-Studio: V3102 Suspicious access to element of 'x' object by a constant index inside a loop. DataEngine.cs 1718
private class ObjectArrayComparer : IEqualityComparer<object[]>
{
public bool Equals(object[] x, object[] y)
{
....
for (int i = 0; i < x.Length; i++)
{
if (!object.Equals(x[0], y[0])) // <=
{
return false;
}
}
return true;
}
....
}На каждой итерации в цикле программист сравнивает x[0] и y[0]. Однако в этом коде цикл не имеет смысла, так как сравниваются только первые элементы. Скорее всего, имелось ввиду сравнение соответствующих элементов массивов. Тогда корректный код будет таким:
for (int i = 0; i < x.Length; i++)
{
if (!object.Equals(x[i], y[i]))
{
return false;
}
}Предупреждение PVS-Studio: V3123 Perhaps the '?:' operator works in a different way than it was expected. Its priority is lower than priority of other operators in its condition. EditRowHostPanel.cs 35
protected override Size MeasureOverride(Size availableSize)
{
....
bool shouldUpdateRowHeight = editorLine == 0 ||
displayedElement == null ? false :
displayedElement.ContainerType != typeof(DataGridGroupHeader);
....
}Предупреждение cвязано с использованием оператора '?:'. Он имеет более низкий приоритет, по сравнению с !=, ||, ==. Поэтому выражение может вычисляться не так, как планировал программист. Судя по всему, в данном случае это ложное срабатывание и код работает правильно. Но читать подобный код очень сложно и никогда нет уверенности, что он понят верно. Ощущение, будто разработчик специально так написал, чтобы никто ничего не понял :) Лучший способ сделать это читабельнее – использовать скобочки или оператор if.
Предупреждение PVS-Studio: V3078 Original sorting order will be lost after repetitive call to 'OrderBy' method. Use 'ThenBy' method to preserve the original sorting. GridModel.Selection.cs 107
internal partial class GridModel
{
private void BuildCellSelectionRegions(....)
{
....
this.MergeCellSelectionRegions(selectedItemsInView
.OrderBy(c => c.Column.ItemInfo.LayoutInfo.Line)
.OrderBy(c => c.RowItemInfo.LayoutInfo.Line));
}
}Ошибка связана с повторным вызовом OrderBy для коллекции типа IOrderedEnumerable. Тут коллекция отсортировывается сначала по столбцам, потом по строкам. Причем в момент сортировки по строкам предыдущая сортировка по столбцам нигде не учитывается. Чтобы не терять сортировку по столбцам и отсортировать коллекцию сразу по нескольким критериям, следует использовать ThenBy:
this.MergeCellSelectionRegions(selectedItemsInView
.OrderBy(c => c.Column.ItemInfo.LayoutInfo.Line)
.ThenBy(c => c.RowItemInfo.LayoutInfo.Line));Предупреждение PVS-Studio: V3008 The 'currentColumnLength' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 791, 785. WrapLayout.cs 791
private void OnAvailableLengthChanged(double oldValue,
double newValue)
{
....
if (....)
{
if (currentColumnLength > 0)
{
var paddingValue = Math.Max(0,
newValue - currentColumnLength);
this.paddingRenderInfo.Add(paddingValue);
currentColumnLength = 0; // <=
slotCount++;
}
this.ColumnSlotsRenderInfo.Update(i, newValue);
this.paddingRenderInfo.Add(0);
currentColumnLength = 0; // <=
slotCount++;
continue;
}
else
{
....
}
....
}Анализатору показалось подозрительным то, что переменной currentColumnLength дважды присваивается значение. Между присвоениями переменная не используется. Вне зависимости от условия, эта переменная окажется в итоге равна нулю. Этот код или некорректен или избыточен.
Предупреждение PVS-Studio: V3127 Two similar code fragments were found. Perhaps, this is a typo and 'emptyIconContainer' variable should be used instead of 'filledIconContainer' RadRatingItem.cs 240
public class RadRatingItem : RadContentControl
{
....
protected override void OnApplyTemplate()
{
....
this.filledIconContainer = this.GetTemplateChild(
"FilledIconContainer") as Border;
if (this.filledIconContainer == null) // <=
{
throw new MissingTemplatePartException(
"FilledIconContainer", typeof(Border));
}
this.emptyIconContainer = this.GetTemplateChild(
"EmptyIconContainer") as Border;
if (this.filledIconContainer == null) // <=
{
throw new MissingTemplatePartException(
"EmptyIconContainer", typeof(Border));
}
this.Initialize();
}
....
}Из-за опечатки в коде появились два одинаковых условия. Судя по генерируемому исключению, второе условие должно выглядеть так:
if (this.emptyIconContainer == null)
{
throw new MissingTemplatePartException(
"EmptyIconContainer", typeof(Border));
}Предупреждение PVS-Studio: V3020 An unconditional 'break' within a loop. NodePool.cs 189
public IEnumerable<KeyValuePair<int, List<T>>>
GetUnfrozenDisplayedElements()
{
foreach (var item in this.generatedContainers)
{
foreach (var pair in item.Value)
{
if (!pair.IsFrozen)
{
yield return item;
}
break;
}
}
}Анализатор обнаружил что тут оператор break не принадлежит к оператору if. Выполнится break вне зависимости от того, какое значение у pair.IsFrozen и из-за этого в foreach, будет выполнена только одна итерация.
На этом я заканчиваю рассмотрение предупреждений. Чтобы разработчики Telerik могли провести более тщательный анализ кода и исправить дефекты, мы готовы предоставить им временную лицензию. Помимо этого, они могут воспользоваться вариантом бесплатного использования PVS-Studio, предоставляемым авторам открытых проектов.
Хотя разработчики Telerik UI for UWP проделали большую работу, но без опечаток, как это обычно и бывает, не обошлось :). Все эти ошибки легко можно было найти статическим анализатором и исправить. Самое главное – использовать анализатор правильно и регулярно.
0
0
0
Мы ответим вам на
Если вы так и не получили ответ, пожалуйста, проверьте, отфильтровано ли письмо в одну из следующих стандартных папок:
0