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

[C][Client] Free list or map memory when json parsing fails #11866

Merged
merged 2 commits into from Mar 27, 2022

Conversation

ityuhui
Copy link
Contributor

@ityuhui ityuhui commented Mar 14, 2022

When a list or map JSON cannot to be parsed successfully, the original code will goto the end of function *_parseFromJSON without freeing the memory of the list or map.

This PR fixes this memory leak.

@wing328 @zhemant @michelealbano

PR checklist

  • Read the contribution guidelines.
  • Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
  • Run the following to build the project and update samples:
    ./mvnw clean package 
    ./bin/generate-samples.sh
    ./bin/utils/export_docs_generators.sh
    
    Commit all changed files.
    This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
    These must match the expectations made by your contribution.
    You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*.
    For Windows users, please run the script in Git BASH.
  • File the PR against the correct branch: master (5.3.0), 6.0.x
  • If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

keyValuePair_t *localKeyValue = (keyValuePair_t*) listEntry->data;
free(localKeyValue->key);
localKeyValue->key = NULL;
free(localKeyValue->value);

This comment was marked as resolved.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I'll deal by data type.

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. Can you take another look ?

@minerba
Copy link
Contributor

minerba commented Mar 16, 2022

@ityuhui
Thanks your PR.

@ityuhui
Copy link
Contributor Author

ityuhui commented Mar 23, 2022

Hi @zhemant

Can you check the new code changes? I have updated based on your comments.

@zhemant
Copy link
Contributor

zhemant commented Mar 23, 2022

Thank you for the changes @ityuhui.

If we are planning to support primitive types(string, byteArray, enum, number, boolean) then these changes are sufficient, I would recommend we add a non-primitive section where we add a comment line data type not supported as a note that we do not support it in C generator currently.

https://swagger.io/docs/specification/data-models/dictionaries/
Values can be primitives (strings, numbers or boolean values), arrays or objects

@ityuhui
Copy link
Contributor Author

ityuhui commented Mar 23, 2022

I would recommend we add a non-primitive section where we add a comment line data type not supported as a note that we do not support it in C generator currently.

https://swagger.io/docs/specification/data-models/dictionaries/ Values can be primitives (strings, numbers or boolean values), arrays or objects

Thanks @zhemant

I addressed this comment by the commit a76b542
PTAL

@ityuhui
Copy link
Contributor Author

ityuhui commented Mar 23, 2022

I have logged a bug kubernetes-client/c#112 for the non-primitive data in the map.

I will implement it in future.

@wing328 wing328 added this to the 6.0.0 milestone Mar 24, 2022
@ityuhui
Copy link
Contributor Author

ityuhui commented Mar 25, 2022

Is this PR ready to merge now ? @zhemant

@zhemant
Copy link
Contributor

zhemant commented Mar 25, 2022

Thanks for the changes @ityuhui . LGTM

@wing328
Copy link
Member

wing328 commented Mar 27, 2022

Tested locally and C petstore built without issues.

@wing328 wing328 merged commit 0a9429f into OpenAPITools:master Mar 27, 2022
@ityuhui ityuhui deleted the yh-free-parseFromJSON-0301 branch March 27, 2022 13:26
@ityuhui
Copy link
Contributor Author

ityuhui commented Mar 27, 2022

Thanks @wing328 @zhemant

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants