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

Add integer overflow check to the malloc wrappers #14408

Merged
merged 1 commit into from
Mar 22, 2021
Merged

Add integer overflow check to the malloc wrappers #14408

merged 1 commit into from
Mar 22, 2021

Conversation

LDong-Arm
Copy link
Contributor

Summary of changes

Backport of #14407

Add a check that the combined size of the buffer to allocate and alloc_info_t does not exceed the maximum integer value representable by size_t.

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)
[x] Tests / results supplied as part of this PR

Manual check: malloc(SIZE_MAX) now fails as expected.

Reviewers

@evedon @donatieng

@LDong-Arm
Add integer overflow check to the malloc wrappers
151ebfc 
Add a check that the combined size of the buffer to allocate and
alloc_info_t does not exceed the maximum integer value representable
by size_t.
@ciarmcom ciarmcom requested review from donatieng, evedon and a team March 10, 2021 16:30
@ciarmcom
Copy link
Member

@LDong-Arm, thank you for your changes.
@evedon @donatieng @ARMmbed/mbed-os-maintainers please review.

@mergify mergify bot added needs: CI and removed needs: review labels Mar 10, 2021
donatieng
donatieng approved these changes Mar 10, 2021
View reviewed changes
Copy link
Contributor

@donatieng donatieng left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @LDong-Arm !

@0xc0170
Copy link
Contributor

0xc0170 commented Mar 12, 2021

We should schedule 5.15 jobs after we close the next 6.x release

@mbed-ci
Copy link

mbed-ci commented Mar 17, 2021

Test run: FAILED

Summary: 1 of 10 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_greentea-test-lts

@LDong-Arm
Copy link
Contributor Author

The failures don't look related to this PR, but I can check on my local K64F and DISCO_L475VG_IOT01A (I don't have a NUCLEO_F429ZI).

@0xc0170
Copy link
Contributor

0xc0170 commented Mar 17, 2021

CI restarted

@mbed-ci
Copy link

mbed-ci commented Mar 17, 2021

Test run: FAILED

Summary: 1 of 10 test jobs failed
Build number : 2
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_greentea-test-lts

@mergify mergify bot added needs: work and removed needs: CI labels Mar 17, 2021
@LDong-Arm
Copy link
Contributor Author

LDong-Arm commented Mar 18, 2021 •
edited
Loading

I locally checked on a K64F - the TLS socket test failures have nothing to do with this PR.
It also fails on mbed-os-5.15.6 - did it pass during the previous release? Does this test also exist in the mbed-os-6.x.x release workflow?
If it did pass last time, the failures could be related to the server this test suite talks to?

@0xc0170
Copy link
Contributor

0xc0170 commented Mar 22, 2021

CI restarted

@0xc0170 0xc0170 requested a review from andypowers March 22, 2021 11:30
andypowers
andypowers approved these changes Mar 22, 2021
View reviewed changes
Copy link
Collaborator

@andypowers andypowers left a comment

Choose a reason for hiding this comment

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

Approved.

@LDong-Arm
Copy link
Contributor Author

I locally checked on a K64F - the TLS socket test failures have nothing to do with this PR.
It also fails on mbed-os-5.15.6 - did it pass during the previous release? Does this test also exist in the mbed-os-6.x.x release workflow?
If it did pass last time, the failures could be related to the server this test suite talks to?

I meant, it's not a CI issue because I can reproduce it locally.

@0xc0170
Copy link
Contributor

0xc0170 commented Mar 22, 2021

I run couple of 5.15 jobs in the last days, they were green. I restarted here one more time.

@mergify mergify bot added needs: work and removed needs: CI labels Mar 22, 2021
@mbed-ci
Copy link

mbed-ci commented Mar 22, 2021

Test run: FAILED

Summary: 1 of 10 test jobs failed
Build number : 3
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_greentea-test-lts

@LDong-Arm
Copy link
Contributor Author

Looks like greentea-test-lts is random - #14147 first failed then passed

@mbed-ci
Copy link

mbed-ci commented Mar 22, 2021

Test run: FAILED

Summary: 2 of 10 test jobs failed
Build number : 4
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_example-test-lts
  • jenkins-ci/mbed-os-ci_greentea-test-lts

@0xc0170
Copy link
Contributor

0xc0170 commented Mar 22, 2021

I reported this to the test team (tls socket failures in the recent PRs)

@mbed-ci
Copy link

mbed-ci commented Mar 22, 2021

Test run: SUCCESS

Summary: 10 of 10 test jobs passed
Build number : 5
Build artifacts

@LDong-Arm
Copy link
Contributor Author

Finally passed!

@adbridge adbridge merged commit 52eedfd into ARMmbed:mbed-os-5.15 Mar 22, 2021
adamgreen added a commit to adamgreen/gcc4mbed that referenced this pull request May 6, 2021
@adamgreen
Add integer overflow check to the malloc wrappers
97e0c62 
Port the following security fix from mbed-os repository:
    ARMmbed/mbed-os#14408

Summary:
Add a check that the combined size of the buffer to allocate and
alloc_info_t does not exceed the maximum integer value representable by
size_t.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@evedon evedon evedon approved these changes

@andypowers andypowers andypowers approved these changes

@adbridge adbridge adbridge approved these changes

@donatieng donatieng donatieng approved these changes

Assignees
No one assigned
Labels
release-version: 5.15.7
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@LDong-Arm @ciarmcom @0xc0170 @mbed-ci @donatieng @adbridge @andypowers @evedon