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

libvector/diglib: fix memory leaks #3617

Merged
merged 1 commit into from
May 3, 2024

Conversation

nilason
Copy link
Contributor

@nilason nilason commented Apr 17, 2024

Fix some memory leaks in lib/vector/diglib exposed by static analysis.

@nilason nilason added this to the 8.4.0 milestone Apr 17, 2024
@nilason nilason self-assigned this Apr 17, 2024
@github-actions github-actions bot added vector Related to vector data processing C Related code is in C libraries labels Apr 17, 2024
@nilason nilason modified the milestones: 8.4.0, 8.5.0 Apr 30, 2024
Copy link
Contributor

@metzm metzm left a comment

Choose a reason for hiding this comment

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

I don't like goto in C code, but the usage here seems ok:

Have you observed actual memory leaks that are fixed by this PR?

@marisn
Copy link
Contributor

marisn commented May 3, 2024

I don't like goto in C code, but the usage here seems ok:

This code is a correct reason why goto should exist and should be used.

@wenzeslaus
Copy link
Member

This code is a correct reason why goto should exist and should be used.

I reluctantly agree. It seems that goto was overused and the legitimate usages disappeared from our minds. I did a quick search earlier and internet thinks usage like this is okay (e.g. for Linux kernel if I recall correctly).

Copy link
Member

@echoix echoix left a comment

Choose a reason for hiding this comment

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

I was already pretty sure of this, but with the discussion here, my last doubts are addressed.

@echoix echoix merged commit c783f5d into OSGeo:main May 3, 2024
26 checks passed
@echoix echoix modified the milestones: 8.5.0, 8.4.0 May 3, 2024
@nilason nilason deleted the coverity_fix_vector_diglib_leaks branch May 15, 2024 09:31
HuidaeCho pushed a commit to HuidaeCho/grass that referenced this pull request May 21, 2024
@nilason
Copy link
Contributor Author

nilason commented Jun 17, 2024

I don't like goto in C code, but the usage here seems ok:

Have you observed actual memory leaks that are fixed by this PR?

This is as far as I personally would go to use goto: simple memory clean up operations. It has the advantage to make sure everything get released, without repeating code or creating additional clean-up functions (for every function).

@nilason
Copy link
Contributor Author

nilason commented Jun 17, 2024

Have you observed actual memory leaks that are fixed by this PR?

The leaks are reported by Coverity. The issues were fixed according to the testing of my fork against Coverity.
While GRASS has a liberal attitude to leaks in modules due to their short-lived runs, I specifically aim to address those reported in the libraries.

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 libraries vector Related to vector data processing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants