Why PVS-Studio doesn’t offer automatic code edits

The PVS-Studio static analyzer detects rather complex and tricky code fragments containing errors. And how to fix them is not always clear even to a person, and now we will consider a couple of examples. Therefore, it is best not to generate any assumptions about automatic code fixing at all.

Sometimes programmers who start trying PVS-Studio ask: why doesn’t the tool offer to automatically fix the error? Interestingly, users no longer ask this question. After using the analyzer for some time, they realize that for the vast majority of detected errors, no automatic replacement is possible. At least until artificial intelligence is invented :).

The reason is that PVS-Studio is not a code style analyzer. It doesn’t suggest formatting or naming changes. He does not suggest (at least at the time of this writing 🙂 to replace everything in C ++ code NULL on nullptr… While this is a good suggestion, it has almost nothing to do with troubleshooting.

PVS-Studio detects errors and potential vulnerabilities. Many errors are thought-provoking and require a change in program behavior. And only a programmer can decide how to fix this or that error.

Having found an error, the analyzer will most likely suggest simplifying the code so that the anomaly disappears, but this will not fix the error itself. Understanding what the code is actually supposed to do and suggesting a meaningful, helpful fix is ​​very difficult.

Consider the error that I analyzed in the article “31 february“.

static const int kDaysInMonth[13] = {
  0, 31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31
};

bool ValidateDateTime(const DateTime& time) {
  if (time.year < 1 || time.year > 9999 ||
      time.month < 1 || time.month > 12 ||
      time.day < 1 || time.day > 31 ||
      time.hour < 0 || time.hour > 23 ||
      time.minute < 0 || time.minute > 59 ||
      time.second < 0 || time.second > 59) {
    return false;
  }
  if (time.month == 2 && IsLeapYear(time.year)) {
    return time.month <= kDaysInMonth[time.month] + 1;
  } else {
    return time.month <= kDaysInMonth[time.month];
  }
}

The analyzer understands that both tests are true. But why, the analyzer does not understand. He knows nothing about days, months and other entities. And to teach to understand such oh, how difficult it is. The only thing that can really be done is for the analyzer to suggest simplifying the function:

bool ValidateDateTime(const DateTime& time) {
  if (time.year < 1 || time.year > 9999 ||
      time.month < 1 || time.month > 12 ||
      time.day < 1 || time.day > 31 ||
      time.hour < 0 || time.hour > 23 ||
      time.minute < 0 || time.minute > 59 ||
      time.second < 0 || time.second > 59) {
    return false;
  }
  if (time.month == 2 && IsLeapYear(time.year)) {
    return true;
  } else {
    return true;
  }
}

Or, what can we say on trifles, let him offer such an automatic replacement:

bool ValidateDateTime(const DateTime& time) {
  if (time.year < 1 || time.year > 9999 ||
      time.month < 1 || time.month > 12 ||
      time.day < 1 || time.day > 31 ||
      time.hour < 0 || time.hour > 23 ||
      time.minute < 0 || time.minute > 59 ||
      time.second < 0 || time.second > 59) {
    return false;
  }
  return true;
}

Cool, but pointless;). The analyzer removed the code that is redundant from the point of view of the C ++ language. And only a person can understand if the code is really redundant (but this also often happens), or there is a typo in the code and you need to replace month on day…

The reader may say that I am thickening and automatic replacement is appropriate. No. People are mistaken in this, what can you want from a soulless program. Look, there is an interesting example of a manual inattentive edit that doesn’t actually fix anything. Since a person cannot, neither can a program.

In August of this viral year, I wrote article about checking the PMDK library. Among other things, the article considered the error of incorrect overflow protection:

static DWORD
get_rel_wait(const struct timespec *abstime)
{
  struct __timeb64 t;
  _ftime64_s(&t);
  time_t now_ms = t.time * 1000 + t.millitm;
  time_t ms = (time_t)(abstime->tv_sec * 1000 +
    abstime->tv_nsec / 1000000);

  DWORD rel_wait = (DWORD)(ms - now_ms);

  return rel_wait < 0 ? 0 : rel_wait;
}

Variable times rel_wait is of unsigned type, then subsequent check rel_wait <0 doesn’t make sense. PVS-Studio warning: V547 [CWE-570] Expression ‘rel_wait <0' is always false. Unsigned type value is never <0. os_thread_windows.c 359Someone was inspired by the article and began to massively correct the errors described in it: Fix various issues reported by PVS-Studio analysis…

And how was it suggested to fix the code? Quite ingenuous: core: simplify windows timer implementation…

But the code was simplified, not fixed! This was noticed and a corresponding discussion began: ISSUE: os_thread_windows.c – get_rel_wait () will block if abstime is in the past…

As you can see, even people make mistakes in the suggested edits. Why try robots.

Anyway, the desire to automatically correct errors is a very strange desire. Every change that fixes a bug requires attention and code review. Moreover, the analyzer can give false positives, which means that you cannot edit this at all. Analyzing your code and dealing with warnings is not the place to rush. It is better to implement regular code analysis and slowly fix bugs that appear in new code.

If you want to share this article with an English-speaking audience, please use the translation link: Andrey Karpov. Why PVS-Studio Doesn’t Offer Automatic Fixes.