Unicorn with delicious cookie
Our website uses cookies to enhance your browsing experience.
Accept
to the top
>
>
>
How PVS-Studio prevents rash code chang…

How PVS-Studio prevents rash code changes, example N2

Jan 18 2022
Author:

When developers do make mistakes, it's often accidental or because the developers are in a hurry. These errors often make their way into small edits to the code. Let's review one of these cases: a developer fixes an error and introduces a new one simultaneously.

Actually, the image above already demonstrates everything. You do not even need to read any further :). However, I would still like to talk about what we see here, so that's what I'm going to do.

Back in 2021, I started monitoring Blender - an open-source project - for errors. I employed the PVS-Studio analyzer to scan it and find errors. Since Blender is developing at a quick pace, I was getting PVS-Studio's warnings to new code quite frequently. Unfortunately, I could not react to them in a timely manner, because I was busy with everything else. Lots of meetings, one article after the other - and some else in between. So I basically missed most of these warnings :( . As a result, during that year, I only posted a couple of notes about fresh errors - though I could have written more.

Yesterday I noticed a new message with two warnings. So, I thought, why not take a look? Especially since I had a couple of minutes. One warning was not all that interesting, while the second one was quite a find. It was definitely a sign for me to focus and write how PVS-Studio can detect errors if used regularly :).

So, @Antonioya committed two new lines that were intended to fix the following bug: Fix T94903: GPencil: Copying keys doesn't preserve Keyframe Type.

The developer was in a hurry and never noticed that the pointer he worked with could be null. The project's code contains a nullptr check that proves it:

gpf->key_type = gpfs->key_type;
if (gpf) {

In its turn, the PVS-Studio analyzer detected an anomaly and issued a warning: V595 [CWE-476]: The 'gpf' pointer was utilized before it was verified against nullptr. Check lines: 458, 459. editaction_gpencil.c

And that's a sure way to notice many mistakes and save time one'd spend fixing them. It's way easier for that same developer to come back to the code they wrote and fix it than for the company for fix this bug when it gets to QA or users.

P.S. First I wanted to simply name the article "How PVS-Studio prevents rash code changes", but then I discovered we already have an article with this name. So I added "example N2". I hope that with time we will write more and more of such articles. Thank you for your time - and have a shot at incorporating PVS-Studio into your development process!

Popular related articles

Subscribe

Comments (2)

antonioya 
01/18/2022, 15:50:27

Thanks for catch up the bug. Yes, I was fixing other bug and received this bug and fixed and did not notice the possible error. Really in this case the gpf pointer will have a valid value in 99,9999...% I have moved the code inside the `if`, but maybe, the null checking could be removed.

About compiler and warning tools, they are really great, but the problem with Visual Studio warning system mis that if you enabled it, you get hundreds or maybe more warnings, and the "real" warnings are missed between a lot of useless warnings, so it is not very practical.

Reply

Andrey Karpov 
01/18/2022, 21:23:23

I understand what you’re talking about. It can be difficult to enable more warnings in the compiler or start using a static code analyzer. All such tools inevitably generate false positives and it is sometimes unclear what to do with all of them.

However professional tools, such as PVS-Studio, differ favorably from compiler warnings or classic linters. These tools take into account such issues.

Firstly, PVS-Studio is not that "furious", it has a lot of settings for diagnostics.

Secondly, there are various ways to specifically suppress warnings, or, for example, mark macros as safe.

Thirdly, and most importantly, there is a thoughtful way to introduce the analyzer into the development process. There is a special mode when we leave technical debt for later and work only with warnings issued for new or changed code. Actually, this is how I noticed this error, and did not look through 100500 warnings :). How it all works is written in detail in the article "How to introduce a static code analyzer in a legacy project and not to discourage the team".

Reply

close comment form
close form

Fill out the form in 2 simple steps below:

Your contact information:

Step 1
Congratulations! This is your promo code!

Desired license type:

Step 2
Team license
Enterprise license
** By clicking this button you agree to our Privacy Policy statement
close form
Request our prices
New License
License Renewal
--Select currency--
USD
EUR
* By clicking this button you agree to our Privacy Policy statement

close form
Free PVS‑Studio license for Microsoft MVP specialists
* By clicking this button you agree to our Privacy Policy statement

close form
To get the licence for your open-source project, please fill out this form
* By clicking this button you agree to our Privacy Policy statement

close form
I want to join the test
* By clicking this button you agree to our Privacy Policy statement

close form
check circle
Message submitted.

Your message has been sent. We will email you at


If you do not see the email in your inbox, please check if it is filtered to one of the following folders:

  • Promotion
  • Updates
  • Spam