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

m.nviz.image: Fix Resource Leak issue #5079

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

ShubhamDesai
Copy link
Contributor

@ShubhamDesai ShubhamDesai commented Feb 9, 2025

This pull request fixes issue identified by Coverity Scan (CID : 1207735, 1207736, 1207924, 1207925, 1208025, 1208026, 1208027, 1208028, 1208029, 1400466, 1501205, 1501280, 1501338)
Used G_free() to fix this issue.

@github-actions github-actions bot added C Related code is in C module misc labels Feb 9, 2025
@nilason nilason added this to the 8.5.0 milestone Feb 10, 2025
@nilason
Copy link
Contributor

nilason commented Feb 10, 2025

There are a number of similar issues in m.nviz.image, please filter with misc/m.nviz.image/*, to address them all (they all seem to be uncomplicated).

@ShubhamDesai
Copy link
Contributor Author

There are a number of similar issues in m.nviz.image, please filter with misc/m.nviz.image/*, to address them all (they all seem to be uncomplicated).

done all changes

@ShubhamDesai ShubhamDesai changed the title m.nviz.image: Fix Resource Leak issue in volume.c m.nviz.image: Fix Resource Leak issue Feb 10, 2025
@nilason
Copy link
Contributor

nilason commented Feb 10, 2025

All calls to G_fully_qualified_name() returns an allocated value which must be freed.

@ShubhamDesai
Copy link
Contributor Author

All calls to G_fully_qualified_name() returns an allocated value which must be freed.

done

ShubhamDesai and others added 3 commits February 10, 2025 17:57
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@ShubhamDesai
Copy link
Contributor Author

All calls to G_fully_qualified_name() returns an allocated value which must be freed.

is there anything wrong in code? mac build failed.
I would fix the clang formatting issues sorry about the noise.

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.

I added a few suggestions.

@@ -27,7 +27,7 @@
*/
int load_rasters(const struct GParams *params, nv_data *data)
{
const char *mapset;
const char *mapset = NULL;
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of re-declare mname, you can do it once here (with initialisation): char *mname = NULL.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Comment on lines 361 to 362
db_free_column(column);
Vect_destroy_field_info(Fi);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is what is causing the Mac runner to fail:

error: variable 'column' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized]
  307 |     if (Fi) {
      |         ^~
vector.c:361:20: note: uninitialized use occurs here
  361 |     db_free_column(column);
      |                    ^~~~~~
vector.c:307:5: note: remove the 'if' if its condition is always true
  307 |     if (Fi) {
      |     ^~~~~~~
vector.c:261:21: note: initialize the variable 'column' to silence this warning
  261 |     dbColumn *column;
      |                     ^
      |                      = NULL
1 error generated.

you should move Vect_destroy_field_info(Fi); to the end of the if (Fi) statement (before or after db_close_database_shutdown_driver(driver);).

column is allocated with db_get_column() four times, and should be freed after each use (by db_column_Ctype()).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -169,7 +175,7 @@ int vlines_set_attrb(const struct GParams *params)
*/
int vpoints_set_attrb(const struct GParams *params)
{
int i, layer, have_colors, with_z;
int i, layer = 0, have_colors, with_z = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

You may move declaration (and initialisation) of int layer = -1, with_z = 0 to the beginning of the for (i = 0; i < nsites; i++) { statement (L184). They are set in every loop with check_map(), and should be zeroed before.

(There is a bug in check_map(), L291: if (with_z) *with_z = Vect_is_3d(&Map);. The with_z is however an out parameter, thus checking it does not make any sense. That is out of scope of this PR anyway).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

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 misc module
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

2 participants