Jump to content
  • Advertisement

PVS-studio team

Member
  • Content Count

    3
  • 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.

  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. PVS-studio team

    Not All is Fine in the Morrowind Universe

    Guys, if you have suggestions about the projects that can be checked by our analyzer, please write to us! We are ready to fight:). We have started supporting C# from the 6.00 version, so C# open-source projects are also welcome.
  10. PVS-studio team

    Not All is Fine in the Morrowind Universe

    Thank you for pointing to a new repository. We will check the new code very soon.    I have some news regarding the new source code. We scanned it and found very minor errors. So, congratulations to OpenMW! 
  11. 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.
  12. PVS-studio team

    Not All is Fine in the Morrowind Universe

    Thank you for the feedback! We reqularly check PVS-Studio with the help of PVS-Studio. So, you can be sure, it has no errors:)  Also, we has an article that desribes how we test our tool http://www.viva64.com/en/a/0047/#ID0EMQAE
  13. PVS-studio team

    Not All is Fine in the Morrowind Universe

    Thank you for the feedback. We will be more accurate next time and ask the editor to check our article once more. Also, I will tell you a small story, that I hope you will find funny. The word "blatant" if you transliterate it in Russian sounds like the word from the jail world. This word means a person/group that is conntected with criminality and has some respect. I am not a linguist but it looks luke this word has interesting etiminology:)
  • Advertisement
×

Important Information

By using GameDev.net, you agree to our community Guidelines, Terms of Use, and Privacy Policy.

We are the game development community.

Whether you are an indie, hobbyist, AAA developer, or just trying to learn, GameDev.net is the place for you to learn, share, and connect with the games industry. Learn more About Us or sign up!

Sign me up!