This article is devoted to checking the OpenRA project using the PVS-Studio static analyzer. What is OpenRA? It is an open source game engine for creating real-time strategy games. The article describes how the analysis was carried out, what features of the project itself were discovered and what interesting triggers PVS-Studio gave. And, of course, here we will consider some of the analyzer's features that made the project verification process more comfortable.
OpenRA
The project selected for review is an RTS game engine in the style of games like Command & Conquer: Red Alert. More information can be found on the website . The source code is written in C # and is available for viewing and use in the repository .
OpenRA was selected for review for 3 reasons. First, it seems to be of interest to many people. In any case, this applies to the inhabitants of GitHub, since the repository has collected more than 8 thousand stars. Secondly, the OpenRA codebase contains 1285 files. Usually this amount is enough to hopefully find interesting triggers in them. And thirdly ... Game engines are cool.
Extra positives
I analyzed OpenRA using PVS-Studio and was initially inspired by the results:
I decided that among so many High-warnings, I can certainly find a whole lot of different responses. And, of course, on their basis I will write the coolest and most interesting article. But it was not there!
One had only to look at the warnings themselves, and everything immediately fell into place. Of the 1306 High warnings, 1,277 were associated with the V3144 diagnostic . It displays messages like "This file is marked with copyleft license, which requires you to open the derived source code". This diagnostics is described in more detail here .
Obviously, the workings of such a plan did not interest me at all, because OpenRA is already an open-source project. Therefore, they needed to be hidden so that they did not interfere with viewing the rest of the log. Since I was using a plugin for Visual Studio, it was easy to do. You just had to right-click on one of the V3144 alerts and select "Hide all V3144 errors" in the menu that opens .
You can also choose which warnings will be displayed in the log by going to the "Detectable Errors (C #)" section in the analyzer options.
In order to go to them using the plugin for Visual Studio 2019, you need to click on the top menu Extensions-> PVS-Studio-> Options.
Test results
After V3144 triggers were filtered out, there are significantly fewer warnings in the log:
Nevertheless, we managed to find interesting moments among them.
Pointless conditions
Quite a few triggers indicated unnecessary checks. This may indicate an error, because usually people do not write such code on purpose. However, in OpenRA, quite often it looks like these unnecessary conditions were added on purpose. For instance:
public virtual void Tick()
{
....
Active = !Disabled && Instances.Any(i => !i.IsTraitPaused);
if (!Active)
return;
if (Active)
{
....
}
}Analyzer warning : V3022 Expression 'Active' is always true. SupportPowerManager.cs 206
PVS-Studio quite rightly notes that the second check is meaningless, because if Active is false , then the execution will not reach it. It might be a mistake, but I think it was written on purpose. What for? Well, why not?
Perhaps we have before us a kind of temporary solution, the revision of which is left "for later." In such cases, it is quite convenient that the analyzer will remind the developer of such imperfections.
Let's look at one more check "just in case":
Pair<string, bool>[] MakeComponents(string text)
{
....
if (highlightStart > 0 && highlightEnd > highlightStart) // <=
{
if (highlightStart > 0) // <=
{
// Normal line segment before highlight
var lineNormal = line.Substring(0, highlightStart);
components.Add(Pair.New(lineNormal, false));
}
// Highlight line segment
var lineHighlight = line.Substring(
highlightStart + 1,
highlightEnd - highlightStart – 1
);
components.Add(Pair.New(lineHighlight, true));
line = line.Substring(highlightEnd + 1);
}
else
{
// Final normal line segment
components.Add(Pair.New(line, false));
break;
}
....
}Analyzer warning : V3022 Expression 'highlightStart> 0' is always true. LabelWithHighlightWidget.cs 54
Again, it is clear that re-checking is completely meaningless. The highlightStart value is checked twice, and in adjacent lines. Error? It is possible that in one of the conditions the wrong variables were selected for testing. Either way, it's hard to say for sure what this is about. One thing is clear - the code needs to be studied and corrected, or an explanation should be left if additional verification is still needed for some reason.
Here's another similar point:
public static void ButtonPrompt(....)
{
....
var cancelButton = prompt.GetOrNull<ButtonWidget>(
"CANCEL_BUTTON"
);
....
if (onCancel != null && cancelButton != null)
{
cancelButton.Visible = true;
cancelButton.Bounds.Y += headerHeight;
cancelButton.OnClick = () =>
{
Ui.CloseWindow();
if (onCancel != null)
onCancel();
};
if (!string.IsNullOrEmpty(cancelText) && cancelButton != null)
cancelButton.GetText = () => cancelText;
}
....
}Analyzer warning : V3063 A part of conditional expression is always true if it is evaluated: cancelButton! = Null. ConfirmationDialogs.cs 78
cancelButton can indeed be null , because the value returned by the GetOrNull method is written to this variable . However, it is logical to consider that in the body of the conditional statement cancelButton no way turn into null . Nevertheless, there is still a check. If you do not pay attention to the external condition, then in general a strange situation turns out: first, the properties of the variable are accessed, and then the developer decided to make sure - is it still null or not?
At first, I assumed that the project might be using some specific logic related to overloading the "==" operator. In my opinion, implementing something similar for reference types in a project is a controversial idea. Still, the unusual behavior makes it harder for other developers to understand the code. At the same time, it is difficult for me to imagine a situation when such tricks cannot be dispensed with. Although it is likely that in some specific case, this would be a convenient solution.
In the Unity game engine, for example, the " == " operator is redefined for the UnityEngine.Object class . The official documentation available at the link shows that comparing instances of this class with nulldoes not work as usual. Well, surely the developers had reasons for implementing such unusual logic.
I didn't find something like that in OpenRA :). So if there is any sense in the previously considered checks for null , then it consists in something else.
PVS-Studio was able to find several more similar moments, but there is no need to list them all here. It's still boring to watch the same positives. Fortunately (or not) the analyzer was able to find other oddities as well.
Unreachable code
void IResolveOrder.ResolveOrder(Actor self, Order order)
{
....
if (!order.Queued || currentTransform == null)
return;
if (!order.Queued && currentTransform.NextActivity != null)
currentTransform.NextActivity.Cancel(self);
....
}Analyzer warning : V3022 Expression '! Order.Queued && currentTransform.NextActivity! = Null' is always false. TransformsIntoTransforms.cs 44
Again we have a pointless check. True, in contrast to the previous ones, here is presented not just an extra condition, but the most real unattainable code. The previously considered always true checks in fact did not affect the operation of the program. They can be removed from the code, or they can be left - nothing will change.
Here, a strange check leads to the fact that part of the code is not executed. At the same time, it's hard for me to guess what changes should be made here as an amendment. In the simplest and most pleasant case, the unreachable code simply shouldn't be executed. Then there is no mistake. However, I doubt the programmer deliberately wrote the line just for beauty.
Uninitialized variable in constructor
public class CursorSequence
{
....
public readonly ISpriteFrame[] Frames;
public CursorSequence(
FrameCache cache,
string name,
string cursorSrc,
string palette,
MiniYaml info
)
{
var d = info.ToDictionary();
Start = Exts.ParseIntegerInvariant(d["Start"].Value);
Palette = palette;
Name = name;
if (
(d.ContainsKey("Length") && d["Length"].Value == "*") ||
(d.ContainsKey("End") && d["End"].Value == "*")
)
Length = Frames.Length - Start;
else if (d.ContainsKey("Length"))
Length = Exts.ParseIntegerInvariant(d["Length"].Value);
else if (d.ContainsKey("End"))
Length = Exts.ParseIntegerInvariant(d["End"].Value) - Start;
else
Length = 1;
Frames = cache[cursorSrc]
.Skip(Start)
.Take(Length)
.ToArray();
....
}
}Analyzer warning : V3128 The 'Frames' field is used before it is initialized in constructor. CursorSequence.cs 35
A very unpleasant moment. Attempting to get the value of the Length property from an uninitialized variable will inevitably throw a NullReferenceException . In a normal situation, such an error would hardly go unnoticed - nevertheless, the impossibility of creating an instance of a class easily reveals itself. But here the exception will only be thrown if the condition
(d.ContainsKey("Length") && d["Length"].Value == "*") ||
(d.ContainsKey("End") && d["End"].Value == "*")will be true.
It's hard to judge how you need to fix the code to make everything work well. I can only assume that the function should look something like this:
public CursorSequence(....)
{
var d = info.ToDictionary();
Start = Exts.ParseIntegerInvariant(d["Start"].Value);
Palette = palette;
Name = name;
ISpriteFrame[] currentCache = cache[cursorSrc];
if (
(d.ContainsKey("Length") && d["Length"].Value == "*") ||
(d.ContainsKey("End") && d["End"].Value == "*")
)
Length = currentCache.Length - Start;
else if (d.ContainsKey("Length"))
Length = Exts.ParseIntegerInvariant(d["Length"].Value);
else if (d.ContainsKey("End"))
Length = Exts.ParseIntegerInvariant(d["End"].Value) - Start;
else
Length = 1;
Frames = currentCache
.Skip(Start)
.Take(Length)
.ToArray();
....
}In this version, the indicated problem is absent, however, only the developer can say how much it corresponds to the original idea.
Potential typo
public void Resize(int width, int height)
{
var oldMapTiles = Tiles;
var oldMapResources = Resources;
var oldMapHeight = Height;
var oldMapRamp = Ramp;
var newSize = new Size(width, height);
....
Tiles = CellLayer.Resize(oldMapTiles, newSize, oldMapTiles[MPos.Zero]);
Resources = CellLayer.Resize(
oldMapResources,
newSize,
oldMapResources[MPos.Zero]
);
Height = CellLayer.Resize(oldMapHeight, newSize, oldMapHeight[MPos.Zero]);
Ramp = CellLayer.Resize(oldMapRamp, newSize, oldMapHeight[MPos.Zero]);
....
}Analyzer Warning : V3127 Two similar code fragments were found. Perhaps this is a typo and 'oldMapRamp' variable should be used instead of 'oldMapHeight' Map.cs 964
The analyzer has detected a suspicious moment related to passing arguments to functions. Let's take a look at the calls separately:
CellLayer.Resize(oldMapTiles, newSize, oldMapTiles[MPos.Zero]);
CellLayer.Resize(oldMapResources, newSize, oldMapResources[MPos.Zero]);
CellLayer.Resize(oldMapHeight, newSize, oldMapHeight[MPos.Zero]);
CellLayer.Resize(oldMapRamp, newSize, oldMapHeight[MPos.Zero]);Oddly enough, the last call passes oldMapHeight , not oldMapRamp . Of course, not all such cases are really errors. It is quite possible that everything is written correctly here. But you must admit that this place looks unusual. I am inclined to believe that a mistake has indeed been made here.
A note from a colleague - Andrey Karpov . And I do not see anything strange in this code. This is a classic last line error !
If there is still no error here, then it is worth adding some explanation. After all, if a moment looks like a mistake, then someone will definitely want to fix it.
True, true and nothing but true
The project contains very peculiar methods, the return value of which has the bool type . Their peculiarity lies in the fact that under any conditions they return true . For instance:
static bool State(
S server,
Connection conn,
Session.Client client,
string s
)
{
var state = Session.ClientState.Invalid;
if (!Enum<Session.ClientState>.TryParse(s, false, out state))
{
server.SendOrderTo(conn, "Message", "Malformed state command");
return true;
}
client.State = state;
Log.Write(
"server",
"Player @{0} is {1}",
conn.Socket.RemoteEndPoint,
client.State
);
server.SyncLobbyClients();
CheckAutoStart(server);
return true;
}Analyzer warning : V3009 It's odd that this method always returns one and the same value of 'true'. LobbyCommands.cs 123
Is this code okay? Is there a mistake? It looks very strange. Why didn't the developer use void ?
It is not surprising that the analyzer considers such a place strange, but still we have to admit that the programmer actually had a reason to write this way. Which one?
I decided to see where this method is called and whether its always true return value is being used. It turned out that there is only one reference to it in the same class - in the commandHandlers dictionary , which has the type
IDictionary<string, Func<S, Connection, Session.Client, string, bool>>During initialization, values are added to it
{"state", State},
{"startgame", StartGame},
{"slot", Slot},
{"allow_spectators", AllowSpectators}etc.
We are presented with a rare (I want to believe it) case when static typing creates problems for us. After all, making a dictionary in which the values will be functions with different signatures ... is at least problematic. commandHandlers is only used in the InterpretCommand method :
public bool InterpretCommand(
S server, Connection conn, Session.Client client, string cmd
)
{
if (
server == null ||
conn == null ||
client == null ||
!ValidateCommand(server, conn, client, cmd)
) return false;
var cmdName = cmd.Split(' ').First();
var cmdValue = cmd.Split(' ').Skip(1).JoinWith(" ");
Func<S, Connection, Session.Client, string, bool> a;
if (!commandHandlers.TryGetValue(cmdName, out a))
return false;
return a(server, conn, client, cmdValue);
}Apparently, the developer's goal was the universal ability to match strings of certain operations performed. I think that the way he has chosen is far from the only one, however, it is not so easy to offer something more convenient / correct in such a situation. Especially if you don't use some kind of dynamic or something similar. If you have any ideas on this subject, leave comments. It would be interesting for me to look at various options for solving this problem.
It turns out that the warnings associated with the always true methods in this class are most likely false. And yet ... It is this "most likely" that scares you. You need to be careful with such things, because there may indeed be a mistake among them.
All such positives should be carefully checked and then marked as false if necessary. This is done quite simply. A special comment should be left in the place indicated by the analyzer:
static bool State(....) //-V3009There is another way: you can select the positives that need to be marked as false, and in the context menu, click on "Mark selected messages as False Alarms".
You can learn more about this topic in the documentation .
Extra check for null?
static bool SyncLobby(....)
{
if (!client.IsAdmin)
{
server.SendOrderTo(conn, "Message", "Only the host can set lobby info");
return true;
}
var lobbyInfo = Session.Deserialize(s);
if (lobbyInfo == null) // <=
{
server.SendOrderTo(conn, "Message", "Invalid Lobby Info Sent");
return true;
}
server.LobbyInfo = lobbyInfo;
server.SyncLobbyInfo();
return true;
}Analyzer warning : V3022 Expression 'lobbyInfo == null' is always false. LobbyCommands.cs 851
Another method that always returns true . However, this time we are examining a different type of trigger. You need to study such things carefully enough, because it is far from the fact that this is just redundant code. But first things first.
The Deserialize method never returns null - you can easily verify this by looking at its code:
public static Session Deserialize(string data)
{
try
{
var session = new Session();
....
return session;
}
catch (YamlException)
{
throw new YamlException(....);
}
catch (InvalidOperationException)
{
throw new YamlException(....);
}
}For readability, I have shortened the source code of the method. You can see it in full by following the link . Well, or take my word for it that the session variable here under no circumstances turns into null .
What do we see at the bottom? Deserialize does not return null , if something went wrong it throws exceptions. The developer who wrote the check for null after the call seemed to think differently. Most likely, in an exceptional situation, the SyncLobby method should execute the code that is currently being executed ... and it never is executed, because lobbyInfo is never null :
if (lobbyInfo == null)
{
server.SendOrderTo(conn, "Message", "Invalid Lobby Info Sent");
return true;
}I believe that instead of this "extra" check, you should still use try - catch . Well, or go from the other side and write some TryDeserialize , which in case of an exception will return null .
Possible NullReferenceException
public ConnectionSwitchModLogic(....)
{
....
var logo = panel.GetOrNull<RGBASpriteWidget>("MOD_ICON");
if (logo != null)
{
logo.GetSprite = () =>
{
....
};
}
if (logo != null && mod.Icon == null) // <=
{
// Hide the logo and center just the text
if (title != null)
title.Bounds.X = logo.Bounds.Left;
if (version != null)
version.Bounds.X = logo.Bounds.X;
width -= logo.Bounds.Width;
}
else
{
// Add an equal logo margin on the right of the text
width += logo.Bounds.Width; // <=
}
....
}Analyzer warning : V3125 The 'logo' object was used after it was verified against null. Check lines: 236, 222. ConnectionLogic.cs 236
Something tells me that there is a 100% error here. We have definitely not "extra" checks before us, because the GetOrNull method , most likely, can really return a null reference. What happens if the logo is null ? Calling the Bounds property will throw an exception, which was clearly not in the developer's plans.
Perhaps the fragment needs to be rewritten something like this:
if (logo != null)
{
if (mod.Icon == null)
{
// Hide the logo and center just the text
if (title != null)
title.Bounds.X = logo.Bounds.Left;
if (version != null)
version.Bounds.X = logo.Bounds.X;
width -= logo.Bounds.Width;
}
else
{
// Add an equal logo margin on the right of the text
width += logo.Bounds.Width;
}
}This option is simple enough to understand, although the additional nesting does not look very cool. As a more capacious solution, you can use the null-conditional operator:
// Add an equal logo margin on the right of the text
width += logo?.Bounds.Width ?? 0; // <=Note that I like the top fix better. It is pleasant to read it and no questions arise. But some developers highly value brevity, so I decided to give the second option too :).
Maybe it's OrDefault after all?
public MapEditorLogic(....)
{
var editorViewport = widget.Get<EditorViewportControllerWidget>("MAP_EDITOR");
var gridButton = widget.GetOrNull<ButtonWidget>("GRID_BUTTON");
var terrainGeometryTrait = world.WorldActor.Trait<TerrainGeometryOverlay>();
if (gridButton != null && terrainGeometryTrait != null) // <=
{
....
}
var copypasteButton = widget.GetOrNull<ButtonWidget>("COPYPASTE_BUTTON");
if (copypasteButton != null)
{
....
}
var copyFilterDropdown = widget.Get<DropDownButtonWidget>(....);
copyFilterDropdown.OnMouseDown = _ =>
{
copyFilterDropdown.RemovePanel();
copyFilterDropdown.AttachPanel(CreateCategoriesPanel());
};
var coordinateLabel = widget.GetOrNull<LabelWidget>("COORDINATE_LABEL");
if (coordinateLabel != null)
{
....
}
....
}Analyzer warning : V3063 A part of conditional expression is always true if it is evaluated: terrainGeometryTrait! = Null. MapEditorLogic.cs 35
Let's analyze this snippet. Note that each time the GetOrNull method of the Widget class is used , it is checked for null . However, if Get is used , there is no validation. This is logical - the Get method does not return null :
public T Get<T>(string id) where T : Widget
{
var t = GetOrNull<T>(id);
if (t == null)
throw new InvalidOperationException(....);
return t;
}If the element is not found, then an exception is thrown - this is reasonable behavior. And at the same time, the logical option would be to check the values returned by the GetOrNull method for equality to a null reference.
In the code above, the value returned by the Trait method is checked for null . In fact, inside the Trait method, Get is called from the TraitDictionary class :
public T Trait<T>()
{
return World.TraitDict.Get<T>(this);
}Could it be that this Get behaves differently from the one discussed earlier? Yet the classes are different. Let's check:
public T Get<T>(Actor actor)
{
CheckDestroyed(actor);
return InnerGet<T>().Get(actor);
}The InnerGet method returns an instance of TraitContainer <T> . The implementation of Get in this class is very similar to Get in the Widget class :
public T Get(Actor actor)
{
var result = GetOrDefault(actor);
if (result == null)
throw new InvalidOperationException(....);
return result;
}The main similarity is that null is never returned here either . In case something went wrong, an InvalidOperationException is similarly thrown . Therefore, the Trait method behaves the same way.
Yes, there may be just an extra check here, which does not affect anything. Perhaps it looks strange, but one cannot say that such code will greatly confuse the reader. But if the check is just needed here, then in some cases an exception will be thrown unexpectedly. It is sad.
At this point, it seems more appropriate to call some TraitOrNull . However, there is no such method :). But there is TraitOrDefault , which is analogous to GetOrNullfor this case.
There is another similar point related to the Get method :
public AssetBrowserLogic(....)
{
....
frameSlider = panel.Get<SliderWidget>("FRAME_SLIDER");
if (frameSlider != null)
{
....
}
....
}Analyzer warning : V3022 Expression 'frameSlider! = Null' is always true. AssetBrowserLogic.cs 128
As in the code above , something is wrong here. Either the check is really unnecessary, or instead of Get , you still need to call GetOrNull .
Lost assignment
public SpawnSelectorTooltipLogic(....)
{
....
var textWidth = ownerFont.Measure(labelText).X;
if (textWidth != cachedWidth)
{
label.Bounds.Width = textWidth;
widget.Bounds.Width = 2 * label.Bounds.X + textWidth; // <=
}
widget.Bounds.Width = Math.Max( // <=
teamWidth + 2 * labelMargin,
label.Bounds.Right + labelMargin
);
team.Bounds.Width = widget.Bounds.Width;
....
}Analyzer warning : V3008 The 'widget.Bounds.Width' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 78, 75. SpawnSelectorTooltipLogic.cs 78
It looks like if the textWidth! = CachedWidth condition is true , some case-specific value should be written to widget.Bounds.Width . However, the assignment below, regardless of whether this condition is true, deprives the string
widget.Bounds.Width = 2 * label.Bounds.X + textWidth;every sense. It is likely that they simply forgot to put the else here :
if (textWidth != cachedWidth)
{
label.Bounds.Width = textWidth;
widget.Bounds.Width = 2 * label.Bounds.X + textWidth;
}
else
{
widget.Bounds.Width = Math.Max(
teamWidth + 2 * labelMargin,
label.Bounds.Right + labelMargin
);
}Checking the default value
public void DisguiseAs(Actor target)
{
....
var tooltip = target.TraitsImplementing<ITooltip>().FirstOrDefault();
AsPlayer = tooltip.Owner;
AsActor = target.Info;
AsTooltipInfo = tooltip.TooltipInfo;
....
}Analyzer warning : V3146 Possible null dereference of 'tooltip'. The 'FirstOrDefault' can return default null value. Disguise.cs 192
When is FirstOrDefault commonly used instead of First ? If the selection is empty, then First will throw an InvalidOperationException . FirstOrDefault will not throw an exception, but will return null for the reference type.
In the project, the ITooltip interface is implemented by various classes. Thus, if target.TraitsImplementing <ITooltip> () returns an empty selection, null will be written to tooltip... Further access to the properties of this object will result in a NullReferenceException .
In cases where the developer is sure that the selection will not be empty, it is more correct to use First . If you are not so sure, then it is worth checking the value returned by FirstOrDefault. It is rather strange that this is not here. After all, the values returned by the previously discussed GetOrNull method were always checked. Why didn't they do it here?
Who knows ... Oh, exactly! Surely a developer can answer these questions. In the end, he should edit this code.
Conclusion
OpenRA somehow turned out to be a project that was pleasant and interesting to check out. The developers have done a great job and at the same time did not forget that the source should be easy to learn. Of course, here there are different ... controversial points, but where without them.
At the same time, even with all their efforts, developers (alas) remain human. Some of the considered positives are extremely difficult to notice without using the analyzer. It is sometimes difficult to find an error even immediately after writing it. What can we say about finding them after a long time.
Obviously, it is much better to detect a mistake than its consequences. For this you can spend hours manually rechecking a huge number of new sources. Well, the old ones at the same time look - suddenly did not notice any mistake earlier? Yes, reviews are really useful, but if you have to look through a lot of code, then over time you stop noticing some things. And a lot of time and effort is spent on it.
Static analysis is just a convenient addition to other methods of checking the quality of source code, such as code-review. PVS-Studio will find "simple" (and sometimes not only) errors instead of the developer, allowing people to focus on more serious issues.
Yes, the analyzer sometimes produces false positives and is not able to find all errors at all. But using it will save you a lot of time and nerves. Yes, he is not perfect and sometimes he makes mistakes himself. However, in general, PVS-Studio makes the development process much easier, more pleasant and even (unexpectedly!) Cheaper.
In fact, you do not need to take my word for it - it would be much better to verify the truth of the above yourself. Follow the link to download the analyzer and get a trial key. How much easier?
Well, that's all. Thank you for attention! I wish you clean code and the same clean error log!
If you want to share this article with an English-speaking audience, please use the translation link: Nikita Lipilin. Unicorns break into RTS: analyzing the OpenRA source code .