v.hull: Dereference after Null check

[ShubhamDesai]

This pull request addresses the issue identified by coverity scan (CID: 1207412)

• if (edges) is added before e = edges->next; and entering the loop to ensure that it is not NULL before accessing next member.
• before accessing the member e->delete check whether e is not NULL.
• while (e != edges && e != NULL); here added check e!=NULL to ensure that loop terminates as soon as e is NULL.

[nilason]

If I understand the static analyser's reasoning correctly:

while (edges && edges->delete) { // the check of 'edges' implies it potentially can be NULL
        e = edges;
        DELETE(edges, e);
    }
    e = edges->next;  // if edges IS in fact null, this will cause crash
...

If my understanding of the above is right, it should probably be enough to add a if (!edges) return; before e = edges->next;.

[ShubhamDesai]

If my understanding of the above is right, it should probably be enough to add a if (!edges) return; before e = edges->next;.

i guess using if (edges) will work the same because for !edges using return doesn't look good because function is void it may lead to additional warning in cppcheck i think.

[nilason]

If my understanding of the above is right, it should probably be enough to add a if (!edges) return; before e = edges->next;.

i guess using if (edges) will work the same because for !edges using return doesn't look good because function is void it may lead to additional warning in cppcheck i think.

In effect they are the same. return; is perfectly valid and only way to return early in a void function (note: there is no return value). Let's compare them side by side:

    if (edges) {
        e = edges->next;
        do {
            if (e->delete) {
                t = e;
                e = e->next;
                DELETE(edges, t);
            }
            else
                e = e->next;
        } while (e != edges);
    }

    if (!edges)
        return;

    e = edges->next;
    do {
        if (e->delete) {
            t = e;
            e = e->next;
            DELETE(edges, t);
        }
        else
            e = e->next;
    } while (e != edges);

In my opinion the readability and intent of the code is much better with the latter (tip: search for "early return" pattern and "pyramid of doom" anti-pattern).

To the root cause of this issue. I managed to reproduce this with Clang's scan-build[^1], and it became more clear whence it originates. The macro DELETE may set edges to NULL if edges == edges->next:

https://github.com/OSGeo/grass/blob/241589424d9612b913ef7f5502839ee6ab2151bc/vector/v.hull/macros.h#L40-L43

We have the same problem with CleanFaces() and a similar one with CleanVertices() too.

@metzm I'm not quite sure of the implications if edges == edges->next and we exit the function early. Was everything that were supposed to be cleaned up, actually cleaned up to that point?

[^1]: The analysis report is available here as a html bundle: scan-build-2024-10-16-105627-63060-1.zip

[metzm]

We have the same problem with CleanFaces() and a similar one with CleanVertices() too. indeed

@metzm I'm not quite sure of the implications if edges == edges->next and we exit the function early. Was everything that were supposed to be cleaned up, actually cleaned up to that point?

I think exiting the function early with

if (!edges)
        return;

is safe because there would be no edges left. edges is the root pointer to the double linked list, if it is NULL, the list is empty.

[echoix]

@nilason Or others, if you are ok with Markus Metz's response and approval, let's merge this. On Monday I'll get it merged if everything is still fine

[ShubhamDesai]

Let us address CID: 1207410 and CID: 1207411 too, with this PR.

@metzm Thank you very much for your input!

I did the changes. also as you suggested @nilason i did the early return. I agree with you to exit early