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

Avoid memory corruption behind buffer wp in function dlt_getloginfo_conv_ascii_to_id #411

Merged
merged 3 commits into from
Oct 11, 2022

Conversation

michael-methner
Copy link
Collaborator

  • Introduced new function dlt_getloginfo_conv_ascii_to_string for '\0' terminated strings
  • Avoid printing garbage characters in dlt-control after APID and CTID (which are not null terminated anymore)
  • Added unit test for dlt_client_parse_get_log_info_resp_text and dlt_getloginfo_conv_ascii_to_string

Signed-off-by: Michael Methner [email protected]

…onv_ascii_to_id

- Introduced new function dlt_getloginfo_conv_ascii_to_string for '\0' terminated strings
- Avoid printing garbage characters in dlt-control after APID and CTID (which are not null terminated anymore)
- Added unit test for dlt_client_parse_get_log_info_resp_text and dlt_getloginfo_conv_ascii_to_string

Signed-off-by: Michael Methner <[email protected]>
@thanhbnq
Copy link
Collaborator

thanhbnq commented Oct 6, 2022

Hello @michael-methner ,
I also would like to separate the PR to 2 commits: one for UT and one for source code.

- removed duplicated gtest statement
- corrected doxygen api description

Signed-off-by: Michael Methner <[email protected]>
@michael-methner
Copy link
Collaborator Author

Hello @michael-methner , I also would like to separate the PR to 2 commits: one for UT and one for source code.

Hello @thanhbnq , I want to keep them in one PR as only then the updated gtests are executed already by GitHub actions. Separating it into commits also does not make sense as they will be squashed anyhow.

@andreirusu96
Copy link
Contributor

Hello, @michael-methner ! Thanks for letting me know of your PR!

The code looks good! I just have a small remark: since you've introduced dlt_getloginfo_conv_ascii_to_string(), maybe you would like to also make use of it for app/ctx description parsing in here and here.

Also, my PR ( #403 ) has 2 more improvements and you also suggested changing the cast in dlt_getloginfo_conv_ascii_to_uint16_t() to the same value as the return value.

Do you want to also include those improvements in your PR or would you prefer to do them in my PR (after a rebase)?

Thanks!

…p_description and context_description

Signed-off-by: Michael Methner <[email protected]>
@michael-methner
Copy link
Collaborator Author

The code looks good! I just have a small remark: since you've introduced dlt_getloginfo_conv_ascii_to_string(), maybe you would like to also make use of it for app/ctx description parsing in here and here.

Thanks for the hint, actually this was my intention to use it here but somehow I missed to commit it. I fixed that one.

Also, my PR ( #403 ) has 2 more improvements and you also suggested changing the cast in dlt_getloginfo_conv_ascii_to_uint16_t() to the same value as the return value.

Do you want to also include those improvements in your PR or would you prefer to do them in my PR (after a rebase)?

Thanks!

I would say we can merge this one now and we will add the casts with your PR after rebase. So you will be the author of that change.

@thanhbnq thanhbnq merged commit 154d225 into COVESA:master Oct 11, 2022
@thanhbnq
Copy link
Collaborator

Thank you @michael-methner and @andreirusu96 for PR and discussion.
I merged PR and please continue your next PR :)

Happy coding!
Thanh

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

Successfully merging this pull request may close these issues.

3 participants