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

Don't check for null pointer before free, it should be handled in the free function #275

Merged
merged 1 commit into from
Jun 6, 2022

Conversation

amgross
Copy link
Contributor

@amgross amgross commented May 16, 2022

Pull request

Choose Correct

  • bug
  • feature

Describe the pull request
Stop checking if pointer is NULL before sending it to the erpc_free function. The erpc_free should handle NULL pointer properly (like c standard free)

To Reproduce
Generate code that allocating space on the heap.

Desktop (please complete the following information):

  • OS: Windows
  • eRPC Version: 1.8.1

Steps you didn't forgot to do

  • I checked if other PR isn't solving this issue.
  • I read Contribution details and did appropriate actions.
  • PR code is tested.
  • PR code is formatted.

Additional context

@amgross
Copy link
Contributor Author

amgross commented May 16, 2022

this commit fixes #274

@Hadatko Hadatko added this to the 1.10.0 milestone May 16, 2022
@Hadatko Hadatko linked an issue May 16, 2022 that may be closed by this pull request
@Hadatko
Copy link
Member

Hadatko commented May 16, 2022

Hi @amgross , thank you for PR, looks great. I will check other port layers if they don't need check for NULL and let you know.

@amgross
Copy link
Contributor Author

amgross commented May 16, 2022

I took a look on the ports and all looked OK for me, but second glance is always good

@Hadatko
Copy link
Member

Hadatko commented May 17, 2022

Hi @amgross:

  1. Port erpc_port_memmanager.cpp is using some deprecating non existing macro for allocation. I will fix it, but
    we need add check for null pointer for erpc_free implementation as NULL pointer would cause returning error from free function. Here and Here
    Currently we are not checking return value, but in future we could add erpc_assert here to check errors. (i think i will do it very soon)
  2. Mqx free function need same check for same reason as above
    image

@Hadatko
Copy link
Member

Hadatko commented May 17, 2022

Mine PR related to this: #277

@amgross
Copy link
Contributor Author

amgross commented May 18, 2022

Hi @Hadatko ,
I agree that there are some ports that return error in case of NULL pointer, but it is OK as long as null pointer is checked and not referenced, and the function returns. Your changes in #277 are the first one that not doing this, and hence your changes make the free function behave different than standard library and I think it is bad idea.
Anyway i think it is not good idea to assert in case of free failure (in non debug flavor), this is just memory leak and not fatal, I will discuss it in #277 .

@amgross amgross mentioned this pull request May 18, 2022
6 tasks
Copy link
Member

@Hadatko Hadatko left a comment

Choose a reason for hiding this comment

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

I am approving this PR.

@amgross
Copy link
Contributor Author

amgross commented May 18, 2022

What is the flow of merging in this repository?
Need also @MichalPrincNXP approval?
one of you will merge it?

@Hadatko
Copy link
Member

Hadatko commented May 18, 2022

@amgross i am letting merging on @MichalPrincNXP. He is performing also more tests directly on NXP devices.

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.

I am sorry for the delay, thank you for this PR.

@MichalPrincNXP MichalPrincNXP merged commit de940fe into EmbeddedRPC:develop Jun 6, 2022
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.

Un-needed check for null pointer before free
3 participants