Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

drivers/mtd: Add check for integer overflow #20587

Merged
merged 1 commit into from
Apr 18, 2024
Merged

drivers/mtd: Add check for integer overflow #20587

merged 1 commit into from
Apr 18, 2024

Conversation

Teufelchen1
Copy link
Contributor

@Teufelchen1 Teufelchen1 commented Apr 16, 2024 •
edited
Loading

Contribution description

Hei 🦫

Fixes a minor potential for integer overflow.

Testing procedure

CI / good review
Maybe make -C tests/unittests tests-mtd test

@github-actions github-actions bot added the Area: drivers Area: Device drivers label Apr 16, 2024
Teufelchen1
Teufelchen1 commented Apr 16, 2024
View reviewed changes
drivers/mtd_emulated/mtd_emulated.c
@@ -16,6 +16,7 @@
static int _init(mtd_dev_t *dev)
{
mtd_emulated_t *mtd = (mtd_emulated_t *)dev;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unrelated change, sneaking it in to make the file happy in uncrustify.

bergzand
bergzand approved these changes Apr 16, 2024
View reviewed changes
Copy link
Member

@bergzand bergzand left a comment

Choose a reason for hiding this comment

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

Changes look good, thanks for the cleanup

Untested ack from my side

@Teufelchen1 Teufelchen1 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Apr 16, 2024
@maribu maribu enabled auto-merge April 16, 2024 13:01
@riot-ci
Copy link

riot-ci commented Apr 16, 2024 •
edited
Loading

Murdock results

✔️ PASSED

3bd047a drivers/mtd: Add check for interger overflow

Success Failures Total Runtime
10066 0 10066 13m:59s

Artifacts

@Teufelchen1
Copy link
Contributor Author

Switched from the unsigned buildins to the "you will figure the type out" buildins.

maribu
maribu approved these changes Apr 16, 2024
View reviewed changes
@maribu maribu added this pull request to the merge queue Apr 16, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 17, 2024
@maribu
Copy link
Member

maribu commented Apr 17, 2024

/opt/riot-toolchain/msp430-elf/10.1.0-18/bin/../lib/gcc/msp430-elf/10.1.0/../../../../msp430-elf/bin/ld: section .bss LMA wraps around address space
collect2: error: ld returned 1 exit status

This means it got larger than what would fit into 16 bit address space, so it needs to be added to Makefile.ci.

Btw.: I get this a lot with the BUILD_IN_DOCKER=1 toolchain, and rarely with Alpine's toolchain. My guess is that the newer linker will perform the check if data exceeds the 16 bit address space after the garbage collection phase, and the older one did this before. This would explain why the same app build on my toolchain not only ends up fitting into 16 bit address space, but also into the (smaller) RAM and ROM (and behave as expected), while it fails in the CI.

But anyway, bumping Makefile.ci it is for now and if the toolchain gets updated, we can re-enable linking in the CI of that app.

@github-actions github-actions bot added the Area: tests Area: tests and testing framework label Apr 18, 2024
@Teufelchen1
Copy link
Contributor Author

Teufelchen1 commented Apr 18, 2024 •
edited
Loading

Thanks for looking into it Maribu. I add (via script) the olimex-msp430-h1611 and telosb to the Makefile.ci

ah maybe I should have put it in an different commit.

@maribu maribu added this pull request to the merge queue Apr 18, 2024
Merged via the queue into RIOT-OS:master with commit daa6b9d Apr 18, 2024
25 checks passed
@mguetschow mguetschow added this to the Release 2024.07 milestone Jul 5, 2024
@maribu maribu changed the title drivers/mtd: Add check for interger overflow drivers/mtd: Add check for integer overflow Jul 26, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@maribu maribu maribu approved these changes

@bergzand bergzand bergzand approved these changes

@leandrolanzieri leandrolanzieri Awaiting requested review from leandrolanzieri leandrolanzieri is a code owner

@aabadie aabadie Awaiting requested review from aabadie aabadie is a code owner

@MichelRottleuthner MichelRottleuthner Awaiting requested review from MichelRottleuthner MichelRottleuthner is a code owner

Assignees
No one assigned
Labels
Area: drivers Area: Device drivers Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Teufelchen1 @riot-ci @maribu @bergzand @mguetschow