Skip to content

sys/net/gnrc_pktbuf_static: add double free detection#20974

Merged
maribu merged 2 commits intoRIOT-OS:masterfrom
benpicco:gnrc_pktbuf_static-double-free
Nov 16, 2024
Merged

sys/net/gnrc_pktbuf_static: add double free detection#20974
maribu merged 2 commits intoRIOT-OS:masterfrom
benpicco:gnrc_pktbuf_static-double-free

Conversation

@benpicco
Copy link
Copy Markdown
Contributor

@benpicco benpicco commented Nov 11, 2024 •
edited
Loading

Contribution description

The basic idea is: all free data is filled with CANARY bytes, once data is allocated this is overwritten with ~CANARY.
When data is freed we now check if the chunk is only set to CANARY - if this happens, it was already freed.

Testing procedure

Set CONFIG_GNRC_PKTBUF_CHECK_USE_AFTER_FREE to 1.

Issues/PRs references

@github-actions github-actions bot added Area: network Area: Networking Area: sys Area: System labels Nov 11, 2024
@benpicco benpicco force-pushed the gnrc_pktbuf_static-double-free branch from 9bd4954 to 9c6c2f3 Compare November 11, 2024 13:51
@benpicco benpicco added the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label Nov 11, 2024
@benpicco benpicco force-pushed the gnrc_pktbuf_static-double-free branch from 9c6c2f3 to 3d1e6f5 Compare November 11, 2024 14:14
@benpicco benpicco removed the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label Nov 11, 2024
@benpicco benpicco force-pushed the gnrc_pktbuf_static-double-free branch from 3d1e6f5 to 982af61 Compare November 11, 2024 14:17
@benpicco benpicco requested a review from maribu November 12, 2024 13:22
@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Nov 12, 2024
@riot-ci
Copy link
Copy Markdown

riot-ci commented Nov 12, 2024 •
edited
Loading

Murdock results

✔️ PASSED

982af61 sys/net/gnrc_pktbuf_static: make hexdump optional

Success Failures Total Runtime
10250 0 10251 18m:04s

Artifacts

@benpicco benpicco added this pull request to the merge queue Nov 14, 2024
@benpicco benpicco added the Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation label Nov 14, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Nov 15, 2024
@maribu
Copy link
Copy Markdown
Member

maribu commented Nov 15, 2024

Looks like the Murdock queue and the Github queue got out of sync. If the current Murdock build of this succeeds but this PR will not get merged due to the sync issue, I'll requeue it without compile-testing to not waste another 4 h.

@maribu
Copy link
Copy Markdown
Member

maribu commented Nov 15, 2024

The Murdock run failed (probably bogus), but let's have a full run again.

@maribu maribu added this pull request to the merge queue Nov 15, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Nov 15, 2024
@maribu maribu added the CI: skip compile test If set, CI server will run only non-compile jobs, but no compile jobs or their dependent jobs label Nov 15, 2024
@maribu
Copy link
Copy Markdown
Member

maribu commented Nov 15, 2024

This did just pass the CI: https://ci.riot-os.org/details/0e58eacd8ce441bab5074592a9f1b431

Because of a timeout in the merge queue, it did not get merged. I'll requeue without compile test now and add it to the head of the queue, so that everything is as if it were merged after passing the CI.

@maribu maribu enabled auto-merge November 15, 2024 21:02
@maribu maribu added this pull request to the merge queue Nov 15, 2024
@maribu
Copy link
Copy Markdown
Member

maribu commented Nov 15, 2024

so that everything is as if it were merged after passing the CI.

The PR that changes the README.md ended up getting queued first. So it is not exactly as if it this PR got merged right after the test. But I feel confident that a README.md change will not break builds.

@maribu maribu removed this pull request from the merge queue due to the queue being cleared Nov 15, 2024
@maribu maribu added this pull request to the merge queue Nov 15, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Nov 16, 2024
@maribu
Copy link
Copy Markdown
Member

maribu commented Nov 16, 2024

OK, now other stuff got merged as well. Let's have a full CI run again.

@maribu maribu removed the CI: skip compile test If set, CI server will run only non-compile jobs, but no compile jobs or their dependent jobs label Nov 16, 2024
@maribu maribu enabled auto-merge November 16, 2024 09:03
@maribu maribu added this pull request to the merge queue Nov 16, 2024
Merged via the queue into RIOT-OS:master with commit 2adb404 Nov 16, 2024
@benpicco benpicco deleted the gnrc_pktbuf_static-double-free branch November 16, 2024 19:29
@MrKevinWeiss MrKevinWeiss added this to the Release 2025.01 milestone Jan 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@maribu maribu maribu approved these changes

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

@PeterKietzmann PeterKietzmann Awaiting requested review from PeterKietzmann

Assignees

No one assigned

Labels

Area: network Area: Networking Area: sys Area: System CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation

Projects

None yet

Milestone

Release 2025.01

Development

Successfully merging this pull request may close these issues.

4 participants

@benpicco @riot-ci @maribu @MrKevinWeiss