False positive for "Null pointers should not be dereferenced" in C when memory explicitly set to 0

Clean Code Report False-positive / False-negative...
1

versions used: SonarQube Scanner 4.2.0.1873

Code sample:

false_positives.c

#include <stdio.h>
#include <stdlib.h>
#include "false_positives.h"

static void* allocate_and_set_to_zeros(size_t size) {
    uint8_t* memory = (uint8_t*)malloc(size);
    if (memory == NULL) {
        return NULL;
    }
    for (size_t i = 0; i < size; i++) {
        memory[i] = 0;
    }
}

void s1_destroy(S1* self) {
    if (self == NULL) {
        return;
    }

    for (unsigned int i = 0; i < self->item_count; i++) {
        printf("%d\n", self->items[i]);
    }

    if (self->items) {
        free(self->items);
    }
    free(self);
}

S1* s1_create(void) {
    S1* s1 = allocate_and_set_to_zeros(sizeof(S1));
    if (s1 == NULL) {
        return NULL;
    }

    unsigned int item_count = 3;
    s1->items = allocate_and_set_to_zeros(item_count * sizeof(s1->items[0]));
    if (s1->items == NULL) {
        s1_destroy(s1);
        return NULL;
    }
    s1->item_count = item_count;

    return s1;
}

false_positives.h

typedef struct {
    int* items;
    unsigned int item_count;
} S1;

void s1_destroy(S1* self);
S1* s1_create(void);

The analyzer does not “see” that zero’ing the memory sets the item_count field to 0, and thus incorrectly believes that the loop on lines 20-22 can be entered when the allocation of items fails.

3

Hello @barbibulle,

Thanks for reporting this false-positive. I can reproduce it. The problem here is that you are setting the item_count to 0 indirectly using pointer arithmetic which is hard to detect. If you set item_count to 0 explicitly the issue will disappear. I created this ticket to handle this issue.

4

Hi @Abbas, thanks for the quick followup. I can definitely set item_count to 0 “manually”, but that would be quite problematic for the code base I’m working on, where we use this sort of “init to 0” pattern in object constructors: it initializes all objects to 0 by just zero’ing out the memory after allocation (typically calling calloc internally), which saves a lot of boiler-plate code (manually setting each field to 0 explicitly), which is verbose, increases the code size, and is hard to maintain.

5

Hello @barbibulle, I wasn’t suggesting that you should set it manually. I was just pointing out the reason for the false positive in our analyzer.
The issue should be handled on our side. You can watch the ticket to be notified when it is fixed.

As a workaround, why don’t you use calloc for such a pattern? Our rule can detect that item_count is set to 0 when calloc is used. for example, the false positive will be avoided if allocate_and_set_to_zeros is implemented in this equivalent way:

static void* allocate_and_set_to_zeros(size_t size) {
    return calloc(1, size);
}
6

In my project, the implementation of the (equivalent of) allocate_and_set_to_zeros does call calloc, but somehow the scanner doesn’t see that. It isn’t in a static function in the same file, but in a library that’s compiled as part of the project (which itself is visible to the scanner)

7

@barbibulle This is actually one of the current limitations of this rule. We don’t share information between translation units; functions coming from other translation units are like a black box. We cannot detect if calloc is called unless the definition ends up in the same translation unit. We are working on this, but it is going to take some time.

8

Sounds good. Thanks for the explanation.