• Advertisement

PVS-studio team

Member
  • Content count

    2
  • Joined

  • Last visited

Community Reputation

1886 Excellent

About PVS-studio team

  • Rank
    Newbie

Personal Information

Recent Profile Visitors

The recent visitors block is disabled and is not being shown to other users.

Enable
  1. Recently a long-awaited event has happen - Unity Technologies uploaded the C# source code of the game engine, available for free download on Github. The code of the engine and the editor is available. Of course, we couldn't pass up, especially since lately we've not written so many articles about checking projects on C#. Unity allows to use the provided sources only for information purposes. We'll use them exactly in these ways. Let's try out the latest version PVS-Studio 6.23 on the Unity code. Introduction Previously we've written an article about checking Unity. At that time so much C#-code was not available for the analysis: some components, libraries and examples of usage. However, the author of the article managed to find quite interesting bugs. How did Unity please us this time? I'm saying "please" and hope not to offend the authors of the project. Especially since the amount of the source Unity C#-code, presented on GitHub, is about 400 thousand lines (excluding empty) in 2058 files with the extension "cs". It's a lot, and the analyzer had a quite considerable scope. Now about the results. Before the analysis, I've slightly simplified the work, having enabled the mode of the code display according to the CWE classification for the found bugs. I've also activated the warnings suppression mechanism of the third level of certainty (Low). These settings are available in the drop-down menu of PVS-Studio in Visual Studio development environment, and in the parameters of the analyzer. Getting rid of the warnings with low certainty, I made the analysis of the Unity source code. As a result, I got 181 warnings of the first level of certainty (High) and 506 warnings of the second level of certainty (Medium). I have not studied absolutely all the warnings, because there were quite a lot of them. Developers or enthusiasts can easily conduct an in-depth analysis by testing Unity themselves. To do this, PVS-Studio provides free trial and free modes of using. Companies can also buy our product and get quick and detailed support along with the license. Judging by the fact that I immediately managed to find couple of real bugs practically in every group of warnings with one or two attempts, there are a lot of them in Unity. And yes, they are diverse. Let's review the most interesting errors. Results of the check Something's wrong with the flags PVS-Studio warning: V3001 There are identical sub-expressions 'MethodAttributes.Public' to the left and to the right of the '|' operator. SyncListStructProcessor.cs 240 MethodReference GenerateSerialization() { .... MethodDefinition serializeFunc = new MethodDefinition("SerializeItem", MethodAttributes.Public | MethodAttributes.Virtual | MethodAttributes.Public | // <= MethodAttributes.HideBySig, Weaver.voidType); .... } When combining enumeration flags MethodAttributes, an error was made: the Public value was used twice. Perhaps, the reason for this is the wrong code formatting. A similar bug is also made in code of the method GenerateDeserialization: V3001 There are identical sub-expressions 'MethodAttributes.Public' to the left and to the right of the '|' operator. SyncListStructProcessor.cs 309 Copy-Paste PVS-Studio warning: V3001 There are identical sub-expressions 'format == RenderTextureFormat.ARGBFloat' to the left and to the right of the '||' operator. RenderTextureEditor.cs 87 public static bool IsHDRFormat(RenderTextureFormat format) { Return (format == RenderTextureFormat.ARGBHalf || format == RenderTextureFormat.RGB111110Float || format == RenderTextureFormat.RGFloat || format == RenderTextureFormat.ARGBFloat || format == RenderTextureFormat.ARGBFloat || format == RenderTextureFormat.RFloat || format == RenderTextureFormat.RGHalf || format == RenderTextureFormat.RHalf); } I gave a piece of code, preliminary having formatted it, so the error is easily detected visually: the comparison with RenderTextureFormat.ARGBFloat is performed twice. In the original code, it looks differently: Probably, another value of enumeration RenderTextureFormat has to be used in one of two identical comparisons. Double work PVS-Studio warning: V3008 CWE-563 The 'fail' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 1633, 1632. UNetWeaver.cs 1633 class Weaver { .... public static bool fail; .... static public bool IsValidTypeToGenerate(....) { .... if (....) { .... Weaver.fail = true; fail = true; return false; } return true; } .... } The true value is assigned twice to the value, as Weaver.fail and fail is one and the same static field of the Weaver class. Perhaps, there is no crucial error, but the code definitely needs attention. No options PVS-Studio warning: V3009 CWE-393 It's odd that this method always returns one and the same value of 'false'. ProjectBrowser.cs 1417 // Returns true if we should early out of OnGUI bool HandleCommandEventsForTreeView() { .... if (....) { .... if (....) return false; .... } return false; } The method always returns false. Pay attention to the comment in the beginning. A developer forgot about the result PVS-Studio warning: V3010 CWE-252 The return value of function 'Concat' is required to be utilized. AnimationRecording.cs 455 static public UndoPropertyModification[] Process(....) { .... discardedModifications.Concat(discardedRotationModifications); return discardedModifications.ToArray(); } When concatenating two arrays discardedModifications and discardedRotationModifications the author forgot to save the result. Probably a programmer assumed that the result would be expressed immediately in the array discardedModifications. But it is not so. As a result, the original array discardedModifications is returned from the method. The code needs to be corrected as follows: static public UndoPropertyModification[] Process(....) { .... return discardedModifications.Concat(discardedRotationModifications) .ToArray(); } Wrong variable was checked PVS-Studio warning: V3019 CWE-697 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'obj', 'newResolution'. GameViewSizesMenuItemProvider.cs 104 private static GameViewSize CastToGameViewSize(object obj) { GameViewSize newResolution = obj as GameViewSize; if (obj == null) { Debug.LogError("Incorrect input"); return null; } return newResolution; } In this method, the developers forgot to consider a situation where the variable objis not equal to null, but it will not be able to cast to the GameViewSize type. Then the variable newResolution will be set to null, and the debug output will not be made. A correct variant of code will be like this: private static GameViewSize CastToGameViewSize(object obj) { GameViewSize newResolution = obj as GameViewSize; if (newResolution == null) { Debug.LogError("Incorrect input"); } return newResolution; } Deficiency PVS-Studio warning: V3020 CWE-670 An unconditional 'return' within a loop. PolygonCollider2DEditor.cs 96 private void HandleDragAndDrop(Rect targetRect) { .... foreach (....) { .... if (....) { .... } return; } .... } The loop will execute only one iteration, after that the method terminates its work. Various scenarios are probable. For example, return must be inside the unit if, or somewhere before return, a directive continue is missing. It may well be that there is no error here, but then one should make the code more understandable. Unreachable code PVS-Studio warning: V3021 CWE-561 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 CustomScriptAssembly.cs 179 public bool IsCompatibleWith(....) { .... if (buildingForEditor) return IsCompatibleWithEditor(); if (buildingForEditor) buildTarget = BuildTarget.NoTarget; // Editor .... } Two identical checks, following one after another. It is clear that in case of buildingForEditor equality to the true value, the second check is meaningless, because the first method terminates its work. If the value buildingForEditor is false, neither then-brunch nor if operator will be executed. There is an erroneous construction that requires correction. Unconditional condition PVS-Studio warning: V3022 CWE-570 Expression 'index < 0 && index >= parameters.Length' is always false. AnimatorControllerPlayable.bindings.cs 287 public AnimatorControllerParameter GetParameter(int index) { AnimatorControllerParameter[] param = parameters; if (index < 0 && index >= parameters.Length) throw new IndexOutOfRangeException( "Index must be between 0 and " + parameters.Length); return param[index]; } The condition of the index check is incorrect - the result will always be false. However, in case of passing the incorrect index to the GetParameter method, the exception IndexOutOfRangeException will still be thrown when attempting to access an array element in the return block. Although, the error message will be slightly different. One has to use || in a condition instead of the operator && so that the code worked the way a developer expected: public AnimatorControllerParameter GetParameter(int index) { AnimatorControllerParameter[] param = parameters; if (index < 0 || index >= parameters.Length) throw new IndexOutOfRangeException( "Index must be between 0 and " + parameters.Length); return param[index]; } Perhaps, due to the use of the Copy-Paste method, there is another the same error in the Unity code: PVS-Studio warning: V3022 CWE-570 Expression 'index < 0 && index >= parameters.Length' is always false. Animator.bindings.cs 711 And another similar error associated with the incorrect condition of the check of the array index: PVS-Studio warning: V3022 CWE-570 Expression 'handle.valueIndex < 0 && handle.valueIndex >= list.Length' is always false. StyleSheet.cs 81 static T CheckAccess<T>(T[] list, StyleValueType type, StyleValueHandle handle) { T value = default(T); if (handle.valueType != type) { Debug.LogErrorFormat(.... ); } else if (handle.valueIndex < 0 && handle.valueIndex >= list.Length) { Debug.LogError("Accessing invalid property"); } else { value = list[handle.valueIndex]; } return value; } And in this case, a release of the IndexOutOfRangeException exception is possible.As in the previous code fragments, one has to use the operator || instead of && to fix an error. Simply strange code Two warnings are issued for the code fragment below. PVS-Studio warning: V3022 CWE-571 Expression 'bRegisterAllDefinitions || (AudioSettings.GetSpatializerPluginName() == "GVR Audio Spatializer")' is always true. AudioExtensions.cs 463 PVS-Studio warning: V3022 CWE-571 Expression 'bRegisterAllDefinitions || (AudioSettings.GetAmbisonicDecoderPluginName() == "GVR Audio Spatializer")' is always true. AudioExtensions.cs 467 // This is where we register our built-in spatializer extensions. static private void RegisterBuiltinDefinitions() { bool bRegisterAllDefinitions = true; if (!m_BuiltinDefinitionsRegistered) { if (bRegisterAllDefinitions || (AudioSettings.GetSpatializerPluginName() == "GVR Audio Spatializer")) { } if (bRegisterAllDefinitions || (AudioSettings.GetAmbisonicDecoderPluginName() == "GVR Audio Spatializer")) { } m_BuiltinDefinitionsRegistered = true; } } It looks like an incomplete method. It is unclear why it has been left as such and why developers haven't commented the useless code blocks. All, that the method does at the moment: if (!m_BuiltinDefinitionsRegistered) { m_BuiltinDefinitionsRegistered = true; } Useless method PVS-Studio warning: V3022 CWE-570 Expression 'PerceptionRemotingPlugin.GetConnectionState() != HolographicStreamerConnectionState.Disconnected' is always false. HolographicEmulationWindow.cs 171 private void Disconnect() { if (PerceptionRemotingPlugin.GetConnectionState() != HolographicStreamerConnectionState.Disconnected) PerceptionRemotingPlugin.Disconnect(); } To clarify the situation, it is necessary to look at the declaration of the methodPerceptionRemotingPlugin.GetConnectionState(): internal static HolographicStreamerConnectionState GetConnectionState() { return HolographicStreamerConnectionState.Disconnected; } Thus, calling the Disconnect() method leads to nothing. One more error relates to the same method PerceptionRemotingPlugin.GetConnectionState(): PVS-Studio warning: V3022 CWE-570 Expression 'PerceptionRemotingPlugin.GetConnectionState() == HolographicStreamerConnectionState.Connected' is always false. HolographicEmulationWindow.cs 177 private bool IsConnectedToRemoteDevice() { return PerceptionRemotingPlugin.GetConnectionState() == HolographicStreamerConnectionState.Connected; } The result of the method is equivalent to the following: private bool IsConnectedToRemoteDevice() { return false; } As we can see, among the warnings V3022 many interesting ones were found. Probably, if one spends much time, he can increase the list. But let's move on. Not on the format PVS-Studio warning: V3025 CWE-685 Incorrect format. A different number of format items is expected while calling 'Format' function. Arguments not used: index. Physics2D.bindings.cs 2823 public void SetPath(....) { if (index < 0) throw new ArgumentOutOfRangeException( String.Format("Negative path index is invalid.", index)); .... } There is no error in code, but as the saying goes, the code "smells". Probably, an earlier message was more informative, like this: "Negative path index {0} is invalid.". Then it was simplified, but developers forgot to remove the parameter index for the method Format. Of course, this is not the same as a forgotten parameter for the indicated output string specifier, i.e. the construction of the type String.Format("Negative path index {0} is invalid."). In such a case, an exception would be thrown. But in our case we also need neatness when refactoring. The code has to be fixed as follows: public void SetPath(....) { if (index < 0) throw new ArgumentOutOfRangeException( "Negative path index is invalid."); .... } Substring of the substring PVS-Studio warning: V3053 An excessive expression. Examine the substrings 'UnityEngine.' and 'UnityEngine.SetupCoroutine'. StackTrace.cs 43 static bool IsSystemStacktraceType(object name) { string casted = (string)name; return casted.StartsWith("UnityEditor.") || casted.StartsWith("UnityEngine.") || casted.StartsWith("System.") || casted.StartsWith("UnityScript.Lang.") || casted.StartsWith("Boo.Lang.") || casted.StartsWith("UnityEngine.SetupCoroutine"); } Search of the substring "UnityEngine.SetupCoroutine" in the condition is meaningless, because before that the search for "UnityEngine." is performed. Therefore, the last check should be removed or one has to clarify the correctness of substrings. Another similar error: PVS-Studio warning: V3053 An excessive expression. Examine the substrings 'Windows.dll' and 'Windows.'. AssemblyHelper.cs 84 static private bool CouldBelongToDotNetOrWindowsRuntime(string assemblyPath) { return assemblyPath.IndexOf("mscorlib.dll") != -1 || assemblyPath.IndexOf("System.") != -1 || assemblyPath.IndexOf("Windows.dll") != -1 || // <= assemblyPath.IndexOf("Microsoft.") != -1 || assemblyPath.IndexOf("Windows.") != -1 || // <= assemblyPath.IndexOf("WinRTLegacy.dll") != -1 || assemblyPath.IndexOf("platform.dll") != -1; } Size does matter PVS-Studio warning: V3063 CWE-571 A part of conditional expression is always true if it is evaluated: pageSize <= 1000. UNETInterface.cs 584 public override bool IsValid() { .... return base.IsValid() && (pageSize >= 1 || pageSize <= 1000) && totalFilters <= 10; } Condition for a check of a valid page size is erroneous. Instead of the operator ||, one has to use &&. The corrected code: public override bool IsValid() { .... return base.IsValid() && (pageSize >= 1 && pageSize <= 1000) && totalFilters <= 10; } Possible division by zero PVS-Studio warning: V3064 CWE-369 Potential division by zero. Consider inspecting denominator '(float)(width - 1)'. ClothInspector.cs 249 Texture2D GenerateColorTexture(int width) { .... for (int i = 0; i < width; i++) colors[i] = GetGradientColor(i / (float)(width - 1)); .... } The problem may occur when passing the value width = 1 into the method. In the method, it is not checked anyway. The method GenerateColorTexture is called in the code just once with the parameter 100: void OnEnable() { if (s_ColorTexture == null) s_ColorTexture = GenerateColorTexture(100); .... } So, there is no error here so far. But, just in case, in the method GenerateColorTexture the possibility of transferring incorrect width value should be provided. Paradoxical check PVS-Studio warning: V3080 CWE-476 Possible null dereference. Consider inspecting 'm_Parent'. EditorWindow.cs 449 public void ShowPopup() { if (m_Parent == null) { .... Rect r = m_Parent.borderSize.Add(....); .... } } Probably, due to a typo, the execution of such code guarantees the use of the null reference m_Parent. The corrected code: public void ShowPopup() { if (m_Parent != null) { .... Rect r = m_Parent.borderSize.Add(....); .... } } The same error occurs later in the code: PVS-Studio warning: V3080 CWE-476 Possible null dereference. Consider inspecting 'm_Parent'. EditorWindow.cs 470 internal void ShowWithMode(ShowMode mode) { if (m_Parent == null) { .... Rect r = m_Parent.borderSize.Add(....); .... } And here's another interesting bug that can lead to access by a null reference due to incorrect check: PVS-Studio warning: V3080 CWE-476 Possible null dereference. Consider inspecting 'objects'. TypeSelectionList.cs 48 public TypeSelection(string typeName, Object[] objects) { System.Diagnostics.Debug.Assert(objects != null || objects.Length >= 1); .... } It seems to me that Unity developers quite often make errors related to misuse of operators || and && in conditions. In this case, if objects has a null value, then this will lead to a check of second part of the condition (objects != null || objects.Length >= 1), which will entail the unexpected throw of an exception. The error should be corrected as follows: public TypeSelection(string typeName, Object[] objects) { System.Diagnostics.Debug.Assert(objects != null && objects.Length >= 1); .... } Early nullifying PVS-Studio warning: V3080 CWE-476 Possible null dereference. Consider inspecting 'm_RowRects'. TreeViewControlGUI.cs 272 public override void GetFirstAndLastRowVisible(....) { .... if (rowCount != m_RowRects.Count) { m_RowRects = null; throw new InvalidOperationException(string.Format("....", rowCount, m_RowRects.Count)); } .... } In this case, the exception throw (access by the null reference m_RowRects) will happen when generating the message string for another exception. Code might be fixed, for example, as follows: public override void GetFirstAndLastRowVisible(....) { .... if (rowCount != m_RowRects.Count) { var m_RowRectsCount = m_RowRects.Count; m_RowRects = null; throw new InvalidOperationException(string.Format("....", rowCount, m_RowRectsCount)); } .... } One more error when checking PVS-Studio warning: V3080 CWE-476 Possible null dereference. Consider inspecting 'additionalOptions'. MonoCrossCompile.cs 279 static void CrossCompileAOT(....) { .... if (additionalOptions != null & additionalOptions.Trim().Length > 0) arguments += additionalOptions.Trim() + ","; .... } Due to the fact that the & operator is used in a condition, the second part of the condition will always be checked, regardless of the result of the check of the first part. In case if the variable additionalOptions has the null value, the exception throw is inevitable. The error has to be corrected, by using the operator && instead of &. As we can see, among the warnings with the number V3080 there are rather insidious errors. Late check PVS-Studio warning: V3095 CWE-476 The 'element' object was used before it was verified against null. Check lines: 101, 107. StyleContext.cs 101 public override void OnBeginElementTest(VisualElement element, ....) { if (element.IsDirty(ChangeType.Styles)) { .... } if (element != null && element.styleSheets != null) { .... } .... } The variable element is used without preliminary check for null. While later in the code this check is performed. The code probably needs to be corrected as follows: public override void OnBeginElementTest(VisualElement element, ....) { if (element != null) { if (element.IsDirty(ChangeType.Styles)) { .... } if (element.styleSheets != null) { .... } } .... } In code there are 18 more errors. Let me give you a list of the first 10: V3095 CWE-476 The 'property' object was used before it was verified against null. Check lines: 5137, 5154. EditorGUI.cs 5137 V3095 CWE-476 The 'exposedPropertyTable' object was used before it was verified against null. Check lines: 152, 154. ExposedReferenceDrawer.cs 152 V3095 CWE-476 The 'rectObjs' object was used before it was verified against null. Check lines: 97, 99. RectSelection.cs 97 V3095 CWE-476 The 'm_EditorCache' object was used before it was verified against null. Check lines: 134, 140. EditorCache.cs 134 V3095 CWE-476 The 'setup' object was used before it was verified against null. Check lines: 43, 47. TreeViewExpandAnimator.cs 43 V3095 CWE-476 The 'response.job' object was used before it was verified against null. Check lines: 88, 99. AssetStoreClient.cs 88 V3095 CWE-476 The 'compilationTask' object was used before it was verified against null. Check lines: 1010, 1011. EditorCompilation.cs 1010 V3095 CWE-476 The 'm_GenericPresetLibraryInspector' object was used before it was verified against null. Check lines: 35, 36. CurvePresetLibraryInspector.cs 35 V3095 CWE-476 The 'Event.current' object was used before it was verified against null. Check lines: 574, 620. AvatarMaskInspector.cs 574 V3095 CWE-476 The 'm_GenericPresetLibraryInspector' object was used before it was verified against null. Check lines: 31, 32. ColorPresetLibraryInspector.cs 31 Wrong Equals method PVS-Studio warning: V3115 CWE-684 Passing 'null' to 'Equals' method should not result in 'NullReferenceException'. CurveEditorSelection.cs 74 public override bool Equals(object _other) { CurveSelection other = (CurveSelection)_other; return other.curveID == curveID && other.key == key && other.type == type; } Overload of the Equals method was implemented carelessly. One has to take into account the possibility of obtaining null as a parameter, as this can lead to a throw of an exception, which hasn't been considered in the calling code. In addition, the situation, when _other can't be cast to the type CurveSelection, will lead to a throw of an exception. The code has to be fixed. A good example of the implementation of Object.equals overload is given in the documentation. In the code, there are other similar errors: V3115 CWE-684 Passing 'null' to 'Equals' method should not result in 'NullReferenceException'. SpritePackerWindow.cs 40 V3115 CWE-684 Passing 'null' to 'Equals' method should not result in 'NullReferenceException'. PlatformIconField.cs 28 V3115 CWE-684 Passing 'null' to 'Equals' method should not result in 'NullReferenceException'. ShapeEditor.cs 161 V3115 CWE-684 Passing 'null' to 'Equals' method should not result in 'NullReferenceException'. ActiveEditorTrackerBindings.gen.cs 33 V3115 CWE-684 Passing 'null' to 'Equals' method should not result in 'NullReferenceException'. ProfilerFrameDataView.bindings.cs 60 Once again about the check for null inequality PVS-Studio warning: V3125 CWE-476 The 'camera' object was used after it was verified against null. Check lines: 184, 180. ARBackgroundRenderer.cs 184 protected void DisableARBackgroundRendering() { .... if (camera != null) camera.clearFlags = m_CameraClearFlags; // Command buffer camera.RemoveCommandBuffer(CameraEvent.BeforeForwardOpaque, m_CommandBuffer); camera.RemoveCommandBuffer(CameraEvent.BeforeGBuffer, m_CommandBuffer); } When the camera variable is used the first time, it is checked for null inequality. But further along the code the developers forget to do it. The correct variant could be like this: protected void DisableARBackgroundRendering() { .... if (camera != null) { camera.clearFlags = m_CameraClearFlags; // Command buffer camera.RemoveCommandBuffer(CameraEvent.BeforeForwardOpaque, m_CommandBuffer); camera.RemoveCommandBuffer(CameraEvent.BeforeGBuffer, m_CommandBuffer); } } Another similar error: PVS-Studio warning: V3125 CWE-476 The 'item' object was used after it was verified against null. Check lines: 88, 85. TreeViewForAudioMixerGroups.cs 88 protected override Texture GetIconForItem(TreeViewItem item) { if (item != null && item.icon != null) return item.icon; if (item.id == kNoneItemID) // <= return k_AudioListenerIcon; return k_AudioGroupIcon; } An error, that in some cases leads to an access by a null link. The execution of the condition in the first block if enables the exit from the method. However, if this does not happen, then there is no guarantee that the reference item is non-zero. Here is the corrected version of the code: protected override Texture GetIconForItem(TreeViewItem item) { if (item != null) { if (item.icon != null) return item.icon; if (item.id == kNoneItemID) return k_AudioListenerIcon; } return k_AudioGroupIcon; } In the code there are 12 similar errors. Let me give you a list of the first 10: V3125 CWE-476 The 'element' object was used after it was verified against null. Check lines: 132, 107. StyleContext.cs 132 V3125 CWE-476 The 'mi.DeclaringType' object was used after it was verified against null. Check lines: 68, 49. AttributeHelper.cs 68 V3125 CWE-476 The 'label' object was used after it was verified against null. Check lines: 5016, 4999. EditorGUI.cs 5016 V3125 CWE-476 The 'Event.current' object was used after it was verified against null. Check lines: 277, 268. HostView.cs 277 V3125 CWE-476 The 'bpst' object was used after it was verified against null. Check lines: 96, 92. BuildPlayerSceneTreeView.cs 96 V3125 CWE-476 The 'state' object was used after it was verified against null. Check lines: 417, 404. EditorGUIExt.cs 417 V3125 CWE-476 The 'dock' object was used after it was verified against null. Check lines: 370, 365. WindowLayout.cs 370 V3125 CWE-476 The 'info' object was used after it was verified against null. Check lines: 234, 226. AssetStoreAssetInspector.cs 234 V3125 CWE-476 The 'platformProvider' object was used after it was verified against null. Check lines: 262, 222. CodeStrippingUtils.cs 262 V3125 CWE-476 The 'm_ControlPoints' object was used after it was verified against null. Check lines: 373, 361. EdgeControl.cs 373 The choice turned out to be small PVS-Studio warning: V3136 CWE-691 Constant expression in switch statement. HolographicEmulationWindow.cs 261 void ConnectionStateGUI() { .... HolographicStreamerConnectionState connectionState = PerceptionRemotingPlugin.GetConnectionState(); switch (connectionState) { .... } .... } The method PerceptionRemotingPlugin.GetConnectionState() is to blame here. We have already come across it when we were analyzing the warnings V3022: internal static HolographicStreamerConnectionState GetConnectionState() { return HolographicStreamerConnectionState.Disconnected; } The method will return a constant. This code is very strange. It needs to be paid attention. Conclusions I think we can stop at this point, otherwise the article will become boring and overextended. Again, I listed the errors that I just couldn't miss. Sure, the Unity code contains a big number of the erroneous and incorrect constructions, that need to be fixed. The difficulty is that many of the issued warnings are very controversial and only the author of the code is able to make the exact "diagnosis" in each case. Generally speaking about the Unity project, we can say that it is rich for errors, but taking into account the size of its code base (400 thousand lines), it's not so bad. Nevertheless, I hope that the authors will not neglect the code analysis tools to improve the quality of their product. Use PVS-Studio and I wish you bugless code!
  2. Introduction C(ontinued)MaNGOS is an actively developing offshoot of an old project: MaNGOS (Massive Network Game Object Server), which was made in order to create an alternative server for the World of Warcraft game. The majority of MaNGOS developers continue working in CMaNGOS. According to the developers themselves, their goal is to create a "well written server in C++" for one of the best MMORPGs. I'll try to help them with this, by checking CMaNGOS using the static analyzer, PVS-Studio. Note: To do the analysis we used CMaNGOS-Classic server, available in the project repository on GitHub. The analysis results An error in the operation precedence PVS-Studio warning: V593 Consider reviewing the expression of the 'A = B < C' kind. The expression is calculated as following: 'A = (B < C)'. SpellEffects.cpp 473 void Spell::EffectDummy(SpellEffectIndex eff_idx) { .... if (uint32 roll = urand(0, 99) < 3) // getRace()) == ALLIANCE) modelid = 2428; // Id == 28891 && m_spellInfo->Id == 28898) // Id is verified against two different values at the same time. The result of this check is always false, of course. The author most likely made a mistake and instead of '||' operator used '&&'. It should be noted that the program commented on strange code behavior, and perhaps, it was caused by this error exactly. There were several errors like this, here is the full list: V547 Expression is always false. Probably the '||' operator should be used here. SpellEffects.cpp 2872 V547 Expression is always true. Probably the '&&' operator should be used here. genrevision.cpp 261 V547 Expression is always true. Probably the '&&' operator should be used here. vmapexport.cpp 361 V547 Expression is always true. Probably the '&&' operator should be used here. MapTree.cpp 125 V547 Expression is always true. Probably the '&&' operator should be used here. MapTree.cpp 234 Suspicious formatting PVS-Studio warning: V640 The code's operational logic does not correspond with its formatting. The statement is indented to the right, but it is always executed. It is possible that curly brackets are missing. instance_blackrock_depths.cpp 111 void instance_blackrock_depths::OnCreatureCreate(Creature* pCreature) { switch (pCreature->GetEntry()) { .... case NPC_HAMMERED_PATRON: .... if (m_auiEncounter[11] == DONE) pCreature->SetFactionTemporary(....); pCreature->SetStandState(UNIT_STAND_STATE_STAND); // SetStandState(UNIT_STAND_STATE_STAND) to be executed regardless of the if condition. If this behavior was meant intentionally, then the formatting should be corrected: if (m_auiEncounter[11] == DONE) pCreature->SetFactionTemporary(....); pCreature->SetStandState(UNIT_STAND_STATE_STAND); Similar operands in the ternary operator PVS-Studio warning: V583 The '?:' operator, regardless of its conditional expression, always returns one and the same value: SAY_BELNISTRASZ_AGGRO_1. razorfen_downs.cpp 104 void AttackedBy(Unit* pAttacker) override { .... if (!m_bAggro) { DoScriptText(urand(0, 1) ? SAY_BELNISTRASZ_AGGRO_1 : // GetHealth() / pEmberstrife->GetMaxHealth() > 0.1f) // UpdateDuration(this, time); .... } Testing this for null PVS-Studio warning: V704 '!this ||!pVictim' expression should be avoided: 'this' pointer can never be NULL on newer compilers. Unit.cpp 1417 void Unit::CalculateSpellDamage(....) { .... if (!this || !pVictim) // GetEntry() == NPC_VEKLOR ? NPC_VEKNILASH : NPC_VEKLOR)) { float fHealPercent = ((float)uiHealedAmount) / ((float)m_creature->GetMaxHealth()); uint32 uiTwinHeal = (uint32)(fHealPercent * ((float)pTwin->GetMaxHealth())); uint32 uiTwinHealth = pTwin->GetHealth() + uiTwinHeal; pTwin->SetHealth(uiTwinHealth < pTwin->GetMaxHealth() ? uiTwinHealth : pTwin->GetMaxHealth()); } } The variable uiHealedAmount is passed by reference, but is not changed in the body of the function. This can be misleading, because we get the impression that the HealedBy() function writes something to the uiHealedAmount. It would be better to pass the variable by a constant reference or by value. Repeated assignment PVS-Studio warning: V519 The 'stat' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 1776, 1781. DetourNavMeshQuery.cpp 1781 dtStatus dtNavMeshQuery::findStraightPath(....) const { .... if (....) { stat = appendPortals(apexIndex, i, closestEndPos, // loadMap(mapID, tileX, tileY, meshData); // get model data m_terrainBuilder->loadVMap(mapID, tileY, tileX, //
  3. Introduction CryEngine is a game engine created by the German company Crytek in the year 2002, and originally used in the first-person shooter Far Cry. There are a lot of great games made on the basis of different versions of CryEngine, by many studios who have licensed the engine: Far Cry, Crysis, Entropia Universe, Blue Mars, Warface, Homefront: The Revolution, Sniper: Ghost Warrior, Armored Warfare, Evolve and many others. In March 2016 the Crytek company announced the release of the new CryEngine V, and soon after, posted the source code on GitHub. To perform the source code analysis, we used the PVS-Studio for Linux. Now, it has become even more convenient for the developers of cross-platform projects to track the quality of their code, with one static analysis tool. The Linux version can be downloaded as an archive, or a package for a package manager. You can set up the installation and update for the majority of distributions, using our repository. This article only covers the general analysis warnings, and only the "High" certainty level (there are also "Medium" and "Low"). To be honest, I didn't even examine all of the "High" level warnings, because there was already enough material for an article after even a quick look. I started working on the article several times over a period of a few months, so I can say with certainty that the bugs described here have living in the code for some months already. Some of the bugs that had been found during the previous check of the project, also weren't fixed. It was very easy to download and check the source code in Linux. Here is a list of all necessary commands: mkdir ~/projects && cd ~/projects git clone https://github.com/CRYTEK/CRYENGINE.git cd CRYENGINE/ git checkout main chmod +x ./download_sdks.py ./download_sdks.py pvs-studio-analyzer trace -- \ sh ./cry_waf.sh build_linux_x64_clang_profile -p gamesdk pvs-studio-analyzer analyze \ -l /path/to/PVS-Studio.lic \ -o ~/projects/CRYENGINE/cryengine.log \ -r ~/projects/CRYENGINE/ \ -C clang++-3.8 -C clang-3.8 \ -e ~/projects/CRYENGINE/Code/SDKs \ -j4 plog-converter -a GA:1,2 -t tasklist \ -o ~/projects/CRYENGINE/cryengine_ga.tasks \ ~/projects/CRYENGINE/cryengine.log The report file cryengine_ga.tasks can be opened and viewed in QtCreator. What did we manage to find in the source code of CryEngine V? A strange Active() function V501 There are identical sub-expressions to the left and to the right of the '==' operator: bActive == bActive LightEntity.h 124 void SetActive(bool bActive) { if (bActive == bActive) return; m_bActive = bActive; OnResetState(); } The function does nothing because of a typo. It seems to me that if there was a contest, "Super Typo", this code fragment would definitely take first place. I think this error has every chance to get into the section, "C/C++ bugs of the month". But that's not all, here is a function from another class: V501 There are identical sub-expressions 'm_staticObjects' to the left and to the right of the '||' operator. FeatureCollision.h 66 class CFeatureCollision : public CParticleFeature { public: CRY_PFX2_DECLARE_FEATURE public: CFeatureCollision(); .... bool IsActive() const { return m_terrain || m_staticObjects || m_staticObjects; } .... bool m_terrain; bool m_staticObjects; bool m_dynamicObjects; }; The variable m_staticObjects is used twice in the function IsActive(), although there is an unused variable m_dynamicObjects. Perhaps, it was this variable that was meant to be used. Code above has no bugs V547 Expression 'outArrIndices < 0' is always false. Unsigned type value is never < 0. CGFLoader.cpp 881 static bool CompactBoneVertices(...., DynArray& outArrIndices, ....) // begin(), &meshData), (uint8)std::distance(meshData....->....begin(), &chunk), (uint8)std::distance(meshData.m_instances.begin(), &instance) }; memcpy(hashableData, pCREGeomCache, sizeof(pCREGeomCache)); .... } Pay attention to the arguments of the memcpy() function. The programmer plans to copy the object pCREGeomCache to the array hashableData, but he accidentally copies not the size of the object, but the size of the pointer using the sizeof operator. Due to the error, the object is not copied completely, only 4 or 8 bytes. V568 It's odd that 'sizeof()' operator evaluates the size of a pointer to a class, but not the size of the 'this' class object. ClipVolumeManager.cpp 145 void CClipVolumeManager::GetMemoryUsage(class ICrySizer* pSizer) const { pSizer->AddObject(this, sizeof(this)); for (size_t i = 0; i < m_ClipVolumes.size(); ++i) pSizer->AddObject(m_ClipVolumes.m_pVolume); } A similar mistake was made when the programmer evaluated the size of this pointer instead of the size of a class. Correct variant: sizeof(*this). V530 The return value of function 'release' is required to be utilized. ClipVolumes.cpp 492 vector m_jitteredDepthPassArray; void CClipVolumesStage::PrepareVolumetricFog() { .... for (int32 i = 0; i < m_jitteredDepthPassArray.size(); ++i) { m_jitteredDepthPassArray.release(); } m_jitteredDepthPassArray.resize(depth); for (int32 i = 0; i < depth; ++i) { m_jitteredDepthPassArray = CryMakeUnique(); m_jitteredDepthPassArray->SetViewport(viewport); m_jitteredDepthPassArray->SetFlags(....); } .... } If we look at the documentation for the class std::unique_ptr, the release() function should be used as follows: std::unique_ptr up(new Foo()); Foo* fp = up.release(); delete fp; Most likely, it was meant to use the reset() function instead of the release() one. V549 The first argument of 'memcpy' function is equal to the second argument. ObjectsTree_Serialize.cpp 1135 void COctreeNode::LoadSingleObject(....) { .... float* pAuxDataDst = pObj->GetAuxSerializationDataPtr(....); const float* pAuxDataSrc = StepData(....); memcpy(pAuxDataDst, pAuxDataDst, min(....) * sizeof(float)); .... } It was forgotten, to pass pAuxDataSrc to the memcpy() function. Instead of this, the same variable pAuxDataDst is used as both source and destination. No one is immune to errors. By the way, those who are willing, may test their programming skills and attentiveness, by doing a quiz on the detection of similar bugs: q.viva64.com. Strange code V501 There are identical sub-expressions to the left and to the right of the '||' operator: val == 0 || val == - 0 XMLCPB_AttrWriter.cpp 363 void CAttrWriter::PackFloatInSemiConstType(float val, ....) { uint32 type = PFSC_VAL; if (val == 0 || val == -0) // pMesh[0]=pmd->pMesh[1] = this; AddRef();AddRef(); for(pmd0=m_pMeshUpdate; pmd0->next; pmd0=pmd0->next); // next = pmd; .... } One more code fragment that remained uncorrected since the last project check. But it is still unclear if this is a formatting error, or a mistake in logic. About pointers V522 Dereferencing of the null pointer 'pCEntity' might take place. BreakableManager.cpp 2396 int CBreakableManager::HandlePhysics_UpdateMeshEvent(....) { CEntity* pCEntity = 0; .... if (pmu && pSrcStatObj && GetSurfaceType(pSrcStatObj)) { .... if (pEffect) { .... if (normal.len2() > 0) pEffect->Spawn(true, pCEntity->GetSlotWorldTM(...); // GetPhysicalProxy()) return 1; } .... } The analyzer detected null pointer dereference. The code of the function is written or refactored in such a way that there is now a branch of code, where the pointer pCEntity will be, initialized by a zero. Now let's have a look at the variant of a potential dereference of a null pointer. V595 The 'pTrack' pointer was utilized before it was verified against nullptr. Check lines: 60, 61. AudioNode.cpp 60 void CAudioNode::Animate(SAnimContext& animContext) { .... const bool bMuted = gEnv->IsEditor() && (pTrack->GetFlags() & IAnimTrack::eAnimTrackFlags_Muted); if (!pTrack || pTrack->GetNumKeys() == 0 || pTrack->GetFlags() & IAnimTrack::eAnimTrackFlags_Disabled) { continue; } .... } The author of this code first used the pointer pTrack, but its validity is checked on the next line of code before the dereference. Most likely, this is not how the program should work. There were a lot of V595 warnings, they won't really fit into the article. Very often, such code is a real error, but thanks to luck, the code works correctly. V571 Recurring check. The 'if (rLightInfo.m_pDynTexture)' condition was already verified in line 69. ObjMan.cpp 70 // Safe memory helpers #define SAFE_RELEASE(p){ if (p) { (p)->Release(); (p) = NULL; } } void CObjManager::UnloadVegetationModels(bool bDeleteAll) { .... SVegetationSpriteLightInfo& rLightInfo = ....; if (rLightInfo.m_pDynTexture) SAFE_RELEASE(rLightInfo.m_pDynTexture); .... } In this fragment there is no serious error, but it is not necessary to write extra code, if the corresponding checks are already included in the special macro. One more fragment with redundant code: V571 Recurring check. The 'if (m_pSectorGroups)' condition was already verified in line 48. PartitionGrid.cpp 50 V575 The 'memcpy' function doesn't copy the whole string. Use 'strcpy / strcpy_s' function to preserve terminal null. SystemInit.cpp 4045 class CLvlRes_finalstep : public CLvlRes_base { .... for (;; ) { if (*p == '/' || *p == '\\' || *p == 0) { char cOldChar = *p; *p = 0; // create zero termination _finddata_t fd; bool bOk = FindFile(szFilePath, szFile, fd); if (bOk) assert(strlen(szFile) == strlen(fd.name)); *p = cOldChar; // get back the old separator if (!bOk) return; memcpy((void*)szFile, fd.name, strlen(fd.name)); //
  4. PVS-studio team

    Anomalies in X-Ray Engine

    The X-Ray Engine is a game engine, used in the S.T.A.L.K.E.R. game series. Its code was made public in September 16 2014, and since then, STALKER fans continue its development. A large project size, and a huge number of bugs in the games, gives us a wonderful chance to show what PVS-Studio is capable of. Introduction X-Ray was created by a Ukrainian company, GSC GameWorld, for the game S.T.A.L.K.E.R.: Shadow of Chernobyl. This engine has a renderer supporting DirectX 8.1/9.0c/10/10.1/11, physical and sound engines, multiplayer, and an artificial intelligence system - A-Life. Later, the company was about to create a 2.0 version for their new game, but development was discontinued, and the source code was put out for public access. This project is easily built with all of its dependencies in Visual Studio 2015. To do the analysis we used the engine source code 1.6v, from a repository on GitHub and PVS-Studio 6.05 static code analyze, which can be downloaded from this link. Copy-paste Let's start with errors related to copying code. The way they get to the code is usually the same: the code was copied, parts of variables were changed, and some remained forgotten. Such errors can quickly spread in the code base, and are very easy to overlook without a static code analyzer. MxMatrix& MxQuadric::homogeneous(MxMatrix& H) const { .... unsigned int i, j; for(i=0; ir_string(ppi_section,"color_base"), "%f,%f,%f", &data.m_attack_effector.ppi.color_base.r, &data.m_attack_effector.ppi.color_base.g, &data.m_attack_effector.ppi.color_base.b); if (ini->line_exist(ppi_section,"color_base")) sscanf(ini->r_string(ppi_section,"color_gray"), "%f,%f,%f", &data.m_attack_effector.ppi.color_gray.r, &data.m_attack_effector.ppi.color_gray.g, &data.m_attack_effector.ppi.color_gray.b); if (ini->line_exist(ppi_section,"color_base")) sscanf(ini->r_string(ppi_section,"color_add"), "%f,%f,%f", &data.m_attack_effector.ppi.color_add.r, &data.m_attack_effector.ppi.color_add.g, &data.m_attack_effector.ppi.color_add.b); .... } PVS-Studio warnings: V581 The conditional expressions of the 'if' operators situated alongside each other are identical. Check lines: 445, 447. base_monster_startup.cpp 447 V581 The conditional expressions of the 'if' operators situated alongside each other are identical. Check lines: 447, 449. base_monster_startup.cpp 449 In this fragment we see several conditional expressions in a row. Obviously, we need to replace the color_base with color_gray and color_add according to the code in the if branch. /* process a single statement */ static void ProcessStatement(char *buff, int len) { .... if (strncmp(buff,"\\pauthr\\",8) == 0) { ProcessPlayerAuth(buff, len); } else if (strncmp(buff,"\\getpidr\\",9) == 0) { ProcessGetPid(buff, len); } else if (strncmp(buff,"\\getpidr\\",9) == 0) { ProcessGetPid(buff, len); } else if (strncmp(buff,"\\getpdr\\",8) == 0) { ProcessGetData(buff, len); } else if (strncmp(buff,"\\setpdr\\",8) == 0) { ProcessSetData(buff, len); } } PVS-Studio warning: V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 1502, 1505. gstats.c 1502 As in the previous example, two similar conditions are used here (strncmp(buff,"\\getpidr\\",9) == 0). It's hard to say for sure whether this is a mistake, or simply unreachable code, but it's worth revising for sure. Perhaps we should have blocks with getpidr/setpidr by analogy with getpdr/setpdr. class RGBAMipMappedCubeMap { .... size_t height() const { return cubeFaces[0].height(); } size_t width() const { return cubeFaces[0].height(); } .... }; PVS-Studio warning: V524 It is odd that the body of 'width' function is fully equivalent to the body of 'height' function. tpixel.h 1090 Methods height() and width() have the same body. Bearing in mind that we evaluate faces of a cube here, perhaps there is no error. But it's better to rewrite the method width() in the following way: size_t width() const { return cubeFaces[0].width(); } Improper use of C++ C++ is a wonderful language that provides the programmer with many a possibility... to shoot yourself in the foot in a multitude of the cruelest ways. Undefined behavior, memory leaks, and of course, typos. And that's what will be discussed in this section. template struct _matrix33 { public: typedef _matrix33Self; typedef Self& SelfRef; .... IC SelfRef sMTxV(Tvector& R, float s1, const Tvector& V1) const { R.x = s1*(m[0][0] * V1.x + m[1][0] * V1.y + m[2][0] * V1.z); R.y = s1*(m[0][1] * V1.x + m[1][1] * V1.y + m[2][1] * V1.z); R.z = s1*(m[0][2] * V1.x + m[1][2] * V1.y + m[2][2] * V1.z); } .... } PVS-Studio warning: V591 Non-void function should return a value. _matrix33.h 435 At the end of the method there is no return *this. According to the standard, it will lead to undefined behavior. As the return value is a reference, it will probably lead to a program crash, upon attempting to access the return value. ETOOLS_API int __stdcall ogg_enc(....) { .... FILE *in, *out = NULL; .... input_format *format; .... in = fopen(in_fn, "rb"); if(in == NULL) return 0; format = open_audio_file(in, &enc_opts); if(!format){ fclose(in); return 0; }; out = fopen(out_fn, "wb"); if(out == NULL){ fclose(out); return 0; } .... } PVS-Studio warning: V575 The null pointer is passed into 'fclose' function. Inspect the first argument. ogg_enc.cpp 47 Quite an interesting example. The analyzer detected that the argument in the fclose is nullptr, which makes the function call meaningless. Presumably, the stream in was to be closed. void NVI_Image::ABGR8_To_ARGB8() { // swaps RGB for all pixels assert(IsDataValid()); assert(GetBytesPerPixel() == 4); UINT hxw = GetNumPixels(); for (UINT i = 0; i < hxw; i++) { DWORD col; GetPixel_ARGB8(&col, i); DWORD a = (col >> 24) && 0x000000FF; DWORD b = (col >> 16) && 0x000000FF; DWORD g = (col >> 8) && 0x000000FF; DWORD r = (col >> 0) && 0x000000FF; col = (a > 16) & 0x000000FF; DWORD g = (col >> 8) & 0x000000FF; DWORD r = (col >> 0) & 0x000000FF; Another example of strange code: VertexCache::VertexCache() { VertexCache(16); } PVS-Studio warning: V603 The object was created but it is not being used. If you wish to call constructor, 'this->VertexCache::VertexCache(....)' should be used. vertexcache.cpp 6 Instead of calling a constructor from another, a new object of VertexCache gets created, and then destroyed, to initialize the instance. As a result, the members of the created object remain uninitialized. BOOL CActor::net_Spawn(CSE_Abstract* DC) { .... m_States.empty(); .... } PVS-Studio warning: V530 The return value of function 'empty' is required to be utilized. actor_network.cpp 657 The analyzer warns that the value returned by the function is not used. It seems that the programmer confused the methods empty() and clear(): the empty() does not clear the array, but checks whether it is empty or not. Such errors are quite common in various projects. The thing is that the name empty() is not very obvious: some view it as an action - deletion. To avoid such ambiguity, it's a good idea to add has, or is to the beginning of the method: it would be harder to confuse isEmpty() with clear(). A similar warning: V530 The return value of function 'unique' is required to be utilized. uidragdroplistex.cpp 780 size_t xrDebug::BuildStackTrace(EXCEPTION_POINTERS* exPtrs, char *buffer, size_t capacity, size_t lineCapacity) { memset(buffer, capacity*lineCapacity, 0); .... } PVS-Studio warning: V575 The 'memset' function processes '0' elements. Inspect the third argument. xrdebug.cpp 104 During the memset call the arguments got mixed up, and as a result the buffer isn't set to zero, as it was originally intended. This error can live in a project for quite a long time, because it is very difficult to detect. In such cases a static analyzer is of great help. The correct use of memset: memset(buffer, 0, capacity*lineCapacity); The following error is connected with incorrectly formed logical expression. void configs_dumper::dumper_thread(void* my_ptr) { .... DWORD wait_result = WaitForSingleObject( this_ptr->m_make_start_event, INFINITE); while ( wait_result != WAIT_ABANDONED) || (wait_result != WAIT_FAILED)) .... } PVS-Studio warning: V547 Expression is always true. Probably the '&&' operator should be used here. configs_dumper.cpp 262 The expression 'x != a || x != b' is always true. Most likely, && was meant to be here instead of || operator. More details on the topic of errors in logical expressions can be found in the article "Logical Expressions in C/C++. Mistakes Made by Professionals". void SBoneProtections::reload(const shared_str& bone_sect, IKinematics* kinematics) { .... CInifile::Sect &protections = pSettings->r_section(bone_sect); for (CInifile::SectCIt i=protections.Data.begin(); protections.Data.end() != i; ++i) { string256 buffer; BoneProtection BP; .... BP.BonePassBullet = (BOOL) ( atoi( _GetItem(i->second.c_str(), 2, buffer) )>0.5f); .... } } PVS-Studio warning: V674 The '0.5f' literal of the 'float' type is compared to a value of the 'int' type. boneprotections.cpp 54 The analyzer detected an integer comparison with a real constant. Perhaps, by analogy, the atof function, not atoi was supposed to be here, but in any case, this comparison should be rewritten so that it doesn't look suspicious. However, only the author of this code can say for sure if this code is erroneous or not. class IGameObject : public virtual IFactoryObject, public virtual ISpatial, public virtual ISheduled, public virtual IRenderable, public virtual ICollidable { public: .... virtual u16 ID() const = 0; .... } BOOL CBulletManager::test_callback( const collide::ray_defs& rd, IGameObject* object, LPVOID params) { bullet_test_callback_data* pData = (bullet_test_callback_data*)params; SBullet* bullet = pData->pBullet; if( (object->ID() == bullet->parent_id) && (bullet->fly_distflags.ricochet_was)) return FALSE; BOOL bRes = TRUE; if (object){ .... } return bRes; } PVS-Studio warning: V595 The 'object' pointer was utilized before it was verified against nullptr. Check lines: 42, 47. level_bullet_manager_firetrace.cpp 42 The verification of the object pointer against nullptr occurs after the object->ID() is dereferenced. In the case where object is nullptr, the program will crash. #ifdef _EDITOR BOOL WINAPI DllEntryPoint(....) #else BOOL WINAPI DllMain(....) #endif { switch (ul_reason_for_call) { .... case DLL_THREAD_ATTACH: if (!strstr(GetCommandLine(), "-editor")) CoInitializeEx(NULL, COINIT_MULTITHREADED); timeBeginPeriod(1); break; .... } return TRUE; } PVS-Studio warning: V718 The 'CoInitializeEx' function should not be called from 'DllMain' function. xrcore.cpp 205 In the DllMain, we cannot use a part of WinAPI function, including CoInitializeEx. You may read documentation on MSDN to be clear on this. There is probably no definite answer of how to rewrite this function, but we should understand that this situation is really dangerous, because it can cause thread deadlock, or a program crash. Precedence errors int sgetI1( unsigned char **bp ) { int i; if ( flen == FLEN_ERROR ) return 0; i = **bp; if ( i > 127 ) i -= 256; flen += 1; *bp++; return i; } PVS-Studio warning: V532 Consider inspecting the statement of '*pointer++' pattern. Probably meant: '(*pointer)++'. lwio.c 316 The error is related to increment usage. To make this expression more clear, let's rewrite it, including the brackets: *(bp++); So we'll have a shift not of the content by bp address, but the pointer itself, which is meaningless in this context. Further on in the code there are fragments of *bp += N type, made me think that this is an error. Placing parentheses could help to avoid this error and make the evaluation more clear. Also a good practice is to use const for arguments that should not changed. Similar warnings: V532 Consider inspecting the statement of '*pointer++' pattern. Probably meant: '(*pointer)++'. lwio.c 354 V532 Consider inspecting the statement of '*pointer++' pattern. Probably meant: '(*pointer)++'. lwob.c 80 void CHitMemoryManager::load (IReader &packet) { .... if (!spawn_callback || !spawn_callback->m_object_callback) if(!g_dedicated_server) Level().client_spawn_manager().add( delayed_object.m_object_id,m_object->ID(),callback); #ifdef DEBUG else { if (spawn_callback && spawn_callback->m_object_callback) { VERIFY(spawn_callback->m_object_callback == callback); } } #endif // DEBUG } PVS-Studio warning: V563 It is possible that this 'else' branch must apply to the previous 'if' statement. hit_memory_manager.cpp 368 In this fragment the else branch is related to the second if because of its right-associativity, which doesn't coincide with the code formatting. Fortunately, this doesn't affect the work of the program in any way, but nevertheless, it can make the debugging and testing process much more complicated. So the recommendation is simple - put curly brackets in more, or less, complex branches. void HUD_SOUND_ITEM::PlaySound(HUD_SOUND_ITEM& hud_snd, const Fvector& position, const IGameObject* parent, bool b_hud_mode, bool looped, u8 index) { .... hud_snd.m_activeSnd->snd.set_volume( hud_snd.m_activeSnd->volume * b_hud_mode?psHUDSoundVolume:1.0f); } PVS-Studio warning: V502 Perhaps the '?:' operator works in a different way than it was expected. The '?:' operator has a lower priority than the '*' operator. hudsound.cpp 108 A ternary conditional operator has a lower precedence than the multiplication operator, that's why the order of operations will be as follows: (hud_snd.m_activeSnd->volume * b_hud_mode)?psHUDSoundVolume:1.0f Apparently, the correct code should be the following: hud_snd.m_activeSnd->volume * (b_hud_mode?psHUDSoundVolume:1.0f) Expressions containing a ternary operator, several if-else branches, or operations AND/OR, are all cases where it's better to put extra brackets. Similar warnings: V502 Perhaps the '?:' operator works in a different way than was expected. The '?:' operator has a lower priority than the '+' operator. uihudstateswnd.cpp 487 V502 Perhaps the '?:' operator works in a different way than was expected. The '?:' operator has a lower priority than the '+' operator. uicellcustomitems.cpp 106 Unnecessary comparisons void CDestroyablePhysicsObject::OnChangeVisual() { if (m_pPhysicsShell){ if(m_pPhysicsShell)m_pPhysicsShell->Deactivate(); .... } .... } PVS-Studio warning: V571 Recurring check. The 'if (m_pPhysicsShell)' condition was already verified in line 32. destroyablephysicsobject.cpp 33 In this case m_pPhysicsShell gets checked twice. Most likely, the second check is redundant. void CSE_ALifeItemPDA::STATE_Read(NET_Packet &tNetPacket, u16 size) { .... if (m_wVersion > 89) if ( (m_wVersion > 89)&&(m_wVersion < 98) ) { .... }else{ .... } } PVS-Studio warning: V571 Recurring check. The 'm_wVersion > 89' condition was already verified in line 987. xrserver_objects_alife_items.cpp 989 This code is very strange. In this fragment we see that a programmer either forgot an expression after if (m_wVersion > 89), or a whole series of else-if. This method requires a more scrutinizing review. void ELogCallback(void *context, LPCSTR txt) { .... bool bDlg = ('#'==txt[0])||((0!=txt[1])&&('#'==txt[1])); if (bDlg){ int mt = ('!'==txt[0])||((0!=txt[1])&&('!'==txt[1]))?1:0; .... } } PVS-Studio warnings: V590 Consider inspecting the '(0 != txt[1]) && ('#' == txt[1])' expression. The expression is excessive or contains a misprint. elog.cpp 29 V590 Consider inspecting the '(0 != txt[1]) && ('!' == txt[1])' expression. The expression is excessive or contains a misprint. elog.cpp 31 The check (0 != txt[1]) is excessive in the expressions of initialization of the bDlg and mt variables. If we omit it, the expression will be much easier to read. bool bDlg = ('#'==txt[0])||('#'==txt[1]); int mt = ('!'==txt[0])||('!'==txt[1])?1:0; Errors in data types float CRenderTarget::im_noise_time; CRenderTarget::CRenderTarget() { .... param_blur = 0.f; param_gray = 0.f; param_noise = 0.f; param_duality_h = 0.f; param_duality_v = 0.f; param_noise_fps = 25.f; param_noise_scale = 1.f; im_noise_time = 1/100; im_noise_shift_w = 0; im_noise_shift_h = 0; .... } PVS-Studio warning: V636 The '1 / 100' expression was implicitly cast from 'int' type to 'float' type. Consider utilizing an explicit type cast to avoid the loss of a fractional part. An example: double A = (double)(X) / Y;. gl_rendertarget.cpp 245 The value of the expression 1/100 is 0, since it is an operation of integer division. To get the value 0.01f, we need to use a real literal and rewrite the expression: 1/100.0f. Although, there is still a chance that such behavior was meant to be here, and there is no error. CSpaceRestriction::merge(....) const { .... LPSTR S = xr_alloc(acc_length); for ( ; I != E; ++I) temp = strconcat(sizeof(S),S,*temp,",",*(*I)->name()); .... } PVS-Studio warning: V579 The strconcat function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the first argument. space_restriction.cpp 201 The function strconcat gets the buffer size as the first parameter. S buffer is declared as a LPSTR, i.e. as a pointer to a string. sizeof(S) will be equal to the pointer size in bites, namely sizeof(char *), not the number of symbols in the string. To evaluate the length we should use strlen(S). class XRCDB_API MODEL { .... u32 status; // 0=ready, 1=init, 2=building .... } void MODEL::build (Fvector* V, int Vcnt, TRI* T, int Tcnt, build_callback* bc, void* bcp) { .... BTHREAD_params P = { this, V, Vcnt, T, Tcnt, bc, bcp }; thread_spawn(build_thread,"CDB-construction",0,&P); while (S_INIT == status) Sleep(5); .... } PVS-Studio warning: V712 Be advised that compiler may delete this cycle, or make it infinite. Use volatile variable(s) or synchronization primitives to avoid this. xrcdb.cpp 100 The compiler can remove the check S_INIT == status as a measure of optimization, because the status variable isn't modified in the loop. To avoid such behavior, we should use volatile variables, or types of data synchronization between the threads. Similar warnings: V712 Be advised that compiler may delete this cycle or make it infinite. Use volatile variable(s) or synchronization primitives to avoid this. levelcompilerloggerwindow.cpp 23 V712 Be advised that compiler may delete this cycle or make it infinite. Use volatile variable(s) or synchronization primitives to avoid this. levelcompilerloggerwindow.cpp 232 void CAI_Rat::UpdateCL() { .... if (!Useful()) { inherited::UpdateCL (); Exec_Look (Device.fTimeDelta); CMonsterSquad *squad = monster_squad().get_squad(this); if (squad && ((squad->GetLeader() != this && !squad->GetLeader()->g_Alive()) || squad->get_index(this) == u32(-1))) squad->SetLeader(this); .... } .... } PVS-Studio warning: V547 Expression 'squad->get_index(this) == u32(- 1)' is always false. The value range of unsigned char type: [0, 255]. ai_rat.cpp 480 To understand why this expression is always false, let's evaluate values of individual operands. u32(-1) is 0xFFFFFFFF or 4294967295. The type, returned by the method squad->get_index(....), is u8, thus its maximum value is 0xFF or 255, which is strictly less than u32(-1). Consequently, the result of such comparison will always be false. This code can be easily fixed, if we change the data type to u8: squad->get_index(this) == u8(-1) The same diagnostic is triggered for redundant comparisons of unsigned variables. namespace ALife { typedef u64 _TIME_ID; } ALife::_TIME_ID CScriptActionCondition::m_tLifeTime; IC bool CScriptEntityAction::CheckIfTimeOver() { return((m_tActionCondition.m_tLifeTime >= 0) && ((m_tActionCondition.m_tStartTime + m_tActionCondition.m_tLifeTime) < Device.dwTimeGlobal)); } PVS-Studio warning: V547 Expression 'm_tActionCondition.m_tLifeTime >= 0' is always true. Unsigned type value is always >= 0. script_entity_action_inline.h 115 The variable m_tLifeTime is unsigned, thus it is always greater than or equal to zero. It's up to the developer to say if it is an excessive check, or an error in the logic of the program. The same warning: V547 Expression 'm_tActionCondition.m_tLifeTime < 0' is always false. Unsigned type value is never < 0. script_entity_action_inline.h 143 ObjectFactory::ServerObjectBaseClass * CObjectItemScript::server_object (LPCSTR section) const { ObjectFactory::ServerObjectBaseClass *object = nullptr; try { object = m_server_creator(section); } catch(std::exception e) { Msg("Exception [%s] raised while creating server object from " "section [%s]", e.what(),section); return (0); } .... } PVS-Studio warning: V746 Type slicing. An exception should be caught by reference rather than by value. object_item_script.cpp 39 The function std::exception::what() is virtual, and can be overridden in inherited classes. In this example, the exception is caught by value, hence the class instance will be copied, and all information about the polymorphic type will be lost. Accessing what() is meaningless in this case. The exception should be caught by reference: catch(const std::exception& e) { Miscelleneous void compute_cover_value (....) { .... float value [8]; .... if (value[0] < .999f) { value[0] = value[0]; } .... } PVS-Studio warning: V570 The 'value[0]' variable is assigned to itself. compiler_cover.cpp 260 The variable value[0] is assigned to itself. It's unclear, why this should be. Perhaps a different value should be assigned to it. void CActor::g_SetSprintAnimation(u32 mstate_rl, MotionID &head, MotionID &torso, MotionID &legs) { SActorSprintState& sprint = m_anims->m_sprint; bool jump = (mstate_rl&mcFall) || (mstate_rl&mcLanding) || (mstate_rl&mcLanding) || (mstate_rl&mcLanding2) || (mstate_rl&mcJump); .... } PVS-Studio warning: V501 There are identical sub-expressions '(mstate_rl & mcLanding)' to the left and to the right of the '||' operator. actoranimation.cpp 290 Most probably, we have an extra check mstate_rl & mcLanding, but quite often such warnings indicate an error in the logic and enum values that weren't considered. Similar warnings: V501 There are identical sub-expressions 'HudItemData()' to the left and to the right of the '&&' operator. huditem.cpp 338 V501 There are identical sub-expressions 'list_idx == e_outfit' to the left and to the right of the '||' operator. uimptradewnd_misc.cpp 392 V501 There are identical sub-expressions '(D3DFMT_UNKNOWN == fTarget)' to the left and to the right of the '||' operator. hw.cpp 312 RELATION_REGISTRY::RELATION_MAP_SPOTS::RELATION_MAP_SPOTS() { .... spot_names[ALife::eRelationTypeWorstEnemy] = "enemy_location"; spot_names[ALife::eRelationTypeWorstEnemy] = "enemy_location"; .... } PVS-Studio warning: V519 The variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 57, 58. relation_registry.cpp 58 The analyzer detected that the same variable is assigned with two values in a row. In this case, it seems that it's just dead code, and it should be removed. void safe_verify(....) { .... printf("FATAL ERROR (%s): failed to verify data\n"); .... } PVS-Studio warning: V576 Incorrect format. A different number of actual arguments is expected while calling 'printf' function. Expected: 2. Present: 1. entry_point.cpp 41 An insufficient number or arguments is passed to the printpf function: the format '%s' shows that the pointer to a string should be passed. Such a situation can lead to a memory access error, and to program termination. Conclusion The analysis of X-Ray Engine detected a good number of both redundant and suspicious code, as well as erroneous and hazardous moments. It is worth noting that a static analyzer is of great help in detecting errors during the early stages of development, which significantly simplifies the life of a programmer and provides more time for creation of new versions of your applications. By Pavel Belikov
  5. Unity3D is one of the most promising and rapidly developing game engines to date. Every now and then, the developers upload new libraries and components to the official repository, many of which weren't available in as open-source projects until recently. Unfortunately, the Unity3D developer team allowed the public to dissect only some of the components, libraries, and demos employed by the project, while keeping the bulk of its code closed. In this article, we will try to find bugs and typos in those components with the help of PVS-Studio static analyzer. Introduction We decided to check all the components, libraries, and demos in C#, whose source code is available in the Unity3D developer team's official repository: UI System- system for GUI development. Networking- system for implementing multiplayer mode. MemoryProfiler - system for profiling resources in use. XcodeAPI- component for interacting with the Xcode IDE. PlayableGraphVisualizer - system for project execution visualization. UnityTestTools - Unity3D testing utilities (no Unit tests included). AssetBundleDemo - project with AssetBundleServer's source files and demos for AssetBundle system. AudioDemos - demo projects for the audio system. NativeAudioPlugins - audio plugins (we are only interested in the demos for these plugins). GraphicsDemos - demo projects for the graphics system. I wish we could take a look at the source files of the engine's kernel itself, but, unfortunately, no one except the developers themselves have access to it currently. So, what we've got on our operating table today is just a small part of the engine's source files. We are most interested in the UI system designed for implementing a more flexible GUI than the older, clumsy one, and the networking library, which served us hand and foot before UNet's release. We are also as much interested in MemoryProfiler, which is a powerful and flexible tool for resource and load profiling. Errors and suspicious fragments found All warnings issued by the analyzer are grouped into 3 levels: High - almost certainly an error. Medium - possible error or typo. Low - unlikely error or typo. We will be discussing only the high and medium levels in this article. The table below presents the list of projects we have checked and analysis statistics across all the projects. The columns "Project name" and "Number of LOC" are self-explanatory, but the "Issued Warnings" column needs some explanation. It contains information about all the warnings issued by the analyzer. Positive warnings are warnings that directly or indirectly point to real errors or typos in the code. False warnings, or false positives, are those that interpret correct code as faulty. As I already said, all warnings are grouped into 3 levels. We will be discussing only the high- and medium-level warnings, since the low level mostly deals with information messages or unlikely errors. For the 10 projects checked, the analyzer issued 16 high-level warnings, 75% of which correctly pointed to real defects in the code, and 18 medium-level warnings, 39% of which correctly pointed to real defects in the code. The code is definitely of high quality, as the average ratio of errors found to the number of LOC is one error per 2000 lines of code, which is a good result. Now that we are done with the statistics, let's see what errors and typos we managed to find. Incorrect regular expression V3057 Invalid regular expression pattern in constructor. Inspect the first argument. AssetBundleDemo ExecuteInternalMono.cs 48 private static readonly Regex UnsafeCharsWindows = new Regex("[^A-Za-z0-9\\_\\-\\.\\:\\,\\/\\@\\\\]"); //
  6. In May 2016, German game-development company Crytek made a decision to upload the source code of their game engine CryEngine V to Github. The engine is written in C++ and has immediately attracted attention of both the open-source developer community and the team of developers of PVS-Studio static analyzer who regularly scan the code of open-source projects to estimate its quality. A lot of great games were created by a number of video-game development studios using various versions of CryEngine, and now the engine has become available to even more developers. This article gives an overview of errors found in the project by PVS-Studio static analyzer. Introduction CryEngine is a game engine developed by German company Crytek in 2002 and originally used in first-person shooter Far Cry. A lot of great games were created by a number of video-game development studios using various licensed versions of CryEngine: Far Cry, Crysis, Entropia Universe, Blue Mars, Warface, Homefront: The Revolution, Sniper: Ghost Warrior, Armored Warfare, Evolve, and many others. In March 2016, Crytek announced a release date for their new engine CryEngine V and uploaded its source code to Github soon after. The project's source code was checked by PVS-Studio static analyzer, version 6.05. This is a tool designed for detecting software errors in program source code in C, C++, and C#. The only true way of using static analysis is to regularly scan code on developers' computers and build-servers. However, in order to demonstrate PVS-Studio's diagnostic capabilities, we run single-time checks of open-source projects and then write articles about errors found. If we like a project, we might scan it again a couple of years later. Such recurring checks are in fact the same as single-time checks since the code accumulates a lot of changes during that time. For our checks, we pick projects that are simply popular and wide-known as well as projects suggested by our readers via e-mail. That's why CryEngine V was by no means the first game engine among those scanned by our analyzer. Other engines that we have already checked include: Unreal Engine 4 (first check, second check, third check) Check of Godot Engine Check of Serious Engine Check of X-Ray Engine Check of Xenko Engine We also checked CryEngine 3 SDK once. I'd like to elaborate on the check of Unreal Engine 4 engine in particular. Using that project as an example allowed us to demonstrate in every detail what the right way of using static analysis on a real project should look like, covering the whole process from the phase of integrating the analyzer into the project to the phase of cutting warnings to zero with subsequent control over bug elimination in new code. Our work on Unreal Engine 4 project developed into collaboration with Epic Games company, in terms of which our team fixed all the defects found in the engine's source code and wrote a joint article with Epic Games on the accomplished work (it was posted on Unreal Engine Blog). Epic Games also purchased a PVS-Studio license to be able to maintain the quality of their code on their own. Collaboration of this kind is something that we would like to try with Crytek, too. Analyzer-report structure In this article, I'd like to answer a few frequently asked questions concerning the number of warnings and false positives, for example, "What is the ratio of false positives?" or "Why are there so few bugs in so large a project?" To begin with, all PVS-Studio warnings are classified into three severity levels: High, Medium, and Low. The High level holds the most critical warnings, which are almost surely real errors, while the Low level contains the least critical warnings or warnings that are very likely to be false positives. Keep in mind that the codes of errors do not tie them firmly to particular severity levels: distribution of warnings across the levels very much depends on the context. This is how the warnings of the General Analysis module are distributed across the severity levels for CryEngine V project: High: 576 warnings; Medium: 814 warnings, Low: 2942 warnings. Figure 1 shows distribution of the warnings across the levels in the form of a pie chart. Figure 1 - Percentage distribution of warnings across severity levels It is impossible to include all the warning descriptions and associated code fragments in an article. Our articles typically discuss 10-40 commented cases; some warnings are given as a list; and most have to be left unexamined. In the best-case scenario, project authors, after we inform them, ask for a complete analysis report for close study. The bitter truth is that in most cases the number of High-level warnings alone is more than enough for an article, and CryEngine V is no exception. Figure 2 shows the structure of the High-level warnings issued for this project. Figure 2 - Structure of High-level warnings Let's take a closer look at the sectors of this chart: Described in the article (6%) - warnings cited in the article and accompanied by code fragments and commentary. Presented as a list (46%) - warnings cited as a list. These warnings refer to the same pattern as some of the errors already discussed, so only the warning text is given. False Positives (8%) - a certain ratio of false positives we have taken into account for future improvement of the analyzer. Other (40%) - all the other warnings issued. These include warnings that we had to leave out so that the article wouldn't grow too large, non-critical warnings, or warnings whose validity could be estimated only by a member of the developer team. As our experience of working on Unreal Engine 4 has shown, such code still "smells" and those warnings get fixed anyway. Analysis results Annoying copy-paste V501 There are identical sub-expressions to the left and to the right of the '-' operator: q2.v.z - q2.v.z entitynode.cpp 93 bool CompareRotation(const Quat& q1, const Quat& q2, float epsilon) { return (fabs_tpl(q1.v.x - q2.v.x) SRenderingPassInfo::SRenderingPassInfo(....)' should be used. i3dengine.h 2589 SRenderingPassInfo() : pShadowGenMask(NULL) , nShadowSide(0) , nShadowLod(0) , nShadowFrustumId(0) , m_bAuxWindow(0) , m_nRenderStackLevel(0) , m_eShadowMapRendering(static_cast(SHADOW_MAP_NONE)) , m_bCameraUnderWater(0) , m_nRenderingFlags(0) , m_fZoomFactor(0.0f) , m_pCamera(NULL) , m_nZoomInProgress(0) , m_nZoomMode(0) , m_pJobState(nullptr) { threadID nThreadID = 0; gEnv->pRenderer->EF_Query(EFQ_MainThreadList, nThreadID); m_nThreadID = static_cast(nThreadID); m_nRenderFrameID = gEnv->pRenderer->GetFrameID(); m_nRenderMainFrameID = gEnv->pRenderer->GetFrameID(false); } SRenderingPassInfo(threadID id) { SRenderingPassInfo(); // GetSpawnData(i); const float amount = spawn.amount; const int spawnedBefore = int(spawn.spawned); const float endTime = spawn.delay + HasDuration() ? spawn.duration : fHUGE; .... } The function above seems to measure time in a wrong way. The precedence of the addition operator is higher than that of the ternary operator :?, so the value 0 or 1 is added to spawn.delay first, and then the value spawn.duration or fHUGE is written into the endTime variable. This error is quite a common one. To learn more about interesting patterns of errors involving operation precedence collected from the PVS-Studio bug database, see my article: Logical Expressions in C/C++. Mistakes Made by Professionals. V634 The priority of the '*' operation is higher than that of the 'GetLightProfileCounts().ResetFrameTicks(); if (passInfo.IsGeneralPass() && m_pPartManager) m_pPartManager->Update(); .... } The m_pPartManager pointer is dereferenced and then checked. Example 2 V595 The 'gEnv->p3DEngine' pointer was utilized before it was verified against nullptr. Check lines: 1477, 1480. gameserialize.cpp 1477 bool CGameSerialize::LoadLevel(....) { .... // can quick-load if (!gEnv->p3DEngine->RestoreTerrainFromDisk()) return false; if (gEnv->p3DEngine) { gEnv->p3DEngine->ResetPostEffects(); } .... } The gEnv->p3DEngine pointer is dereferenced and then checked. Example 3 V595 The 'pSpline' pointer was utilized before it was verified against nullptr. Check lines: 158, 161. facechannelkeycleanup.cpp 158 void FaceChannel::CleanupKeys(....) { CFacialAnimChannelInterpolator backupSpline(*pSpline); // Create the key entries array. int numKeys = (pSpline ? pSpline->num_keys() : 0); .... } The pSpline pointer is dereferenced and then checked. Miscellaneous V622 Consider inspecting the 'switch' statement. It's possible that the first 'case' operator is missing. mergedmeshrendernode.cpp 999 static inline void ExtractSphereSet(....) { .... switch (statusPos.pGeom->GetType()) { if (false) { case GEOM_CAPSULE: statusPos.pGeom->GetPrimitive(0, &cylinder); } if (false) { case GEOM_CYLINDER: statusPos.pGeom->GetPrimitive(0, &cylinder); } for (int i = 0; i < 2 && ....; ++i) { .... } break; .... } This fragment is probably the strangest of all found in CryEngine V. Whether or not the case label will be selected does not depend on the if statement, even in case of if (false). In the switch statement, an unconditional jump to the label occurs if the condition of the switch statement is met. Without the break statement, one could use such code to "bypass" irrelevant statements, but, again, maintaining such obscure code isn't easy. One more question is, why does the same code execute when jumping to the labels GEOM_CAPSULE and GEOM_CYLINDER? V510 The 'LogError' function is not expected to receive class-type variable as second actual argument. behaviortreenodes_action.cpp 143 typedef CryStringT<char> string; // The actual fragment name. string m_fragName; //! cast to C string. const value_type* c_str() const { return m_str; } const value_type* data() const { return m_str; }; void LogError(const char* format, ...) const { .... } void QueueAction(const UpdateContext& context) { .... ErrorReporter(*this, context).LogError("....'%s'", m_fragName); .... } When it is impossible to specify the number and types of all acceptable parameters to a function, one puts ellipsis (...) at the end of the list of parameters in the function declaration, which means "and perhaps a few more". Only POD (Plain Old Data) types can be used as actual parameters to the ellipsis. If an object of a class is passed as an argument to a function's ellipsis, it almost always signals the presence of a bug. In the code above, it is the contents of the object that get to the stack, not the pointer to a string. Such code results in forming "gibberish" in the buffer or a crash. The code of CryEngine V uses a string class of its own, and it already has an appropriate method, c_str(). The fixed version: LogError("....'%s'", m_fragName.c_str(); A few more suspicious fragments: V510 The 'LogError' function is not expected to receive class-type variable as second actual argument. behaviortreenodes_core.cpp 1339 V510 The 'Format' function is not expected to receive class-type variable as second actual argument. behaviortreenodes_core.cpp 2648 V510 The 'CryWarning' function is not expected to receive class-type variable as sixth actual argument. crypak.cpp 3324 V510 The 'CryWarning' function is not expected to receive class-type variable as fifth actual argument. crypak.cpp 3333 V510 The 'CryWarning' function is not expected to receive class-type variable as fifth actual argument. shaderfxparsebin.cpp 4864 V510 The 'CryWarning' function is not expected to receive class-type variable as fifth actual argument. shaderfxparsebin.cpp 4931 V510 The 'Format' function is not expected to receive class-type variable as third actual argument. featuretester.cpp 1727 V529 Odd semicolon ';' after 'for' operator. boolean3d.cpp 1314 int CTriMesh::Slice(....) { .... bop_meshupdate *pmd = new bop_meshupdate, *pmd0; pmd->pMesh[0]=pmd->pMesh[1] = this; AddRef();AddRef(); for(pmd0=m_pMeshUpdate; pmd0->next; pmd0=pmd0->next); // next = pmd; .... } This code is very strange. The programmer put a semicolon after the for loop, while the code formatting suggests that it should have a body. V535 The variable 'j' is being used for this loop and for the outer loop. Check lines: 3447, 3490. physicalworld.cpp 3490 void CPhysicalWorld::SimulateExplosion(....) { .... for(j=0;jnIslands;j++) // GetBreakability() == 2 && pCollision->idmat[0] != pCollision->idmat[1] && (energy = pMat->GetBreakEnergy()) > 0 && pCollision->mass[0] * 2 > energy && .... pMat->GetHitpoints()
  7. The first-person shooter 'Serious Sam' celebrated its release anniversary on March, 2016. In honor of this, the game developers form the Croatian company Croteam decided to open the source code for the game engine, Serious Engine 1 v.1.10. It provoked the interest of a large number of developers, who got an opportunity to have a look at the code and improve it. I have also decided to participate in the code improvement, and wrote an article reviewing the bugs that were found by PVS-Studio analyzer. Introduction Serious Engine is a game engine developed by a Croatian company Croteam. V 1.1o, and was used in the games 'Serious Sam Classic: The First Encounter', and 'Serious Sam Classic: The Second Encounter'. Later on, the Croteam Company released more advanced game engines - Serious Engine 2, Serious Engine 3, and Serious Engine 4; the source code of Serious Engine version 1.10 was officially made open and available under the license GNU General Public License v.2 The project is easily built in Visual Studio 2013, and checked by PVS-Studio 6.02 static analyzer. Typos! V501 There are identical sub-expressions to the left and to the right of the '==' operator: tp_iAnisotropy == tp_iAnisotropy gfx_wrapper.h 180 class CTexParams { public: inline BOOL IsEqual( CTexParams tp) { return tp_iFilter == tp.tp_iFilter && tp_iAnisotropy == tp_iAnisotropy && //
  8. PVS-studio team

    Sony C#/.NET Component Set Analysis

    Some of you may know that we have recently released version 6.00 of our analyzer, that now has C# support. The ability to scan C# projects increases the number of open-source projects we can analyze. This article is about one such check. This time it is a project, developed by Sony Computer Entertainment (SCEI). What have we checked? Sony Computer Entertainment is a video game company. Being a branch of Sony Corporation, it specializes in video games and game consoles. This company develops video games, hardware and software for PlayStation consoles. Authoring Tools Framework (ATF) is a set of C#/.NET components for making tools on Windows(R). ATF has been used by most Sony Computer Entertainment first party game studios to make custom tools. This component set is used by such studios as Naughty Dog, Guerrilla Games and Quantic Dream. Tools developed with these program components were used during the creation of such well know games as 'The Last of Us' and 'Killzone'. ATF is an open-source project which is available at this GitHub repository. Analysis tool To perform the source code analysis, we used PVS-Studio static code analyzer. This tool scans projects written in C/C++/C#. Every diagnostic message has a detailed description in the documentation with examples of incorrect code and possible ways to fix the bugs. Quite a number of the diagnostic descriptions have a link to corresponding sections of error base, where you can see information about the bugs that were found in real projects with the help of these diagnostics. You can download the analyzer here and run it on your (or somebody's) code. Examples of errors public static void DefaultGiveFeedback(IComDataObject data, GiveFeedbackEventArgs e) { .... if (setDefaultDropDesc && (DropImageType)e.Effect != currentType) { if (e.Effect != DragDropEffects.None) { SetDropDescription(data, (DropImageType)e.Effect, e.Effect.ToString(), null); } else { SetDropDescription(data, (DropImageType)e.Effect, e.Effect.ToString(), null); } .... } } Analyzer warning: V3004 The 'then' statement is equivalent to the 'else' statement. Atf.Gui.WinForms.vs2010 DropDescriptionHelper.cs 199 As you see in the code, the same method with similar arguments will be called, in spite of the e.Effect != DragDropEffects.None being true or not. It's hard to suggest any ways to fix this code fragment, without being a developer of this code, but I think it is clear that this fragment needs a more thorough revision. What exactly should be fixed, is a question which should be directed to the author of this code. Let's look at the following code fragment: public ProgressCompleteEventArgs(Exception progressError, object progressResult, bool cancelled) { ProgressError = ProgressError; ProgressResult = progressResult; Cancelled = cancelled; } Analyzer warning: V3005 The 'ProgressError' variable is assigned to itself. Atf.Gui.Wpf.vs2010 StatusService.cs 24 It was supposed that during the method call, the properties would get values, passed as arguments; at the same time the names of properties and parameters differ only in the first letter case. As a result - ProgressError property is assigned to itself instead of being given the progressError parameter. Quite interesting here, is the fact that it's not the only case where capital and small letters get confused. Several of the projects we've checked have the same issues. We suspect that we'll find a new error pattern typical for C# programs soon. There is a tendency to initialize properties in a method, where the names of parameters differ from the names of the initialized properties by just one letter. As a result, we have errors like this. The next code fragment is probably not erroneous, but it looks rather strange, to say the least. public double Left { get; set; } public double Top { get; set; } public void ApplyLayout(XmlReader reader) { .... FloatingWindow window = new FloatingWindow( this, reader.ReadSubtree()); .... window.Left = window.Left; window.Top = window.Top; .... } Analyzer warning: V3005 The 'window.Left' variable is assigned to itself. Atf.Gui.Wpf.vs2010 DockPanel.cs 706 | V3005 The 'window.Top' variable is assigned to itself. Atf.Gui.Wpf.vs2010 DockPanel.cs 707 In the analyzer warnings you can see that the window object properties Left and Top are assigned to themselves. In some cases this variant is perfectly appropriate, for example when the property access method has special logic. But there is no additional logic for these properties, and so it is unclear why the code is written this way. Next example: private static void OnBoundPasswordChanged(DependencyObject d, DependencyPropertyChangedEventArgs e) { PasswordBox box = d as PasswordBox; if (d == null || !GetBindPassword(d)) { return; } // avoid recursive updating by ignoring the box's changed event box.PasswordChanged -= HandlePasswordChanged; .... } Analyzer warning: V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'd', 'box'. Atf.Gui.Wpf.vs2010 PasswordBoxBehavior.cs 38 We have already seen quite a number of errors of this type in the C# projects that we checked. By casting an object to a compatible type using as operator the programmer gets a new object, but further in the code the source object is compared to null. This code can work correctly, if you are sure that the d object will always be compatible with the PasswordBox type. But it is not so (for now or if there are more changes in the program); you can easily get NullReferenceException in code that used to work correctly. So in any case this code needs to be reviewed. In the following example, conversely, the programmer clearly tried to make the code as secure as possible, although it's not quite clear what for. public Rect Extent { get { return _extent; } set { if (value.Top < -1.7976931348623157E+308 || value.Top > 1.7976931348623157E+308 || value.Left < -1.7976931348623157E+308 || value.Left > 1.7976931348623157E+308 || value.Width > 1.7976931348623157E+308 || value.Height > 1.7976931348623157E+308) { throw new ArgumentOutOfRangeException("value"); } _extent = value; ReIndex(); } } Analyzer warning: V3022 Expression is always false. Atf.Gui.Wpf.vs2010 PriorityQuadTree.cs 575 This condition will always be false. Let's look into the code and see why. This is an implementation of the property that has Rect type, hence value also has Rect type. Top, Left, Width, Height are properties of this type, that have double type. This code checks if these property values exceed the range of values, that the double type takes. We also see that "magic numbers" are used here for comparison, not constants, defined in the double type. That's why this condition will always be false, since the double type values are always within the value range. Apparently, the programmer wanted to secure the program from a non-standard implementation of double type in the compiler. Nevertheless, it looks rather strange, so it was reasonable for the analyzer to issue a warning, suggesting that the programmer double-check the code. Let's go on. public DispatcherOperationStatus Status { get; } public enum DispatcherOperationStatus { Pending, Aborted, Completed, Executing } public object EndInvoke(IAsyncResult result) { DispatcherAsyncResultAdapter res = result as DispatcherAsyncResultAdapter; if (res == null) throw new InvalidCastException(); while (res.Operation.Status != DispatcherOperationStatus.Completed || res.Operation.Status == DispatcherOperationStatus.Aborted) { Thread.Sleep(50); } return res.Operation.Result; } Analyzer warning: V3023 Consider inspecting this expression. The expression is excessive or contains a misprint. Atf.Gui.Wpf.vs2010 SynchronizeInvoke.cs 74 The condition of the while loop is redundant, it could be simplified by removing the second subexpression. Then the loop can be simplified in the following way: while (res.Operation.Status != DispatcherOperationStatus.Completed) Thread.Sleep(50); Next example, quite an interesting one: private Vec3F ProjectToArcball(Point point) { float x = (float)point.X / (m_width / 2); // Scale so bounds map // to [0,0] - [2,2] float y = (float)point.Y / (m_height / 2); x = x - 1; // Translate 0,0 to the center y = 1 - y; // Flip so +Y is up if (x < -1) x = -1; else if (x > 1) x = 1; if (y < -1) y = -1; else if (y > 1) y = 1; .... } Analyzer warning: V3041 The expression was implicitly cast from 'int' type to 'float' type. Consider utilizing an explicit type cast to avoid the loss of a fractional part. An example: double A = (double)(X) / Y;. Atf.Gui.OpenGL.vs2010 ArcBallCameraController.cs 216 | V3041 The expression was implicitly cast from 'int' type to 'float' type. Consider utilizing an explicit type cast to avoid the loss of a fractional part. An example: double A = (double)(X) / Y;. Atf.Gui.OpenGL.vs2010 ArcBallCameraController.cs 217 This is one of those cases where it's very hard for a third-party developer to say for sure if there is an error in this code or not. On the one hand - integer division with implicit casting to a real type looks strange. On the other hand, sometimes it can be done deliberately, regardless of the precision loss. It's difficult to say what was meant here. Perhaps the programmer didn't want to lose the precision of the code, but it will still occur as a result of m_width / 2 operation. In this case we should rewrite the code in the following way: float x = point.X / ((float)m_width / 2); On the other hand, there is a possibility that an integer number was meant to be written to x, as further on we see operations of comparison with integer values. But in this case, there was no need to do explicit casting to float type. float x = point.X / (m_width / 2); Our analyzer continues developing and obtaining new diagnostics. The next error was found with the help of our new diagnostic. But as this diagnostic wasn't in the release version of the analyzer, there will be no link to the documentation, but I hope the idea is clear: public static QuatF Slerp(QuatF q1, QuatF q2, float t) { double dot = q2.X * q1.X + q2.Y * q1.Y + q2.Z * q1.Z + q2.W * q1.W; if (dot < 0) q1.X = -q1.X; q1.Y = -q1.Y; q1.Z = -q1.Z; q1.W = -q1.W; .... } Analyzer warning: V3043 The code's operational logic does not correspond with its formatting. The statement is indented to the right, but it is always executed. It is possible that curly brackets are missing. Atf.Core.vs2010 QuatF.cs 282 You can see that a sum of several products is evaluated and the result is written to the dot variable. After that if the dot value is negative, there is inversion of all values of this operation. More precisely, the inversion was meant to be here, judging by the code formatting. In reality, only X property of q1 will be inverted, all other properties will be inverted regardless of the value of dot variable. Solution for this problem is curly brackets: if (dot < 0) { q1.X = -q1.X; q1.Y = -q1.Y; q1.Z = -q1.Z; q1.W = -q1.W; } Let's go on. public float X; public float Y; public float Z; public void Set(Matrix4F m) { .... ww = -0.5 * (m.M22 + m.M33); if (ww >= 0) { if (ww >= EPS2) { double wwSqrt = Math.Sqrt(ww); X = (float)wwSqrt; ww = 0.5 / wwSqrt; Y = (float)(m.M21 * ww); Z = (float)(m.M31 * ww); return; } } else { X = 0; Y = 0; Z = 1; return; } X = 0; ww = 0.5 * (1.0f - m.M33); if (ww >= EPS2) { double wwSqrt = Math.Sqrt(ww); Y = (float)wwSqrt; //
  9. I have checked the OpenMW project by PVS-Studio and written this tiny article. Very few bugs were found, so OpenMW team can be proud of their code. OpenMW OpenMW is an attempt to reconstruct the popular RPG Morrowind, a full-blown implementation of all of the game's specifics with open source code. To run OpenMW, you will need an original Morrowind disk. The source code can be downloaded from https://code.google.com/p/openmw/ Suspicious fragments found Fragment No. 1 std::string getUtf8(unsigned char c, ToUTF8::Utf8Encoder& encoder, ToUTF8::FromType encoding) { .... conv[0xa2] = 0xf3; conv[0xa3] = 0xbf; conv[0xa4] = 0x0; conv[0xe1] = 0x8c; conv[0xe1] = 0x8c; mData.mFlags & ESM::MagicEffect::NoDuration) ) Fragment No. 3 void Clothing::blank() { mData.mType = 0; mData.mWeight = 0; mData.mValue = 0; mData.mEnchant = 0; mParts.mParts.clear(); mName.clear(); mModel.clear(); mIcon.clear(); mIcon.clear(); mEnchant.clear(); mScript.clear(); } PVS-Studio diagnostic message: V586 The 'clear' function is called twice for deallocation of the same resource. Check lines: 48, 49. components loadclot.cpp 49 The mIcon object is cleared twice. The second clearing is redundant or something else should have been cleared instead. Fragment No. 4 void Storage::loadDataFromStream( ContainerType& container, std::istream& stream) { std::string line; while (!stream.eof()) { std::getline( stream, line ); .... } .... } PVS-Studio diagnostic message: V663 Infinite loop is possible. The 'cin.eof()' condition is insufficient to break from the loop. Consider adding the 'cin.fail()' function call to the conditional expression. components translation.cpp 45 When working with the std::istream class, calling the eof() function to terminate the loop is not enough. If a failure occurs when reading data, the call of the eof() function will return false all the time. To terminate the loop in this case, you need an additional check of the value returned by fail(). Fragment No. 5 class Factory { .... bool getReadSourceCache() { return mReadSourceCache; } bool getWriteSourceCache() { return mReadSourceCache; } .... bool mReadSourceCache; bool mWriteSourceCache; .... }; PVS-Studio diagnostic message: V524 It is odd that the body of 'getWriteSourceCache' function is fully equivalent to the body of 'getReadSourceCache' function. components factory.hpp 209 I guess the getWriteSourceCache() function should look like this: bool getWriteSourceCache() { return mWriteSourceCache; } Fragments No. 6, 7, 8 std::string rangeTypeLabel(int idx) { const char* rangeTypeLabels [] = { "Self", "Touch", "Target" }; if (idx >= 0 && idx = 0 && idx < 3) A similar defect was found in two other fragments:V557 Array overrun is possible. The value of 'idx' index could reach 143. esmtool labels.cpp 391V557 Array overrun is possible. The value of 'idx' index could reach 27. esmtool labels.cpp 475 Fragment No. 9 enum UpperBodyCharacterState { UpperCharState_Nothing, UpperCharState_EquipingWeap, UpperCharState_UnEquipingWeap, .... }; bool CharacterController::updateWeaponState() { .... if((weaptype != WeapType_None || UpperCharState_UnEquipingWeap) && animPlaying) .... } PVS-Studio diagnostic message: V560 A part of conditional expression is always true: UpperCharState_UnEquipingWeap. openmw character.cpp 949 This condition is very strange. In its current form, it can be reduced to if (animPlaying). Something is obviously wrong with it. Fragments No. 10, 11 void World::clear() { mLocalScripts.clear(); mPlayer->clear(); .... if (mPlayer) .... } PVS-Studio diagnostic message: V595 The 'mPlayer' pointer was utilized before it was verified against nullptr. Check lines: 234, 245. openmw worldimp.cpp 234 Similar defect: V595 The mBody pointer was utilized before it was verified against nullptr. Check lines: 95, 99. openmw physic.cpp 95 Fragment No. 12 void ExprParser::replaceBinaryOperands() { .... if (t1==t2) mOperands.push_back (t1); else if (t1=='f' || t2=='f') mOperands.push_back ('f'); else std::logic_error ("failed to determine result operand type"); } PVS-Studio diagnostic message: V596 The object was created but it is not being used. The 'throw' keyword could be missing: throw logic_error(FOO); components exprparser.cpp 101 The keyword throw is missing. The fixed code should look like this: throw std::logic_error ("failed to determine result operand type"); Conclusion Purchase a PVS-Studio for in your team, and you will save huge amount of time usually spent on eliminating typos and diverse bugs.
  10. The majority of the projects we report about in these articles contain dozens of PVS-Studio analyzer warnings. Of course we choose just a small portion of data from the analyzer report to be in our articles. There are some projects though, where the quantity of warnings is not that high and the number of some interesting "bloomers" is just not enough for an article. Usually these are small projects, which ceased developing. Today I'm going to tell you about Appleseed project check, the code of which we found quite high-quality, from the point of view of the analyzer. Introduction Appleseed is a modern, open source, physically-based rendering engine designed to produce photorealistic images, animations and visual effects. It provides individuals and small studios with an efficient, reliable suite of tools built on robust foundations and open technologies. This project contains 700 source code files. Our PVS-Studio analyzer found just several warnings of 1st and 2nd level that could be of interest to us. Check Results V670 The uninitialized class member m_s0_cache is used to initialize the m_s1_element_swapper member. Remember that members are initialized in the order of their declarations inside a class. animatecamera cache.h 1009 class DualStageCache : public NonCopyable { .... S1ElementSwapper m_s1_element_swapper; //
  11. We have finished a large comparison of the static code analyzers Cppcheck, PVS-Studio and Visual Studio 2013's built-in analyzer. In the course of this investigation, we checked over 10 open-source projects. Some of them do deserve to be discussed specially. In today's article, I'll tell you about the results of the check of the CryEngine 3 SDK project. CryEngine 3 SDK Wikipedia: CryEngine 3 SDK is a toolset for developing computer games on the CryEngine 3 game engine. CryEngine 3 SDK is developed and maintained by German company Crytek, the developer of the original engine CyrEngine 3. CryEngine 3 SDK is a proprietary freeware development toolset anyone can use for non-commercial game development. For commercial game development exploiting CryEngine 3, developers have to pay royalties to Crytek. PVS-Studio Let's see if PVS-Studio has found any interesting bugs in this library. True, PVS-Studio catches a bit more bugs if you turn on the 3rd severity level diagnostics. For example: static void GetNameForFile( const char* baseFileName, const uint32 fileIdx, char outputName[512] ) { assert(baseFileName != NULL); sprintf( outputName, "%s_%d", baseFileName, fileIdx ); } V576 Incorrect format. Consider checking the fourth actual argument of the 'sprintf' function. The SIGNED integer type argument is expected. igame.h 66 From the formal viewpoint, the programmer should have used %u to print the unsigned variable fileIdx. But I'm very doubtful that this variable will ever reach a value larger than INT_MAX. So this error will not cause any severe consequences. Analysis results My brief comment on the analysis results is, developers should use static analysis. There will be much fewer bugs in programs and I will drop writing articles like this one. Double check void CVehicleMovementArcadeWheeled::InternalPhysicsTick(float dt) { .... if (fabsf(m_movementAction.rotateYaw)>0.05f || vel.GetLengthSquared()>0.001f || m_chassis.vel.GetLengthSquared()>0.001f || angVel.GetLengthSquared()>0.001f || angVel.GetLengthSquared()>0.001f) .... } V501: There are identical sub-expressions 'angVel.GetLengthSquared() > 0.001f' to the left and to the right of the '||' operator. vehiclemovementarcadewheeled.cpp 3300 The angVel.GetLengthSquared()>0.001f check is executed twice. One of them is redundant, or otherwise there is a typo in it which prevents some other value from being checked. Identical code blocks under different conditions Fragment No. 1. void CVicinityDependentObjectMover::HandleEvent(....) { .... else if ( strcmp(szEventName, "ForceToTargetPos") == 0 ) { SetState(eObjectRangeMoverState_MovingTo); SetState(eObjectRangeMoverState_Moved); ActivateOutputPortBool( "OnForceToTargetPos" ); } else if ( strcmp(szEventName, "ForceToTargetPos") == 0 ) { SetState(eObjectRangeMoverState_MovingTo); SetState(eObjectRangeMoverState_Moved); ActivateOutputPortBool( "OnForceToTargetPos" ); } .... } V517: The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 255, 261. vicinitydependentobjectmover.cpp 255 I suspect that this piece of code was written through the Copy-Paste technique. I also suspect that the programmer forgot to change some lines after the copying. Fragment No. 2. The ShouldGiveLocalPlayerHitableFeedbackOnCrosshairHoverForEntityClass() function is implemented in a very strange way. That's a real name! bool CGameRules:: ShouldGiveLocalPlayerHitableFeedbackOnCrosshairHoverForEntityClass (const IEntityClass* pEntityClass) const { assert(pEntityClass != NULL); if(gEnv->bMultiplayer) { return (pEntityClass == s_pSmartMineClass) || (pEntityClass == s_pTurretClass) || (pEntityClass == s_pC4Explosive); } else { return (pEntityClass == s_pSmartMineClass) || (pEntityClass == s_pTurretClass) || (pEntityClass == s_pC4Explosive); } } V523: The 'then' statement is equivalent to the 'else' statement. gamerules.cpp 5401 Other similar defects: environmentalweapon.cpp 964persistantstats.cpp 610persistantstats.cpp 714recordingsystem.cpp 8924movementtransitions.cpp 610gamerulescombicaptureobjective.cpp 1692vehiclemovementhelicopter.cpp 588 An uninitialized array cell TDestructionEventId destructionEvents[2]; SDestructibleBodyPart() : hashId(0) , healthRatio(0.0f) , minHealthToDestroyOnDeathRatio(0.0f) { destructionEvents[0] = -1; destructionEvents[0] = -1; } V519: The 'destructionEvents[0]' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 75, 76. bodydestruction.h 76 The destructionEvents array consists of two items. The programmer wanted to initialize the array in the constructor, but failed. A parenthesis in a wrong place bool ShouldRecordEvent(size_t eventID, IActor* pActor=NULL) const; void CActorTelemetry::SubscribeToWeapon(EntityId weaponId) { .... else if(pMgr->ShouldRecordEvent(eSE_Weapon), pOwnerRaw) .... } V639: Consider inspecting the expression for 'ShouldRecordEvent' function call. It is possible that one of the closing ')' brackets was positioned incorrectly. actortelemetry.cpp 288 It's a rare and interesting bug - a closing parenthesis is written in a wrong place. The point is that the ShouldRecordEvent() function's second argument is optional. It turns that the ShouldRecordEvent() function is called first, and then the comma operator , returns the value on the right. The condition depends on the pOwnerRaw variable alone. Long story short, the whole thing is darn messed up here. A function name missing virtual void ProcessEvent(....) { .... string pMessage = ("%s:", currentSeat->GetSeatName()); .... } V521: Such expressions using the ',' operator are dangerous. Make sure the expression '"%s:", currentSeat->GetSeatName()' is correct. flowvehiclenodes.cpp 662 In this fragment, the pMessage variable is assigned the value currentSeat->GetSeatName(). No formatting is done, and it leads to missing the colon ':' in this line. Though a trifle, it is still a bug. The fixed code should look like this: string pMessage = string().Format("%s:", currentSeat->GetSeatName()); Senseless and pitiless checks Fragment No. 1. inline bool operator != (const SEfResTexture &m) const { if (stricmp(m_Name.c_str(), m_Name.c_str()) != 0 || m_TexFlags != m.m_TexFlags || m_bUTile != m.m_bUTile || m_bVTile != m.m_bVTile || m_Filter != m.m_Filter || m_Ext != m.m_Ext || m_Sampler != m.m_Sampler) return true; return false; } V549: The first argument of 'stricmp' function is equal to the second argument. ishader.h 2089 If you haven't noticed the bug, I'll tell you. The m_Name.c_str() string is compared to itself. The correct code should look like this: stricmp(m_Name.c_str(), m.m_Name.c_str()) Fragment No. 2. A logical error this time: SearchSpotStatus GetStatus() const { return m_status; } SearchSpot* SearchGroup::FindBestSearchSpot(....) { .... if(searchSpot.GetStatus() != Unreachable || searchSpot.GetStatus() != BeingSearchedRightAboutNow) .... } V547: Expression is always true. Probably the '&&' operator should be used here. searchmodule.cpp 469 The check in this code does not make any sense. Here is an analogy: if (A != 1 || A != 2) The condition is always true. Fragment No. 3. const CCircularBufferTimeline * CCircularBufferStatsContainer::GetTimeline( size_t inTimelineId) const { .... if (inTimelineId >= 0 && (int)inTimelineId < m_numTimelines) { tl = &m_timelines[inTimelineId]; } else { CryWarning(VALIDATOR_MODULE_GAME,VALIDATOR_ERROR, "Statistics event %" PRISIZE_T " is larger than the max registered of %" PRISIZE_T ", event ignored", inTimelineId,m_numTimelines); } .... } V547: Expression 'inTimelineId >= 0' is always true. Unsigned type value is always >= 0. circularstatsstorage.cpp 31 Fragment No. 4. inline typename CryStringT::size_type CryStringT::rfind( value_type ch, size_type pos ) const { const_str str; if (pos == npos) { .... } else { if (pos == npos) pos = length(); .... } V571: Recurring check. The 'if (pos == npos)' condition was already verified in line 1447. crystring.h 1453 The pos = length() assignment will never be executed. A similar defect: cryfixedstring.h 1297 Pointers Programmers are very fond of checking pointers for being null. Wish they knew how often they do it wrong - check when it's too late. I'll cite only one example and give you a link to a file with the list of all the other samples. IScriptTable *p; bool Create( IScriptSystem *pSS, bool bCreateEmpty=false ) { if (p) p->Release(); p = pSS->CreateTable(bCreateEmpty); p->AddRef(); return (p)?true:false; } V595: The 'p' pointer was utilized before it was verified against nullptr. Check lines: 325, 326. scripthelpers.h 325 The list of other 35 messages: CryEngineSDK-595.txt Undefined behavior void AddSample( T x ) { m_index = ++m_index % N; .... } V567: Undefined behavior. The 'm_index' variable is modified while being used twice between sequence points. inetwork.h 2303 One-time loops void CWeapon::AccessoriesChanged(bool initialLoadoutSetup) { .... for (int i = 0; i < numZoommodes; i++) { CIronSight* pZoomMode = .... const SZoomModeParams* pCurrentParams = .... const SZoomModeParams* pNewParams = .... if(pNewParams != pCurrentParams) { pZoomMode->ResetSharedParams(pNewParams); } break; } .... } V612: An unconditional 'break' within a loop. weapon.cpp 2854 The loop body will be executed only once because of the unconditional statement break, while there are no continue operators around in this loop. We found a few more suspicious loops like that:gunturret.cpp 1647vehiclemovementbase.cpp 2362vehiclemovementbase.cpp 2382 Strange assignments Fragment No. 1. void CPlayerStateGround::OnPrePhysicsUpdate(....) { .... modifiedSlopeNormal.z = modifiedSlopeNormal.z; .... } V570: The 'modifiedSlopeNormal.z' variable is assigned to itself. playerstateground.cpp 227 Fragment No. 2. const SRWIParams& Init(....) { .... objtypes=ent_all; flags=rwi_stop_at_pierceable; org=_org; dir=_dir; objtypes=_objtypes; .... } V519: The 'objtypes' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 2807, 2808. physinterface.h 2808 The objtypes class member is assigned values twice. Fragment No. 3. void SPickAndThrowParams::SThrowParams::SetDefaultValues() { .... maxChargedThrowSpeed = 20.0f; maxChargedThrowSpeed = 15.0f; } V519: The 'maxChargedThrowSpeed' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 1284, 1285. weaponsharedparams.cpp 1285 A few more similar strange assignments:The bExecuteCommandLine variable. Check lines: 628, 630. isystem.h 630The flags variable. Check lines: 2807, 2808. physinterface.h 2808The entTypes Variable. Check lines: 2854, 2856. physinterface.h 2856The geomFlagsAny variable. Check lines: 2854, 2857. physinterface.h 2857The m_pLayerEffectParams variable. Check lines: 762, 771. ishader.h 771 Careless entity names void CGamePhysicsSettings::Debug(....) const { .... sprintf_s(buf, bufLen, pEntity->GetName()); .... } V618: It's dangerous to call the 'sprintf_s' function in such a manner, as the line being passed could contain format specification. The example of the safe code: printf("%s", str); gamephysicssettings.cpp 174 It's not quite an error, but a dangerous code anyway. Should the % character be used in an entity name, it may lead to absolutely unpredictable consequences. Lone wanderer CPersistantStats::SEnemyTeamMemberInfo *CPersistantStats::GetEnemyTeamMemberInfo(EntityId inEntityId) { .... insertResult.first->second.m_entityId; .... } V607: Ownerless expression 'insertResult.first->second.m_entityId'. persistantstats.cpp 4814 An alone standing expression doing nothing. What is it? A bug? Incomplete code? Another similar fragment: recordingsystem.cpp 2671 The new operator bool CreateWriteBuffer(uint32 bufferSize) { FreeWriteBuffer(); m_pWriteBuffer = new uint8[bufferSize]; if (m_pWriteBuffer) { m_bufferSize = bufferSize; m_bufferPos = 0; m_allocated = true; return true; } return false; } V668: There is no sense in testing the 'm_pWriteBuffer' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. crylobbypacket.h 88 The code is obsolete. Nowadays, the new operator throws an exception when a memory allocation error occurs. Other fragments in need of refactoring:cry_math.h 73datapatchdownloader.cpp 106datapatchdownloader.cpp 338game.cpp 1671game.cpp 4478persistantstats.cpp 1235sceneblurgameeffect.cpp 366killcamgameeffect.cpp 369downloadmgr.cpp 1090downloadmgr.cpp 1467matchmakingtelemetry.cpp 69matchmakingtelemetry.cpp 132matchmakingtelemetry.cpp 109telemetrycollector.cpp 1407telemetrycollector.cpp 1470telemetrycollector.cpp 1467telemetrycollector.cpp 1479statsrecordingmgr.cpp 1134statsrecordingmgr.cpp 1144statsrecordingmgr.cpp 1267statsrecordingmgr.cpp 1261featuretester.cpp 876menurender3dmodelmgr.cpp 1373 Conclusions No special conclusions. But I wish I could check the CryEngine 3 engine itself, rather than CryEngine 3 SDK. Guess how many bugs I could find there? May your code stay bugless!
  12. This article was originally published at Unreal Engine Blog. Republished by the editors' permission. Our company develops, promotes, and sells the PVS-Studio static code analyzer for C/C++ programmers. However, our collaboration with customers is not limited solely to selling PVS-Studio licenses. For example, we often take on contract projects as well. Due to NDAs, we're not usually allowed to reveal details about this work, and you might not be familiar with the projects names, anyway. But this time, we think you'll be excited by our latest collaboration. Together with Epic Games, we're working on the Unreal Engine project. This is what we're going to tell you about in this article. As a way of promoting our PVS-Studio static code analyzer, we've thought of an interesting format for our articles: We analyze open-source projects and write about the bugs we manage to find there. Take a look at this updatable list of projects we have already checked and written about. This activity benefits everyone: readers enjoy learning from others' mistakes and discover new means to avoid them through certain coding techniques and style. For us, it's a way to have more people learn about our tool. As for the project authors, they too benefit by gaining an opportunity to fix some of the bugs. Among the articles was "A Long-Awaited Check of Unreal Engine 4". Unreal Engine's source code was extraordinarily high quality, but all software projects have defects and PVS-Studio is excellent at surfacing some of the most tricky bugs. We ran an analysis and reported our findings to Epic. The Unreal Engine team thanked us for checking their code, and quickly fixed the bugs we reported. But we didn't want to stop there, and thought we should try selling a PVS-Studio license to Epic Games. Epic Games was very interested in using PVS-Studio to improve the engine continuously over time. They suggested we analyze and fix Unreal Engine's source code so that they were completely clear of bugs and the tool wouldn't generate any false positives in the end. Afterwords, Epic would use PVS-Studio on their code base themselves, thus making its integration into their development process as easy and smooth as possible. Epic Games promised to not only purchase the PVS-Studio license, but would also pay us for our work. We accepted the offer. The job is done. And now you are welcome to learn about various interesting things we came across while working on Unreal Engine's source code. Pavel Eremeev, Svyatoslav Razmyslov, and Anton Tokarev were the participants on the PVS-Studio's part. On the Epic Game's, the most active participants were Andy Bayle and Dan O'Connor - it all would have been impossible without their help, so many thanks to them! PVS-Studio integration into Unreal Engine's build process To manage the build process, Unreal Engine employs a build system of its own - Unreal Build Tool. There is also a set of scripts to generate project files for a number of different platforms and compilers. Since PVS-Studio is first of all designed to work with the Microsoft Visual C++ compiler, we used the corresponding script to generate project files (*.vcxproj) for the Microsoft Visual Studio IDE. PVS-Studio comes with a plugin that can integrate into the Visual Studio IDE and enables a "one-click" analysis. However, projects generated for Unreal Engine are not the "ordinary" MSBuild projects used by Visual Studio. When compiling Unreal Engine from Visual Studio, the IDE invokes MSBuild when starting the build process, but MSBuild itself is used just as a "wrapper" to run the Unreal Build Tool program. To analyze the source code in PVS-Studio, the tool needs a preprocessor's output - an *.i file with all the headers included and macros expanded. Quick note. This section is only interesting if you have a customized build process like Unreal's If you are thinking of trying PVS-Studio on a project of yours that has some intricate peculiarities about its build process, I recommend reading this section to the end. Perhaps it will be helpful for your case. But if you have an ordinary Visual Studio project or can't wait to read about the bugs we have found, you can skip it. To launch the preprocessor correctly, the tool needs information about compilation parameters. In "ordinary" MSBuild projects, this information is inherent; the PVS-Studio plugin can "see" it and automatically preprocess all the necessary source files for the analyzer that will be called afterwards. With Unreal Engine projects, things are different. As I've already said above, their projects are just a "wrapper" while the compiler is actually called by Unreal Build Tool. That's why compilation parameters in this case are not available for the PVS-Studio plugin for Visual Studio. You just can't run analysis "in one click", though the plugin can be used to view the analysis results. The analyzer itself (PVS-Studio.exe) is a command-line application that resembles the C++ compiler regarding the way it is used. Just like the compiler, it has to be launched individually for every source file, passing this file's compilation parameters through the command line or response file. And the analyzer will automatically choose and call the appropriate preprocessor and then perform the analysis. Note. There's also an alternative way. You can launch the analyzer for preprocessed files prepared in advance. Thus, the universal solution for integrating the PVS-Studio analyzer into the build process is to call its exe-file in the same place where the compiler is called, i.e. inside the build system - Unreal Build Tool in our case. Sure, it will require modifying the current build system, which may not be desirable, as in our case. Because of that, just for cases like this, we created a compiler call "intercepting" system - Compiler Monitoring. The Compiler Monitoring system can "intercept" compilation process launches (in the case with Visual C++, this is the cl.exe proces), collecting all of the parameters necessary for successful preprocessing, then re-launch preprocessing for files under compilation for further analysis. That's what we did. Figure 1. A scheme of the analysis process for the Unreal Engine project Unreal Engine analysis integration comes down to calling, right before the build process, the monitoring process (CLMonitor.exe) that will make all the necessary steps to do the preprocessing and launch the analyzer at the end of the build process. To run the monitoring process, we need to run a simple command: CLMonitor.exe monitor CLMonitor.exe will call itself in "tracking mode" and terminate. At the same time, another CLMonitor.exe process will remain running in the background "intercepting" the compiler calls. When the build process is finished, we need to run another simple command: CLMonitor.exe analyze "UE.plog" Please pay attention: in PVS-Studio 5.26 and above you should write: CLMonitor.exe analyze -l "UE.plog" Now CLMonitor.exe will launch the analysis of previously-collected source files, saving the results into the UE.plog file that can be easily handled in our IDE plugin. We set a nightly build process of the most interesting Unreal Engine configurations followed by their analysis on our Continuous Integration server. It was a means for us to, first, make sure our edits hadn't broken the build and, second, to get in the morning a new log about Unreal Engine's analysis with all the edits of the previous day taken into account. So, before sending a Pull Request for submitting our edits to the Unreal Engine project repository on GitHub, we could easily make sure that the current version was stable in our repository by simply rebuilding it on the server. Non-linear bug fixing speed So, we have solved the project build process and analysis. Now let's talk about bug fixes we've done based on the diagnostic messages output by the analyzer. At first glance, it may seem natural that the number of warnings output by the analyzer should drop evenly from day to day: about the same number of messages is suppressed by certain PVS-Studio mechanisms as the number of fixes that are done in the code. That is, theoretically you could expect a graph looking somewhat like this: Figure 2. A perfect graph. The number of bugs drops evenly from day to day. In reality, however, messages are eliminated faster during the initial phase of the bug fixing process than at the later stages. First, at the initial stage, we suppress warnings triggered by macros, which helps quickly reduce the overall number of issues. Second, it happened so that we had fixed the most evident issues first and put off more intricate things until later. I can explain on this. We wanted to show the Epic Games developers that we had started working and there was a progress. It would be strange to start with difficult issues and get stuck there, wouldn't it? It took us 17 working days in total analyzing the Unreal Engine code and fixing bugs. Our goal was to eliminate all the general analysis messages of the first and second severity levels. Here is how the work progressed: Table 1. The number of warnings remaining on each day. Notice the red figures. During the first two days, we were getting accustomed to the project and then suppressed warnings in some macros, thus greatly reducing the number of false positives. Seventeen working days is quite a lot and I'd like to explain why it required this amount of time. First, it was not the whole team that worked on the project, but only two of its members. Of course, they were busy with some other tasks as well during this time. Secondly, Unreal Engine's code was entirely unfamiliar to us, so making fixes was quite a tough job. We had to stop every now and then to figure out if and how we should fix a certain spot. Now, here is the same data in the form of a smoothed graph: Figure 3. A smoothed graph of the warning numbers over time. A practical conclusion - to remember ourselves and tell others: It's a bad idea to try estimating the time it will take you to fix all the warnings based on only the first couple of days of work. It's very pacey at first, so the forecast may appear too optimistic. But we still needed to make an estimate somehow. I think there should be a magical formula for this, and hopefully we'll discover it and show it to the world someday. But presently, we are too short of statistical data to offer something reliable. About the bugs found in the project We have fixed quite a lot of code fragments. These fixes can be theoretically grouped into 3 categories: Real bugs. We will show you a few of these as an example.Not actually errors, yet these code fragments were confusing the analyzer and so they can confuse programmers who will study this code in the future. In other words, it was "sketchy" code that should be fixed as well. So we did.Edits made solely because of the need to "please" the analyzer that would generate false positives on those fragments. We were trying to isolate false warning suppressions in a special separate file or improve the work of the analyzer itself whenever possible. But we still had to do some refactoring in certain places to help the analyzer figure things out. As I promised, here are some examples of the bugs. We have picked out the most interesting defects that were clear to understand. The first interesting message by PVS-Studio: V506 Pointer to local variable 'NewBitmap' is stored outside the scope of this variable. Such a pointer will become invalid. fontcache.cpp 466 void GetRenderData(....) { .... FT_Bitmap* Bitmap = nullptr; if( Slot->bitmap.pixel_mode == FT_PIXEL_MODE_MONO ) { FT_Bitmap NewBitmap; .... Bitmap = &NewBitmap; } .... OutRenderData.RawPixels.AddUninitialized( Bitmap->rows * Bitmap->width ); .... } The address of the NewBitmap object is saved into the Bitmap pointer. The trouble with it is that right after this, the NewBitmap object's lifetime expires and it is destroyed. So it turns out that Bitmap is pointing to an already destroyed object. When trying to use a pointer to address a destroyed object, undefined behavior occurs. What form it will take is unknown. The program may work well for years if you are lucky enough that the data of the dead object (stored on the stack) is not overwritten by something else. A correct way to fix this code is to move NewBitmap's declaration outside the if operator: void GetRenderData(....) { .... FT_Bitmap* Bitmap = nullptr; FT_Bitmap NewBitmap; if( Slot->bitmap.pixel_mode == FT_PIXEL_MODE_MONO ) { FT_Bitmap_New( &NewBitmap ); // Convert the mono font to 8bbp from 1bpp FT_Bitmap_Convert( FTLibrary, &Slot->bitmap, &NewBitmap, 4 ); Bitmap = &NewBitmap; } else { Bitmap = &Slot->bitmap; } .... OutRenderData.RawPixels.AddUninitialized( Bitmap->rows * Bitmap->width ); .... } The next warning by PVS-Studio: V522 Dereferencing of the null pointer 'GEngine' might take place. Check the logical condition. gameplaystatics.cpp 988 void UGameplayStatics::DeactivateReverbEffect(....) { if (GEngine || !GEngine->UseSound()) { return; } UWorld* ThisWorld = GEngine->GetWorldFromContextObject(....); .... } If the GEngine pointer is not null, the function returns and everything is OK. But if it is null, it gets dereferenced. We fixed the code in the following way: void UGameplayStatics::DeactivateReverbEffect(....) { if (GEngine == nullptr || !GEngine->UseSound()) { return; } UWorld* ThisWorld = GEngine->GetWorldFromContextObject(....); .... } An interesting typo is waiting for you in the next code fragment. The analyzer has detected there a meaningless function call: V530 The return value of function 'Memcmp' is required to be utilized. pathfollowingcomponent.cpp 715 int32 UPathFollowingComponent::OptimizeSegmentVisibility( int32 StartIndex) { .... if (Path.IsValid()) { Path->ShortcutNodeRefs.Reserve(....); Path->ShortcutNodeRefs.SetNumUninitialized(....); } FPlatformMemory::Memcmp(Path->ShortcutNodeRefs.GetData(), RaycastResult.CorridorPolys, RaycastResult.CorridorPolysCount * sizeof(NavNodeRef)); .... } The return result of the Memcmp function is not used. And this is what the analyzer didn't like. The programmer actually intended to copy a region of memory through the Memcpy() function but made a typo. This is the fixed version: int32 UPathFollowingComponent::OptimizeSegmentVisibility( int32 StartIndex) { .... if (Path.IsValid()) { Path->ShortcutNodeRefs.Reserve(....); Path->ShortcutNodeRefs.SetNumUninitialized(....); FPlatformMemory::Memcpy(Path->ShortcutNodeRefs.GetData(), RaycastResult.CorridorPolys, RaycastResult.CorridorPolysCount * sizeof(NavNodeRef)); } .... } Now let's talk about a diagnostic message you are sure to encounter in nearly every project - so common is the bug it refers to. We are talking about the V595 diagnostic. In our bug database, it is at the top of the list regarding the frequency of its occurrence in projects (see examples). At first glance, that list is not as large as, say, for the V501 diagnostic. But it's actually because V595 diagnostics are somewhat boring and we don't write out many of them from every single project. We usually just cite one example and add a note like: And 161 additional diagnostic messages. In half of the cases, these are real errors. This is what it looks like: Figure 4. The dread of V595 diagnostic. Diagnostic rule V595 is designed to detect code fragments where a pointer is dereferenced before being checked for null. We always find some quantity of these in projects we analyze. The pointer check and dereferencing operation may be set quite far from each other within a function - tens or even hundreds of lines away, which makes it harder to fix the bug. But there are also small and very representative examples like, for example, this function: float SGammaUIPanel::OnGetGamma() const { float DisplayGamma = GEngine->DisplayGamma; return GEngine ? DisplayGamma : 2.2f; } PVS-Studio's diagnostic message: V595 The 'GEngine' pointer was utilized before it was verified against nullptr. Check lines: 47, 48. gammauipanel.cpp 47 We fixed this in the following way: float SGammaUIPanel::OnGetGamma() const { return GEngine ? GEngine->DisplayGamma : 2.2f; } Moving on to the next fragment: V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 289, 299. automationreport.cpp 289 void FAutomationReport::ClustersUpdated(const int32 NumClusters) { ... //Fixup Results array if( NumClusters > Results.Num() ) // Results.Num() ) // Results.Num() ) { for( int32 ClusterIndex = Results.Num(); ClusterIndex < NumClusters; ++ClusterIndex ) { .... Results.Add( AutomationTestResult ); } } else if( NumClusters < Results.Num() ) { Results.RemoveAt(NumClusters, Results.Num() - NumClusters); } .... } And here's a code sample to test your attentiveness. The analyzer's warning: V616 The 'DT_POLYTYPE_GROUND' named constant with the value of 0 is used in the bitwise operation. pimplrecastnavmesh.cpp 2006 /// Flags representing the type of a navigation mesh polygon. enum dtPolyTypes { DT_POLYTYPE_GROUND = 0, DT_POLYTYPE_OFFMESH_POINT = 1, DT_POLYTYPE_OFFMESH_SEGMENT = 2, }; uint8 GetValidEnds(...., const dtPoly& Poly) { .... if ((Poly.getType() & DT_POLYTYPE_GROUND) != 0) { return false; } .... } Everything looks fine at a first glance. You may think that some bit is allocated by mask and its value is checked. But it is actually just named constants that are defined in the dtPolyTypes enumeration and they are not meant for allocating any certain bits. In this condition, the DT_POLYTYPE_GROUND constant equals 0, which means the condition will never be true. The fixed code: uint8 GetValidEnds(...., const dtPoly& Poly) { .... if (Poly.getType() == DT_POLYTYPE_GROUND) { return false; } .... } A typo detected: V501 There are identical sub-expressions to the left and to the right of the '||' operator: !bc.lclusters ||!bc.lclusters detourtilecache.cpp 687 dtStatus dtTileCache::buildNavMeshTile(....) { .... bc.lcset = dtAllocTileCacheContourSet(m_talloc); bc.lclusters = dtAllocTileCacheClusterSet(m_talloc); if (!bc.lclusters || !bc.lclusters) //
  • Advertisement