>
>
>
"Our legacy of the past" or w…

Mikhail Gelvikh
Articles: 12

"Our legacy of the past" or why we divided the V512

As the saying goes, the first step is always the hardest. That's exactly what happened in our case – after delaying it for so long, we have finally split the V512 diagnostic rule. You can read more about the causes and consequences in this little note.

We originally implemented one of the first general analysis (GA) diagnostics, V512, with certain imperfections. Once upon a time (in 2013, according to the documentation), this diagnostic covered buffer overflow and buffer underflow errors. We've known for a while that this was incorrect. However, the diagnostic rule functioned well, so changing its behavior seemed overreaching. So that's how it was.

Then why change anything, you may ask? But there is a reason, and a pretty good one. From time to time, customers contacted our support to point out that V512 unintentionally combines two almost contradictory diagnostic rules. And the certainty level of buffer underflow warnings is much lower than that of the buffer overflow warnings. Moreover, buffer underflow warnings are almost always false positives. Here's a recent case:

"The buffer underflow warning is absolutely harmful. For example, the function code: we create a buffer on the stack (1KB). Next, if the input argument is 1, we copy 100 bytes into that buffer from one place. Otherwise, we copy 1KB from another place. Then we output the converted buffer contents (of course, taking into account the length of what was copied to the buffer). And then PVS-Studio issues a buffer underflow warning because you've copied 100 bytes. So, who is wrong?".

Generally, people only want to see warnings about buffer overflow and almost never about buffer underflow. And different certainty levels don't help much. Customers need an easy way to turn off the buffer underflow diagnostic rule. We needed to separate the buffer overflow diagnostic rule to ensure that customers could keep getting helpful buffer overflow warnings without being distracted by unneeded buffer underflow warnings.

So, we had to do something about it, and for some reason we decided... No, not to split up the diagnostics. Not yet :) We only decided to implement a special comment –//‑V512_UNDERFLOW_OFF, which disables the buffer underflow detection. Equally, we implemented a similar comment to disable the buffer overflow detection –//‑V512_OVERFLOW_OFF. We wonder if anyone has ever used the last one...

It all seemed like some kind of a quick and dirty fix to overcome the problem. Just slap some duct tape on it, why not! But now, sadly, no one can remember (or does not want to confess) why this particular decision was taken. Nevertheless, we bypassed solved the problem, updated the documentation, and decided that everyone would be happy. But instead, customers continued to request support. So, what was the matter?

I'm unlikely to surprise you now, but in fact, people commonly look at the documentation only if something goes wrong. Well, since people kept getting warnings for correct code, they kept contacting our support. We had to regularly explain that the behavior was expected. We recommended the use of our special comments to suppress the warnings.

Eventually, we got a bit fed up with all these ongoing explanations, so we finally decided to split the diagnostic rules: the V512 was to be kept for buffer overflow errors, and the V1086 was to be the new one for buffer underflow errors. To avoid any confusion, we renamed them. From now on, the diagnostic rules are called as follows:

  • V512. Call of the 'Foo' function will lead to buffer overflow.
  • V1086. Call of the 'Foo' function will lead to buffer underflow.

However, even then, some oddities remained. In order to maintain backward compatibility, we had to keep supporting the once-built special comments. The //‑V512_OVERFLOW_OFF will suppress the V512, and //‑V512_UNDERFLOW_OFF will suppress the V1086. Well, that was our "legacy of the past".