New diagnostic rules in PVS-Studio 7.34

• C++
  • V1116
  • V1117 [For the C language]
  • V2022 [For the C language]
  • V5014 [OWASP Standard]
• C#
  • V3207
  • V3208 [Unity Engine]
  • V3209 [Unity Engine]
  • V3210 [Unity Engine]
  • V4007 [Unity Engine]
  • One more thing...
• Java
  • V6123
  • V6124
  • V6125
  • V5309 [OWASP standard]

The PVS-Studio 7.34 release has introduced a bunch of new diagnostic rules into the analyzer: taint analysis for Java, Unity-specific diagnostic rules for C#, deep dive into OWASP, and much more! This article will cover them all.

C++

In this release, the C++ team focused on General-Analysis diagnostic rules and the support for various software development standards.

But hold onto your hat, this is just the beginning! The team plans to cover even more MISRA standard diagnostic rules, so stay tuned for more news :)

And for now, let's go through the main rules in the 7.34 release.

V1116

Creating an exception object without an explanatory message may result in insufficient logging.

This diagnostic rule is designed to detect exceptions created without explanatory messages.

The absence of a message may hinder the process of error detection and fixing, as well as the overall code readability.

Here is an example of code that makes the PVS-Studio analyzer generate a warning:

void SomeCheck(const char *val)
{
  if (!val) throw std::runtime_error { "" };
  ....
}

void Foo()
{
  const char *val = ....;
  try
  {
    SomeCheck(val);              // <=
  }
  catch(std::runtime_error &err)
  {
    std::cerr << err.what() << std::endl;
  }
}

If an error occurs, the SomeCheck function throws an exception with an empty message, which will be handled in the Foo function. During processing, std::cerr is expected to contain information about the reason for the exception, but it does not.

By writing code in this way, a developer sends wishes of "happy debugging" to colleagues. This impedes the understanding what exactly caused the failure.

The rule works for a standard exception. You can use the custom annotation mechanism to issue warnings for custom exceptions.

V1117 [For the C language]

The declared function type is cv-qualified. The behavior when using this type is undefined.

This diagnostic rule applies only to the C language.

It aims to detect cases of function type definitions that use const or volatile qualifiers.

According to the C23 standard (10th point of paragraph 6.7.4.1), using these types leads to undefined behavior.

An example of code that makes the PVS-Studio analyzer generate a warning:

typedef int fun_t(void);

typedef const fun_t const_qual_fun_t;          // <=

typedef const fun_t * ptr_to_const_qual_fun_t; // <=

void foo()
{
  const fun_t c_fun_t;       // <=
  const fun_t * ptr_c_fun_t; // <=
}

V2022 [For the C language]

Implicit type conversion from integer type to enum type.

Another diagnostic rule for the C language that can help when refactoring and debugging.

This rule enables the analyzer to detect implicit casts of integer types to enum types.

A code example with the PVS-Studio warning:

Orientation GetOrientation (bool b)
{
  int posOne = 1;
  int posTwo = 2;
  return b ? posOne : posTwo;    // V2022
}

This code uses the conditional operator (?:) to choose between two integer variables posOne and posTwo, resulting in an implicit cast.

V5014 [OWASP Standard]

OWASP. Cryptographic function is deprecated. Its use can lead to security issues. Consider switching to an equivalent newer function.

Here's a new diagnostic rule focused on security, aligned with SAST principles.

This rule was designed according to the OWASP security verification standard.

It aims to detect calls of obsolete cryptographic functions. Their use can cause critical software security problems.

A code example with the PVS-Studio warning:

BOOL ImportKey(HCRYPTPROV hProv, LPBYTE pbKeyBlob, DWORD dwBlobLen)
{
  HCRYPTKEY hPubKey;
  if (!CryptImportKey(hProv, pbKeyBlob, dwBlobLen, 0, 0, &hPubKey))
  {
    return FALSE;
  }
  if (!CryptDestroyKey(hPubKey))
  {
    return FALSE;
  }
  return TRUE;
}

According to Microsoft documentation, the CryptoImportKey and CryptoDestroyKey functions are deprecated. These should be replaced with secure counterparts from Cryptography Next Generation (BCryptoImportKey and BCryptoDestroyKey).

But that's just a warm up! The C and C++ team plans to cover even more diagnostic rules on various software development standards. Special attention will be paid to the MISRA standard. So, wait for the news :)

C#

In the new PVS-Studio 7.34 release, the C# team focused on creating Unity-specific diagnostic rules but didn't forget about General-Analysis rules either.

Let's start with the latter.

V3207

The 'not A or B' logical pattern may not work as expected. The 'not' pattern is matched only to the first expression from the 'or' pattern.

This new diagnostic rule aims to detect incorrect use of the not A or B pattern. The problem stems from developers' confusion about the operation precedence.

A code example with the PVS-Studio warning:

private void ShowWordDetails(string key)
{
  if (key is not "" or null)
  {
    PanelReferenceBox.Controls.Clear();

    CurrentWord = Words.Find(x => x.Name == key);
    ....
  }
}

At the beginning of the method, the input parameter key is checked for an empty string or null.

But there is an error in the logic of the conditional expression. The priority of the not operator is higher than that of the or operator. As a result, negation doesn't apply to the right-hand side of the expression. Also, if key is set to null, the condition will be true.

V3208 [Unity Engine]

Unity Engine. Using 'WeakReference' with 'UnityEngine.Object' is not supported. GC will not reclaim the object's memory because it is linked to a native object.

This is the first diagnostic rule in a new series of Unity-specific rules.

It aims at detecting uses of UnityEngine.Object (or other objects inherited from it) together with System.WeakReference.

Due to the implicit use of the instance by the engine itself, the behavior of a weak reference may differ from what is expected.

A code example with the PVS-Studio warning:

WeakReference<GameObject> _goWeakRef;
void UnityObjectWeakReference()
{
  var go = new GameObject();
  _goWeakRef = new WeakReference<GameObject>(go);
}

In the example, we can see a weak reference to an object of the GameObject class. Even if an author hasn't created strong references to this object, the garbage collector will not be able to clean it up.

V3209 [Unity Engine]

Unity Engine. Using await on 'Awaitable' object more than once can lead to exception or deadlock, as such objects are returned to the pool after being awaited.

In another diagnostic rule for Unity, the analyzer searches for places with multiple uses of the same UnityEngine.Awaitable object with the await operator.

Objects are stored in an object pool for optimization purposes.

On await-call, the Awaitable object is returned to the pool. After that if await is applied to the same object again, we get an exception. In some cases, a deadlock is also possible.

A code example with the PVS-Studio warning:

async Awaitable<bool> AwaitableFoo() { .... }

async Awaitable ExampleFoo()
{
  Awaitable<bool> awaitable = AwaitableFoo();
  if (await awaitable)
  {
    var result = await awaitable;
    ....
  }
}

In this code, we get an exception or a deadlock. Let me explain why. We get a value using theawait call of awaitable. Then we initialize the result variable with this value. The deadlock occurs, as await has previously been applied to awaitable in a conditional construction.

V3210 [Unity Engine]

Unity Engine. Unity does not allow removing the 'Transform' component using 'Destroy' or 'DestroyImmediate' methods. The method call will be ignored.

This diagnostic rule aims at detecting anomalies related to calls of the Destroy or DestroyImmediate methods of the UnityEngine.Object class.

The problem occurs in a situation when an argument of the UnityEngine.Transform type is used. This causes an error during the method call. Removing the Transform component from a game object is not allowed in Unity.

A code example with the PVS-Studio warning:

using UnityEngine;

class Projectile : MonoBehaviour
{
  public void Update() 
  {
    if (....)
    {
      Destroy(transform);
    }
    ....
  }
}

The transform property from the MonoBehaviour base class returns an instance of the Transform class, which is passed as an argument to the Destroy method.

When calling the method in this way, Unity will give an error message, but the component itself won't be destroyed.

V4007 [Unity Engine]

Unity Engine. Avoid creating and destroying UnityEngine objects in performance-sensitive context. Consider activating and deactivating them instead.

This diagnostic rule targets a different range of errors—performance issues.

The purpose of this rule is to help the analyzer detect the creation of Unity objects in a frequently executed method.

Regular creation/destruction of game objects not only loads the CPU, but also leads to an increased frequency of garbage collector calls. This affects the performance.

A code example with the PVS-Studio warning:

CustomObject _instance;

void Update()
{
  if (....)
  {
     CreateCustomObject();
     ....
  }
  else if (....)
  {
    ....
    Destroy(_instance.gameObject);       // <=
  }
}

void CreateCustomObject()
{
  var go = new GameObject();             // <=
  _instance = go.AddComponent<CustomObject>();
  ....
}

Here in the Update method, a game object _instance is created and destroyed. Since Update is executed every time the frames are updated, it is recommended to avoid these operations in it if possible.

By the way, other Unity-diagnostics are yet to come! Get ready for good news from our team :)

One more thing...

We cannot but tell you about one important enhancement in the C# analyzer—tracking changes of the method return value between calls. What does it change? Let's break it down.

Check out this example:

void Example()
{
  if (Foo() != null)
  {
    var value = Foo().Value;
  }
}

Value Foo()
{
  if (_condition)
  {
    ....
    return value;
  }
  return null;
}

The Example() method checks the return value of Foo() for null. The Foo() method is then called again in the body of the condition, and its return value is dereferenced.

Earlier, the analyzer would generate a warning in this case because it didn't consider the context of an invocation, focusing only on the code of its declaration. The analyzer used to imply that null could be returned.

Now the analyzer understands that Foo() returns the same value in both cases and there will be no warning.

But let's look at an example with slightly modified code...

void Example()
{
  if (Foo() != null)
  {
    _condition = false;      // <=
    var value = Foo().Value;
  }
}

Value Foo()
{
  if (_condition)
  {
    ....
    return value;
  }
  return null;
}

From the Foo() method declaration, we can get that when _condition == true, the method returns not null.

The analyzer will see the _condition field change before the second invocation and will make an assumption: if the field used inside Foo() has changed, the return value of Foo() might have changed too.

As a result, warnings of a potential dereference will remain.

Java

With the release of PVS-Studio 7.34, the Java analyzer now has a mechanism for taint analysis!

This mechanism became the basis for the first diagnostic rule—search for SQL injections. Future updates of the Java analyzer will focus on SAST, the OWASP Top-10 list of the most common potential vulnerabilities, and other taint-related diagnostic rules.

Right now, let's start with some new General Analysis rules, as they are also worthwhile.

V6123

Modified value of the operand is not used after the increment/decrement operation.

This new diagnostic rule highlights areas in code where values of postfix operations are not used.

The problem is that either an operation is redundant or, more seriously, operations got mixed up and a developer wanted to use the prefix one.

A code example with the PVS-Studio warning:

int calculateSomething() {
  int value = getSomething();
  ....
  return value++;
}

The ++ operator won't affect the value that the calculateSomething method will return.

V6124

Converting an integer literal to the type with a smaller value range will result in overflow.

As you can see from the name of this diagnostic rule, it detects possible overflow.

A code example with the PVS-Studio warning:

public static void test() {
  byte a = (byte) 256;       // a = 0
  short b = (short) 32768;   // b = -32768
  int c = (int) 2147483648L; // c = -2147483648
}

An integer type variable has been assigned a value out of the valid range, which will cause an overflow.

Variables will obviously store different values than the ones the developer tried to assign.

V6125

Calling the 'wait', 'notify', and 'notifyAll' methods outside of synchronized context will lead to 'IllegalMonitorStateException'.

This diagnostic helps identify synchronization problems.

A code example with the PVS-Studio warning:

public void someMethod() {
    notify();
}

public void anotherMethod() throws InterruptedException {
    wait();
}

The analyzer spots wait, notify, and notifyAll methods, as they may be called in an unsynchronized context. They operate with the monitor of the object by which the synchronization occurs. That is, their invocation is correct only in the synchronized context and only on the object by which synchronization occurs.

If wait, notify or notifyAll methods are called in an unsynchronized context or on the wrong object, we get the IllegalMonitorStateException exception.

V5309 [OWASP standard]

OWASP. Possible SQL injection. Potentially tainted data is used to create SQL command.

The first taint-related diagnostic rule of the Java analyzer! More specifically, the analyzer can now detect potential SQL injections.

SQL injection is a vulnerability that allows an attacker to inject their code into an SQL query. If the query uses external data, without validating it correctly, one risks the integrity and confidentiality of the information stored in a database.

@Autowired
private JdbcTemplate template;

@GetMapping("/get_all_secret_data")
public void getEndpoint(@RequestParam String param) {
    var sql = "SELECT * FROM Users WHERE id = '" + param + "'";
    template.execute(sql);
    ....
}

In case the user turns out to be malicious and the value of param is approximately the following:- "111' or 1=1; drop table users; select ' ",—you can say goodbye to the users table. Therefore, it is important to check the external data.

Thanks for reading!

If you have requests for articles or questions, don't hesitate to send them via the feedback form. Last but not least, we'd love to hear your thoughts in the comments:)