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

lib/ukalloc: Fix underallocation bug in malloc #931

Closed
wants to merge 1 commit into from

Conversation

andreittr
Copy link
Contributor

@andreittr andreittr commented Jun 5, 2023

Description of changes

This change fixes a logic error that allocates less memory than necessary in specific cases, risking the memory corruption of internal data structures, leading to unpredictable behavior and crashes.

The bug manifests when requesting exactly 24 bytes less than an integer number of pages (4072, 8168, etc.) because our malloc code reserves 24 bytes of extra space for bookkeeping metadata. However, the same malloc implementation returns buffers 32 bytes above an allocated page boundary, leading to 8 bytes being inadvertently underallocated. Any code that rightfully uses the entire buffer returned by malloc risks corrupting the following page, with dire consequences.
This fix reserves the correct amount of space when computing how many pages to allocate.

Prerequisite checklist

  • Read the contribution guidelines regarding submitting new changes to the project;
  • Tested your changes against relevant architectures and platforms;
  • Ran the checkpatch.uk on your commit series before opening this PR;
  • Updated relevant documentation.

Base target

  • Architecture(s): N/A
  • Platform(s): N/A
  • Application(s): N/A

Additional configuration

Minimal snippet to reproduce the issue:

{
    long *p = malloc(4072);
    p[508] = -1;
    free(p);
}

This change fixes a logic error that allocates less memory than
necessary in specific cases, risking the memory corruption of internal
data structures, leading to unpredictable behavior and crashes.

Signed-off-by: Andrei Tatar <andrei@unikraft.io>
@andreittr andreittr requested a review from a team as a code owner June 5, 2023 08:43
@razvand razvand requested review from Starnox and John-Ted and removed request for a team June 15, 2023 16:10
@razvand razvand self-assigned this Jun 15, 2023
@razvand razvand added area/lib Internal Unikraft Microlibrary lib/ukalloc lang/c Issues or PRs to do with C/C++ topic/mm Topics pertaining to memory management labels Jun 15, 2023
@razvand razvand added this to the v0.14.0 (Prometheus) milestone Jun 15, 2023
Copy link
Contributor

@John-Ted John-Ted left a comment

Choose a reason for hiding this comment

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

With this PR, the native port of redis now functions correctly. Previously, I would get crashes if I sent a large number of requests.

Reviewed-by: Ioan-Teodor Teugea ioan_teodor.teugea@stud.acs.upb.ro

Copy link
Contributor

@Starnox Starnox left a comment

Choose a reason for hiding this comment

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

Nice catch! Tested it throughly!

Reviewed-by: Eduard-Florin Mihailescu mihailescu.eduard@gmail.com

Copy link
Contributor

@razvand razvand left a comment

Choose a reason for hiding this comment

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

Approved-by: Razvan Deaconescu razvand@unikraft.io

@unikraft-bot unikraft-bot added the ci/merged Merged by CI label Jun 28, 2023
@andreittr andreittr deleted the ttr/fixmalloc branch June 30, 2023 07:00
rares-miculescu pushed a commit to rares-miculescu/unikraft that referenced this pull request Jul 1, 2023
This change fixes a logic error that allocates less memory than
necessary in specific cases, risking the memory corruption of internal
data structures, leading to unpredictable behavior and crashes.

Signed-off-by: Andrei Tatar <andrei@unikraft.io>
Reviewed-by: Eduard-Florin Mihailescu <mihailescu.eduard@gmail.com>
Approved-by: Razvan Deaconescu <razvand@unikraft.io>
Tested-by: Unikraft CI <monkey@unikraft.io>
GitHub-Closes: unikraft#931

lib: Added ofw as library

lib: Add ofw as library
rares-miculescu pushed a commit to rares-miculescu/unikraft that referenced this pull request Jul 5, 2023
This change fixes a logic error that allocates less memory than
necessary in specific cases, risking the memory corruption of internal
data structures, leading to unpredictable behavior and crashes.

Signed-off-by: Andrei Tatar <andrei@unikraft.io>
Reviewed-by: Eduard-Florin Mihailescu <mihailescu.eduard@gmail.com>
Approved-by: Razvan Deaconescu <razvand@unikraft.io>
Tested-by: Unikraft CI <monkey@unikraft.io>
GitHub-Closes: unikraft#931
i-Pear pushed a commit to i-Pear/unikraft that referenced this pull request Jul 31, 2023
This change fixes a logic error that allocates less memory than
necessary in specific cases, risking the memory corruption of internal
data structures, leading to unpredictable behavior and crashes.

Signed-off-by: Andrei Tatar <andrei@unikraft.io>
Reviewed-by: Eduard-Florin Mihailescu <mihailescu.eduard@gmail.com>
Approved-by: Razvan Deaconescu <razvand@unikraft.io>
Tested-by: Unikraft CI <monkey@unikraft.io>
GitHub-Closes: unikraft#931
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/lib Internal Unikraft Microlibrary ci/merged Merged by CI lang/c Issues or PRs to do with C/C++ lib/ukalloc topic/mm Topics pertaining to memory management
Projects
Status: Done!
Development

Successfully merging this pull request may close these issues.

None yet

5 participants