It has become a tradition for newly employed developers on the PVS-Studio team to start off by writing an article reviewing bugs found by the analyzer in some open-source project. Telerik UI for UWP is the project picked for today's review.
PVS-Studio is a tool for detecting bugs and potential vulnerabilities in the source code of programs written in C, C++, C#, and Java. The analyzer runs on Windows, Linux, and macOS.
PVS-Studio can be run in a number of ways:
Telerik UI for UWP is a set of UI controls for the Universal Windows Platform (UWP). The project's source code is available at GitHub. The set includes over 20 components allowing users to visualize data in chart form, create lists and tables, and use a map to display content in a geographical context.
PVS-Studio diagnostic message: 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);
}
}
}Two methods, OnMinValuePropertyChanged and OnMaxValuePropertyChanged, perform the same actions. I strongly suspect there's a bug here. Note that both methods call the same method, RaiseMinimumPropertyChangedEvent, while the RadGaugeAutomationPeer class implements individual methods for "Minimum" and "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);
}The RaiseMinimumPropertyChangedEvent method is used twice, while the RaiseMaximumPropertyChangedEvent method is not used at all. This makes me doubt the OnMaxValuePropertyChanged method works well... I guess it was meant to look like this:
private static void OnMaxValuePropertyChanged(
DependencyObject sender,
DependencyPropertyChangedEventArgs args)
{
....
peer.RaiseMaximumPropertyChangedEvent((double)args.OldValue,
(double)args.NewValue);
....
}But even with this fix, the code doesn't look neat because of the numerous duplicate elements. It's difficult to read, and the similarly looking lines dull your attention, which makes code review a difficult job. Static analysis tools, on the contrary, can easily handle it (which doesn't mean you shouldn't refactor your code and especially eliminate duplicate lines).
Looking at this fragment and the next one, I suspect that the project authors indulge in copy-paste every now and then. Well, we all do... :)
PVS-Studio diagnostic message: 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);
}Both operands of the '||' operator in the if statement's conditional expression are represented by identical subexpressions. Obviously, the second subexpression should be different. Maybe the second RenderSize was meant to be DesiredSize or maybe the second subexpression shouldn't be there at all. In any case, this code needs fixing.
PVS-Studio diagnostic message: 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;
}
....
}The text entered into the textbox field is read into a variable. The string's first character is then compared twice with the character '-', which doesn't look right. Obviously, this function doesn't perform text validation as intended.
PVS-Studio diagnostic message: 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) // <=
....
}Both conditional expressions, newValue.HasValue and newValue != null, return true if newValue has a value. The analyzer points this out, but whether this bug should be fixed by removing one of the subexpressions or by replacing it with another one (in case there was something else to be checked) is something that only the authors of this code can figure out.
PVS-Studio diagnostic message: 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();
}
}
}If the CurrentAttachedMenu variable happens to be equal to null, evaluating the CurrentAttachedMenu.IsOpen expression will result in raising an exception. It looks as if it's just a typo and the developers actually meant the opposite operation, '!=', rather than the null check, but if that is the case, the condition of the if statement will throw an exception if the CurrentAttachedMenu variable is equal to null.
There were 37 more warnings of this type, some of which apparently point to genuine bugs. But that's a bit too many warnings for one article, so I'll skip them.
PVS-Studio diagnostic message: 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;
}
....
}The programmer must have confused one variable for another. The null check is done on the source reference, dragDropElement, rather than the reference resulting from the cast, uiDragDropElement, which is the one that was actually meant to be checked. This assumption is supported by the fact that uiDragDropElement is used further without any null checks.
PVS-Studio diagnostic message: 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;
}
....
}Two conditions check the same variable showIndicatorWhenNoData. The second check might be redundant, but it's also possible that one of the duplicate subexpressions should be entirely something else.
PVS-Studio diagnostic message: 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);
}
}Technically speaking, this snippet is correct; the analyzer just points out certain redundancy in the condition. But keep in mind that redundant code is often a sign of a programming mistake such as checking one variable more times than necessary instead of some other variable.
The condition can be simplified a bit by removing unnecessary code as follows:
internal class SelectedItemCollection : ObservableCollection<object>
{
....
private bool CanInsertItem(object item)
{
return this.suspendLevel == 0 && this.AllowSelect &&
(this.AllowMultipleSelect || this.Count == 0);
}
}Other similar warnings:
Let's consider another piece of code, to which the analyzer issued the following:
PVS-Studio diagnostic messages:
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));
}The variables leftMargin and topMargin are assigned some values but never used after that. It's not necessarily a bug, but code like that still looks suspicious. It could be a sign of a typo or bad refactoring.
There was another warning of this type: V3137 The 'currentColumnLength' variable is assigned but is not used by the end of the function. WrapLayout.cs 824
PVS-Studio diagnostic message: 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);
}
}The index parameter of the FindGroupAndItemIndex method is overwritten before use. Most likely, this indicates a programmer error.
PVS-Studio diagnostic message: 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);
}
}
}The event handler is called in a potentially unsafe way, at the risk of raising a NullReferenceException. This will happen if the event has no subscribers left between the null check and the call of the event handler.
The report points out 49 more problems of this type. They are not much interesting to discuss here, and after all, the project authors can easily find them with PVS-Studio on their own, so let's skip over to the next examples.
PVS-Studio diagnostic message: 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;
}
....
}
....
}A NullReferenceException may be raised when addressing the info.Target.Opacity property. To better understand what the problem is about, we need to take a look at certain blocks of the PlayAnimationInfo class, particularly the Target property.
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;
}
}
....
}Actually, the deeper you dig into this code, the more potential issues you unearth. Let's take a look at the most interesting one – the one that triggered the warning. The problem is that even if execution follows the then branch of the if statement, it doesn't guarantee returning a non-null reference even if we're not taking the effects of type conversion into account (the object is initialized by the constructor).
How is that possible? You see, if the object referred to by WeakReference is garbage-collected between the IsAlive check and the call to Target, this.target.Target will return null. That is, the IsAlive check does not guarantee that the object will be still available the next time you call Target.
By the way, the return null; issue is detected by another diagnostic: V3080 Possible null dereference. Consider inspecting 'info.Target'. FadeAnimation.cs 84
There were a few more defects like that:
Let's move on to the next example.
PVS-Studio diagnostic message: 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);
}
}
}To figure out the meaning of this warning, we need to look at the NotifyCollectionChangedEventArgs constructor's parameters:
public NotifyCollectionChangedEventArgs(
NotifyCollectionChangedAction action,
object newItem,
object oldItem,
int index);The analyzer tells us that the variables oldItem and newItem are swapped in the following expression:
return new NotifyCollectionChangedEventArgs(
action,
oldItem,
newItem,
changeIndex);However, the constructor's implementation has those variables listed in the opposite order. You can only wonder whether or not this was done on purpose.
PVS-Studio diagnostic message: 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;
}
....
}The elements x[0] and y[0] are compared at each loop iteration. But since only the first elements are compared, the loop doesn't make sense. The developers probably intended to compare the arrays' respective elements instead. In that case, the correct version would look like this:
for (int i = 0; i < x.Length; i++)
{
if (!object.Equals(x[i], y[i]))
{
return false;
}
}PVS-Studio diagnostic message: 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);
....
}This warning deals with the use of the '?:' operator. Its precedence is lower than that of !=, ||, and ==, which means the order of evaluating the expression above may be different from the expected one. This particular case seems to be a false positive, with the code actually working as intended. But code like that is very difficult to read, and you can never be sure if you understood it correctly. It looks as if it was written that way deliberately so that no one could figure it out :) The best way to make it easier to read is to use parentheses or an if statement.
PVS-Studio diagnostic message: 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));
}
}This bug has to do with a recurring call of the OrderBy method on a collection of type IOrderedEnumerable. The collection is first sorted by columns and then by rows. The problem is that the result of the first sort – by columns – is not stored anywhere and it will be lost when the sort by rows starts. If you want to keep the result of the column-wise sort and do multiple-criteria sort, use the ThenBy method:
this.MergeCellSelectionRegions(selectedItemsInView
.OrderBy(c => c.Column.ItemInfo.LayoutInfo.Line)
.ThenBy(c => c.RowItemInfo.LayoutInfo.Line));PVS-Studio diagnostic message: 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
{
....
}
....
}The analyzer found it strange that the currentColumnLength variable is assigned a value twice while not being used anywhere between these two assignments. No matter the condition, the variable will eventually end up as null. This code is either faulty or redundant.
PVS-Studio diagnostic message: 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();
}
....
}The two identical conditions above appeared as a result of a typo. The exception thrown by this code suggests that the second condition should look like this:
if (this.emptyIconContainer == null)
{
throw new MissingTemplatePartException(
"EmptyIconContainer", typeof(Border));
}PVS-Studio diagnostic message: 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;
}
}
}The break statement is not part of the if statement. It will execute no matter what value is stored in pair.IsFrozen, so the foreach loop will iterate only once.
That's all for my review of bugs found in Telerik. We are ready to provide the developers with a free temporary license so that they can do a more thorough analysis and fix the defects. They can also make use of the free PVS-Studio licensing options available to open-source developers.
Though the authors of Telerik UI for UWP have done a big job developing their project, they still let a number of typos creep in, as it typically happens with us :). All those bugs could have been easily caught and fixed using a static analyzer, but the crucial thing to remember about static analysis is that it should be used in the right way and on a regular basis.