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

g.gisenv: Fix Resource Leak issue in main.c #4966

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

Conversation

ShubhamDesai
Copy link
Contributor

This pull request fixes issue identified by Coverity Scan (CID : 1207681)
Used G_free() to fix this issue.

@github-actions github-actions bot added C Related code is in C module general labels Jan 19, 2025
@ShubhamDesai
Copy link
Contributor Author

@nilason won't the G_free directly work here?

@@ -224,6 +225,7 @@ char *parse_variable(const char *v_name, char **value)
G_verbose_message(_("GRASS variable must be uppercase. Using '%s'."),
u_name);
}
G_free(name);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that you should also add a G_free at line 218 before the G_fatal_error

Copy link
Contributor

Choose a reason for hiding this comment

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

It wouldn't be bad, but introduce unnecessary code. G_fatal_error will print an error message and exit the process. Thus, in-memory will be freed safely and quickly by the system.

@nilason
Copy link
Contributor

nilason commented Jan 19, 2025

@nilason won't the G_free directly work here?

Not quite sure what you mean.

@nilason
Copy link
Contributor

nilason commented Jan 19, 2025

@nilason won't the G_free directly work here?

Not quite sure what you mean.

If you mean G_free(name) directly after u_name = G_store(name);, then yes.

@ShubhamDesai
Copy link
Contributor Author

@nilason won't the G_free directly work here?

Not quite sure what you mean.

If you mean G_free(name) directly after u_name = G_store(name);, then yes.

I used G_free() directly but still it failed.
why this issue arised? can you please help.

@nilason
Copy link
Contributor

nilason commented Jan 21, 2025

@nilason won't the G_free directly work here?

Not quite sure what you mean.

If you mean G_free(name) directly after u_name = G_store(name);, then yes.

I used G_free() directly but still it failed. why this issue arised? can you please help.

I now see you mean the tests are failing. The reason wasn't obvious to me at first. This may be a good exercise :).
Hint: understand the purpose of the function, and study the function line-by-line, in particular the use of the variable value, therein lies the root of the problem.

@ShubhamDesai
Copy link
Contributor Author

ShubhamDesai commented Jan 21, 2025

@nilason won't the G_free directly work here?

Not quite sure what you mean.

If you mean G_free(name) directly after u_name = G_store(name);, then yes.

I used G_free() directly but still it failed. why this issue arised? can you please help.

I now see you mean the tests are failing. The reason wasn't obvious to me at first. This may be a good exercise :). Hint: understand the purpose of the function, and study the function line-by-line, in particular the use of the variable value, therein lies the root of the problem.

The parse_variable function parses a string such as "VAR=value", separating the variable name (VAR) and value (value). Based on your hint I updated the function and tested locally with following testcases
Input with a variable and value:
Input: "VAR=value"
Result: "VAR"
Value: "value"

Input with only a variable (no =):
Input: "VAR"
Result: "VAR"
Value: (null)

Input with an empty value:
Input: "VAR="
Result: "VAR"
Value: (null)

All of them were as expected. It passed on my local windows machine
Could you please review once.

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 general module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants