Skip to content

Conversation

@AdithyaBaglody
Copy link
Contributor

If during the execution of the code the malloc fails to allocate
memory, appropriate checks are needed for safe execution.
This patch introduces new checks to ensure safe execution.

@AdithyaBaglody AdithyaBaglody changed the base branch from develop to master July 31, 2020 07:01
@AdithyaBaglody AdithyaBaglody changed the base branch from master to develop July 31, 2020 07:30
@AdithyaBaglody AdithyaBaglody force-pushed the malloc_fix branch 2 times, most recently from 16f9777 to 52fb3c1 Compare July 31, 2020 10:19
@MichalPrincNXP
Copy link
Member

Hello @AdithyaBaglody , the situation with memory allocation checking is not ideal, I agree. However, your pull request solves structures only and that is not systematic to cover just one of all possible data types. Would you be willing to update the patch and cover all supported cases? Also, it is necessary to follow the coding style and not putting if conditional statements on a single line - instead it is required to use curly brackets and put the command in it. Thank you.

@MichalPrincNXP MichalPrincNXP self-assigned this Aug 3, 2020
@MichalPrincNXP MichalPrincNXP self-requested a review August 3, 2020 16:13
Copy link
Member

@MichalPrincNXP MichalPrincNXP left a comment

Choose a reason for hiding this comment

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

Hello @AdithyaBaglody , the situation with memory allocation checking is not ideal, I agree. However, your pull request solves structures only and that is not systematic to cover just one of all possible data types. Would you be willing to update the patch and cover all supported cases? Also, it is necessary to follow the coding style and not putting if conditional statements on a single line - instead it is required to use curly brackets and put the command in it. Thank you.

@AdithyaBaglody
Copy link
Contributor Author

Hello @AdithyaBaglody , the situation with memory allocation checking is not ideal, I agree. However, your pull request solves structures only and that is not systematic to cover just one of all possible data types. Would you be willing to update the patch and cover all supported cases? Also, it is necessary to follow the coding style and not putting if conditional statements on a single line - instead it is required to use curly brackets and put the command in it. Thank you.

@MichalPrincNXP I did some changes in the patch to conform to the coding standards. Also i have added a few more conditions for other data types aswell. I did a few cases and saw all the malloc'd variables were protected. Since i am not familiar with all the supported cases, i am not sure how much i can help in getting them fixed.

@AdithyaBaglody
Copy link
Contributor Author

@MichalPrincNXP hi did you get a chance to review these changes?

If during the execution of the code the malloc fails to allocate
memory appropriate checks are needed for safe execution.
This patch introduces new checks to ensure safe execution.

Signed-off-by: Adithya Baglody <[email protected]>
@MichalPrincNXP
Copy link
Member

Hello @AdithyaBaglody , I am sorry for delayed response ... looking at your pull request again, I have several comments:
a. It is better to add new changes in new commit instead of modifying the previous commit and pushing it with --force, it helps to better understand the development history ...
b. Taking an example from test_unions: there is ArithmeticService_service::sendMyFoo_shim that allocates structure f and even the allocation fails it passes null pointer to read_foo_struct() function. This is not acceptable and your changes in c_common_functions.template solve that by adding null pointer checker at the read_foo_struct() function beginning. I am ok with accepting these changes.
c. But, I am afraid adding "if (!codec->getStatus())" checkers in other parts is not necessary. Looking at the read_foo_struct() again, it would be like this:
image
i.e. "if (!codec->getStatus())" checkers are called repeatedly and I do not see the necessity. Could you please elaborate your motivation for this change, any case you are dealing with?
d. As for Travis check fail, this is caused by python erpcgen test, pattern 'codec->getStatus()' is not expected in test case: test_error_checks_c.yml::not_allowed::type12_in and in test case: test_error_checks_c.yml::not_allowed::type13_out. This is aligned with point c.

Regards
Michal

@Hadatko
Copy link
Member

Hadatko commented Feb 23, 2022

Hi, i reviewed the changes. I reverted changes which where not related to return from struct/union read/write functions when data are NULL.
Quickly it looked like not an issue and if there would be, i would solve it differently. If you steel see some case as an issue please create new PR where case is described.

I also fixed formatting code.

@Hadatko
Copy link
Member

Hadatko commented Feb 23, 2022

Thank you for your contribution. Your changes can be still found on your branch in history before my commits.

@Hadatko Hadatko added this to the 1.9.1 milestone Feb 23, 2022
@MichalPrincNXP MichalPrincNXP merged commit 084cbe8 into EmbeddedRPC:develop Feb 28, 2022
@MichalPrincNXP
Copy link
Member

Thank you!

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

Development

Successfully merging this pull request may close these issues.

3 participants