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.

Sign up for GitHub

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

Jump to bottom

Fix string buffer length in UUID trace helper #14574

Merged
merged 1 commit into from
May 25, 2021
Merged

Fix string buffer length in UUID trace helper #14574

merged 1 commit into from
May 25, 2021

Conversation

noonfom
Copy link

@noonfom noonfom commented Apr 21, 2021 •
edited
Loading

Summary of changes

The string buffer length in the to_string trace helper for UUIDs is not correct and results in rogue memory accesses.

This commit updates the string buffer length to the correct value of 37.

Impact of changes

Migration actions required

Documentation

None

Pull request type 

[x] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[ ] Feature update (New feature / Functionality change / New API)
[ ] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results 

[ ] No Tests required for this change (E.g docs only update)
[x] Covered by existing mbed-os tests (Greentea or Unittest)
[ ] Tests / results supplied as part of this PR

Reviewers

@ARMmbed/mbed-os-connectivity

@ciarmcom ciarmcom added the release-type: patch Indentifies a PR as containing just a patch label Apr 21, 2021
@ciarmcom ciarmcom requested a review from a team April 21, 2021 16:00
@ciarmcom
Copy link
Member

@noonfom, thank you for your changes.
@ARMmbed/mbed-os-maintainers please review.

Patater
Patater previously requested changes Apr 21, 2021
View reviewed changes
Copy link
Contributor

@Patater Patater left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Would it be possible to add a regression test for this?

You've marked [x] Covered by existing mbed-os tests (Greentea or Unittest). Maybe we have a test already that would have failed if only we'd run it. Could you provide more detail about which test covers this change?

connectivity/FEATURE_BLE/source/common/ble_trace_helpers.cpp Outdated Show resolved Hide resolved
@mergify mergify bot dismissed Patater’s stale review April 21, 2021 17:22

Pull request has been modified.

@mergify mergify bot added needs: CI and removed needs: work labels Apr 21, 2021
pan-
pan- previously approved these changes Apr 22, 2021
View reviewed changes
@noonfom noonfom mentioned this pull request Apr 22, 2021
Open
2 tasks
@AGlass0fMilk
Copy link
Member

Nice work!

@AGlass0fMilk
Copy link
Member

Should fix ARMmbed/mbed-os-experimental-ble-dfu-example#1

Patater
Patater reviewed Apr 23, 2021
View reviewed changes
Copy link
Contributor

@Patater Patater left a comment

Choose a reason for hiding this comment

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

@pan- Do we need some sort of automated regression test (e.g. unit test) before we merge this?

@pan-
Copy link
Member

pan- commented Apr 23, 2021

@Patater This can be indirectly tested with valgrind by doing enough conversions in a unit test that converts many UUIDS. I would be in favor of adding it has part of this PR.

@0xc0170 Is valgrind use in CI to run unit tests ?

@0xc0170
Copy link
Contributor

0xc0170 commented Apr 23, 2021 •
edited
Loading

@0xc0170 Is valgrind use in CI to run unit tests ?

Yes, you can logs from valgrind in logs.zip file (VALGRIND should be defined in CI and executed). Once we run CI, we can verify

@mergify mergify bot dismissed stale reviews from paul-szczepanek-arm and pan- April 27, 2021 11:13

Pull request has been modified.

@noonfom
Copy link
Author

noonfom commented Apr 27, 2021

In commit 27160db, I added a unit test for the to_string trace helper.

I also updated the PR description to reflect this change.

@Patater @pan- @0xc0170 please re review.

@noonfom noonfom mentioned this pull request Apr 30, 2021
Merged
pan-
pan- previously approved these changes May 5, 2021
View reviewed changes
@pan-
Copy link
Member

pan- commented May 5, 2021

@0xc0170 Can we get this in ?

@mergify mergify bot added needs: CI and removed needs: work labels May 5, 2021
@mergify mergify bot dismissed stale reviews from Patater and pan- May 13, 2021 10:59

Pull request has been modified.

@noonfom
Copy link
Author

noonfom commented May 13, 2021

@0xc0170 I moved the unit test to a separate branch. I'll open a new PR once #14426 merges.

Can we please rerun CI and merge this commit? The bug is causing issues for users (see this comment on #14654).

@noonfom noonfom mentioned this pull request May 13, 2021
Open
@adbridge
Copy link
Contributor

@pan- @paul-szczepanek-arm can you re-review this after the changes please.

@adbridge
Copy link
Contributor

I've kicked the CI off in the meantime

adbridge
adbridge approved these changes May 14, 2021
View reviewed changes
Copy link
Contributor

@adbridge adbridge left a comment

Choose a reason for hiding this comment

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

Looks OK from a maintainers perspective

@mbed-ci
Copy link

mbed-ci commented May 14, 2021

Jenkins CI Test : ✔️ SUCCESS

Build Number: 2 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

CLICK for Detailed Summary

jobs Status
jenkins-ci/mbed-os-ci_unittests ✔️
jenkins-ci/mbed-os-ci_cmake-cloud-example-ARM ✔️
jenkins-ci/mbed-os-ci_build-cloud-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_build-cloud-example-ARM ✔️
jenkins-ci/mbed-os-ci_build-greentea-ARM ✔️
jenkins-ci/mbed-os-ci_cmake-cloud-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_cmake-example-ARM ✔️
jenkins-ci/mbed-os-ci_build-greentea-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_cmake-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_build-example-ARM ✔️
jenkins-ci/mbed-os-ci_build-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_greentea-test ✔️

@AGlass0fMilk
Copy link
Member

AGlass0fMilk commented May 18, 2021 •
edited
Loading

Can we get this merged? The issue it fixes breaks Mbed-OS BLE on master for applications that use custom UUIDs

@noonfom
Copy link
Author

noonfom commented May 18, 2021

@AGlass0fMilk I feel your pain. This PR will forever haunt me 😓

@0xc0170 @adbridge Can we please merge this first thing tomorrow?

@AGlass0fMilk
Copy link
Member

I feel your pain. This PR will forever haunt me 😓

No worries, something was bound to break in the massive updates to BLE that have been going on. The traces are very nice to have. They make the BLE system seem a little less like a huge, mysterious black box.

@ciarmcom ciarmcom added the stale Stale Pull Request label May 21, 2021
@ciarmcom
Copy link
Member

This pull request has automatically been marked as stale because it has had no recent activity. , please complete review of the changes to move the PR forward. Thank you for your contributions.

@ciarmcom ciarmcom added stale Stale Pull Request and removed stale Stale Pull Request labels May 21, 2021
@adbridge
Copy link
Contributor

Going to re-run the Ci as this has been pending for so long , just to double check it is still ok

@mbed-ci
Copy link

mbed-ci commented May 24, 2021

Jenkins CI Test : ✔️ SUCCESS

Build Number: 3 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

CLICK for Detailed Summary

jobs Status
jenkins-ci/mbed-os-ci_unittests ✔️
jenkins-ci/mbed-os-ci_build-cloud-example-ARM ✔️
jenkins-ci/mbed-os-ci_cmake-cloud-example-ARM ✔️
jenkins-ci/mbed-os-ci_build-greentea-ARM ✔️
jenkins-ci/mbed-os-ci_build-cloud-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_cmake-cloud-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_cmake-example-ARM ✔️
jenkins-ci/mbed-os-ci_build-greentea-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_build-example-ARM ✔️
jenkins-ci/mbed-os-ci_cmake-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_build-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_greentea-test ✔️

@ciarmcom ciarmcom removed the stale Stale Pull Request label May 24, 2021
@adbridge adbridge merged commit 8b1cd98 into ARMmbed:master May 25, 2021
@mbedmain mbedmain added release-version: 6.12.0 Release-pending and removed release-type: patch Indentifies a PR as containing just a patch Release-pending labels Jun 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pan- pan- pan- approved these changes

@adbridge adbridge adbridge approved these changes

@paul-szczepanek-arm paul-szczepanek-arm paul-szczepanek-arm approved these changes

@chrisswinchatt-arm chrisswinchatt-arm chrisswinchatt-arm approved these changes

@Patater Patater Patater left review comments

Assignees
No one assigned
Labels
component: connectivity release-version: 6.12.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.