Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

v.hull: Dereference after Null check #4524

Merged
merged 5 commits into from
Oct 21, 2024
Merged

Conversation

ShubhamDesai
Copy link
Contributor

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.

@github-actions github-actions bot added vector Related to vector data processing C Related code is in C module labels Oct 14, 2024
vector/v.hull/chull.c Outdated Show resolved Hide resolved
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
vector/v.hull/chull.c Outdated Show resolved Hide resolved
vector/v.hull/chull.c Outdated Show resolved Hide resolved
@nilason
Copy link
Contributor

nilason commented Oct 15, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor

nilason commented Oct 16, 2024

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-build1, and it became more clear whence it originates. The macro DELETE may set edges to NULL if edges == edges->next:

#define DELETE(head, p) \
if (head) { \
if (head == head->next) \
head = NULL; \

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?

Footnotes

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

metzm
metzm previously approved these changes Oct 16, 2024
@metzm
Copy link
Contributor

metzm commented Oct 17, 2024

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
Copy link
Member

echoix commented Oct 19, 2024

@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

Copy link
Contributor

@nilason nilason left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@metzm Thank you very much for your input!

@ShubhamDesai
Copy link
Contributor Author

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

@nilason nilason enabled auto-merge (squash) October 21, 2024 12:31
Copy link
Contributor

@nilason nilason left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ShubhamDesai Thanks!

@nilason nilason merged commit 5d4f7fb into OSGeo:main Oct 21, 2024
26 checks passed
@github-actions github-actions bot added this to the 8.5.0 milestone Oct 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C Related code is in C module vector Related to vector data processing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants