cpp:S2259 false positive

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

Context

Observed behavior

Rule cpp:S2259 marks a certain pointer as being accessed while null, but it is not.

Steps to reproduce

I tried to extract the minimal code that should reproduce the issue from our production code, but couldn’t test it directly, please let me know if you have issues reproducing it:

struct DataObject {
    int content = 0;
};

struct DataSource {
    std::string name;
    DataObject value;
};

struct DataTarget {
    DataObject* property1 = nullptr;
    DataObject* property2 = nullptr;
    DataObject* property3 = nullptr;
};

void readAttribute(std::map<std::string, bool>& attributeFound, const DataSource& ds, const std::string& targetName, DataObject*& out) {
    if (ds.name != targetName) {
        return;
    }
    if (attributeFound[ds.name]) {
        return;
    }

    out = &ds.value;
    attributeFound[ds.name] = true;
}

void demoFunction() {
    // This vector is being filled by some other function gethering data in the actual code
    // and element order in this vector is undetermined
    std::vector<DataSource> datasources = {{"ds_property2", {42}}, {"ds_property3", {43}}, {"ds_property1", {41}}};
    std::map<std::string, bool> attributeFound = {
        {"ds_property1", false},
        {"ds_property2", false},
        {"ds_property3", false},
    };

    DataTarget dataTarget;

    // Browse all data sources and try to find the properties we need
    for (const DataSource& ds : datasources)
    {
        readAttribute(attributeFound, ds, "ds_property1", dataTarget.property1);
        readAttribute(attributeFound, ds, "ds_property2", dataTarget.property2);
        readAttribute(attributeFound, ds, "ds_property3", dataTarget.property3);
    }

    if (attributeFound["ds_property1"]) {
        // Here cpp:S2259 triggers on dataTarget.property1->content of the line below, which is wrong
        std::cout << "Property 1 = " << dataTarget.property1->content << std::endl; 
    }
}
5

Hi, @FlorentP42!

It is indeed a false positive, thank you for reporting it!

Unfortunately, our analyzer cannot properly reason about standard containers.
Simplifying your example even further you can find that even these two cases are reported incorrectly:

#include <map>

int demoFunction1(std::map<int, bool> propertyFound) {
    int* ptr = nullptr;
    propertyFound[1] = false;
    if (propertyFound[1]) { // Assuming propertyFound[1] true even though it was set to false above
        return *ptr; // S2259 false positive
    }
    return 0;
}

int demoFunction2(std::map<int, bool> propertyFound, int refVal) {
    int* ptr = nullptr;
    if (propertyFound[1]) { // Assuming propertyFound[1] is false
        ptr = &refVal;
    }
    if (propertyFound[1]) { // Assuming propertyFound[1] is now true
        return *ptr; // S2259 false positive
    }
    return 0;
}

As you can see, even for a map int → bool, our analyzer can assume impossible facts.

We are aware of this limitation and would like to remove it: CPP-3608 Model behavior of the standard containers. I linked your thread to this ticket.

Unfortunately, I cannot promise you when we will be able to tackle it, but I invite you to follow the ticket.

Let me know if you have more questions.

7

All right, good to know this is a know issue already tracked, I guess I will keep this open until a proper fix is found then :slight_smile: