2

There is a problem, coverity finds an error (potential OoB) in the place of the code where semantically this problem cannot arise. Because of the static analyzer’s message, I don’t want to make an additional check for this place in the blueprint. Is it possible to suppress this warning with code annotation?

static struct rule *Rule_sort(struct rule *rp){
  int i;
  struct rule *pNext;
  struct rule *x[32];
  memset(x, 0, sizeof(x));
  while( rp ){
    pNext = rp->next;
    rp->next = 0;
    for(i=0; i<sizeof(x)/sizeof(x[0]) && x[i]; i++){
      rp = Rule_merge(x[i], rp);
      x[i] = 0;
    }
    // potential OoB
    x[i] = rp;
    rp = pNext;
  }
  rp = 0;
  for(i=0; i<sizeof(x)/sizeof(x[0]); i++){
    rp = Rule_merge(x[i], rp);
  }
  return rp;
}
Anton Mamedov
  • 23
  • 1
  • 3
  • *where semantically this problem cannot arise* How are you so certain about that? – Andrew Henle Oct 16 '21 at 14:10
  • @AndrewHenle, Yes, I am sure. The number of cells in the array used is logarithmically dependent on the size of the input list. To overflow an array, you need a list of 2 ^ 33 elements. There is a limitation in the code before that that makes it impossible to create such a list. I would like to disable this check for this feature. – Anton Mamedov Oct 16 '21 at 14:15
  • *The number of cells in the array used is logarithmically dependent on the size of the input list.* Oh. And you're certain that code is bug-free and will always be bug-free? – Andrew Henle Oct 16 '21 at 14:58
  • @AndrewHenle, Yes, I just can't find the CWE-119 validation tag to mark it as intentional with code annotation – Anton Mamedov Oct 16 '21 at 15:11
  • You're putting more effort into trying to avoid fixing a potential buffer overflow than it would take to fix the code both to ensure there's no overflow and to return or log an error to alert everyone that there ***is*** a bug, should it ever happen. That seems ... misguided. You're relying on an unfounded belief that a particular bug "can never happen" (that's a risible assertion...) and that this code will never be used in the future in a way where it will happen. – Andrew Henle Oct 16 '21 at 21:26

1 Answers1

3

To suppress a Coverty finding with a source code annotation, add a comment to the line just before where the finding is reported of the form // coverity[event_tag] or /* coverity[event_tag] */, where event_tag is the "tag" of the event. The tag is an identifier-like word that indicates the general form of that event. See the blog post Coverity: Suppressing false positives with code annotations for a few more details.

You haven't shown the Coverity finding, but I suspect it is an OVERRUN and the tag is overrun-local. Assuming so, you could suppress it like this:

    for(i=0; i<sizeof(x)/sizeof(x[0]) && x[i]; i++){
      rp = Rule_merge(x[i], rp);
      x[i] = 0;
    }
    /* coverity[overrun-local] */
    x[i] = rp;
    rp = pNext;

With this annotation, the next time the analysis runs and its results committed to the database, the finding will be automatically marked "Intentional", and hence it will not appear in the default list of findings or other common reports.

If you're not sure where to locate the tag for a finding, see How do I get the event tags for a Coverity issue?

Having explained how to suppress the finding, in this particular case I would advise against it. I think the code quoted in the question is not safe, as the inner for loop condition i<sizeof(x)/sizeof(x[0]) seems to be wrong. That is equivalent to i < 32 here. If that condition could ever be false, it would mean i was equal to 32, and hence the loop would terminate with i==32, and therefore overrun the array by one element.

Put another way, if you ever get into the inner for loop while i==31, the array will be overrun because i will then always be incremented to 32 and x[i] written.

Even if there was nothing wrong per se with the code, I'd still be hesitant to simply suppress the finding. Think of a static analysis finding like a concern raised by a colleague: even if the concern is not indicative of a bug, take it as an opportunity to improve the clarity and robustness of the code. In a case like this, if the code was correct but Coverity complained, I would lean towards adding an assert rather than outright suppression.

Scott McPeak
  • 8,803
  • 2
  • 40
  • 79
  • Thanks. Please tell me which tag for the ARRAY_VS_SINGLETON problem. Where can I see a list of tags for problems? – Anton Mamedov Oct 17 '21 at 10:13
  • The tag names appear in the event list in the web-based GUI for each individual finding. If I recall correctly they are also listed in the checker reference documentation (which is accessible from the GUI). – Scott McPeak Oct 17 '21 at 10:23
  • It would be great to add more info how to find this `tag name`. I don't see it from web UI... – The Godfather May 06 '22 at 11:40
  • @TheGodfather I just posted a Q&A for finding the tag. See link edited into the answer. – Scott McPeak May 06 '22 at 20:49