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

Fixed memcpy for template #352

Merged
merged 2 commits into from
Apr 6, 2023

Conversation

Hadatko
Copy link
Member

@Hadatko Hadatko commented Mar 7, 2023

Pull request

Choose Correct

  • bug
  • feature

Describe the pull request

memcpy could have undefined behavior

To Reproduce

Expected behavior

Screenshots

Desktop (please complete the following information):

  • OS:
  • eRPC Version:

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.
  • Allow edits from maintainers pull request option is set (recommended).

Additional context

@Hadatko Hadatko added the bug label Mar 7, 2023
@Hadatko Hadatko added this to the 1.11.0 milestone Mar 7, 2023
@Hadatko Hadatko requested a review from MichalPrincNXP March 7, 2023 12:43
@Hadatko Hadatko self-assigned this Mar 7, 2023
@Hadatko Hadatko linked an issue Mar 7, 2023 that may be closed by this pull request
@Hadatko
Copy link
Member Author

Hadatko commented Mar 7, 2023

@amgross Do you agree with PR?

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 OK with the integration

@@ -63,13 +63,16 @@ if (({$info.sizeTemp} <= {$info.maxSize}) && ({$info.dataTemp} != NULL))
{% if source == "server" || info.useMallocOnClientSide == true %}
{$indent}{$info.name} = (uint8_t *) erpc_malloc({$info.maxSize} * sizeof(uint8_t));
{% if generateAllocErrorChecks == true %}
{$indent}if (({$info.name} == NULL) && ({$info.sizeTemp} > 0))
{$indent}if ({$info.sizeTemp} > 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

Still if generateAllocErrorChecks is false the issue will happen, suggest to move this check outside the {% if generateAllocErrorChecks == true %}

erpcgen/src/templates/c_coders.template Show resolved Hide resolved
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.

@Hadatko , it seems @amgross catches it correctly, could you please comment and update this PR accordingly? Thank you.

Signed-off-by: Cervenka Dusan <cervenka@acrios.com>
@Hadatko
Copy link
Member Author

Hadatko commented Apr 3, 2023

Hi guys, what do you think now?

Signed-off-by: Cervenka Dusan <cervenka@acrios.com>
@amgross
Copy link
Contributor

amgross commented Apr 4, 2023

If you can next time not force push changes I will appreciate it.

@Hadatko
Copy link
Member Author

Hadatko commented Apr 4, 2023

@amgross Sorry for that, i accidentally did commit with amend as i need to do in other project. They do not want a lot of commits.

@amgross
Copy link
Contributor

amgross commented Apr 4, 2023

All fine,
Of course the merge to the branch should be in single commit that unifying all the commits

@MichalPrincNXP MichalPrincNXP merged commit e8e2e63 into EmbeddedRPC:develop Apr 6, 2023
@MichalPrincNXP
Copy link
Member

Thank you for resolving the discussion and providing this fix.

@Hadatko Hadatko deleted the bugfix/memcpy branch April 6, 2023 11:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

Optional undefined behavior of memcpy
3 participants