Skip to content

[msan] Detect dereferencing zero-alloc as use-of-uninitialized-value #155944

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

Merged
merged 1 commit into from
Aug 29, 2025
Merged

[msan] Detect dereferencing zero-alloc as use-of-uninitialized-value #155944

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions compiler-rt/lib/msan/msan_allocator.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,12 @@ static void *MsanAllocate(BufferedStackTrace *stack, uptr size, uptr alignment,
__msan_set_origin(allocated, size, o.raw_id());
}
}

uptr actually_allocated_size = allocator.GetActuallyAllocatedSize(allocated);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't this be like:

if (flags()->poison_in_malloc) {
     __msan_poison(allocated + size, allocator.GetActuallyAllocatedSize(allocated) - size);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would be expensive to do the extra poisoning. The main advantage is it would provide a small amount of buffer overflow protection, but that is outside the scope of MSan. If users want to reliably detect buffer overflows, they should use ASan/HWASan/MTE.

MSan gives no guarantees about:

    char *p = malloc(12); // Allocator actually allocates 16 bytes
    printf ("%d\n", p[100]); // Whether MSan catches this or not depends on which memory it reuses

There's no particular reason it should care about p[12] or p[15] either, which are allocated only because of the nuances of the sanitizer allocator's size classes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why did you create this patch than, it falls into the same category?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be expensive to do the extra poisoning.
I don't think so. It's about alignment tail which likely smaller than primary allocation, which we poison anyway.

Copy link
Collaborator

@vitalybuka vitalybuka Aug 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

It would be expensive to do the extra poisoning.

I don't think so. It's about alignment tail which likely smaller than requested allocation, which we poison anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

b. this patch is already "out of scope" (it's essentially OOB detection)

To be logically consistent with the position that MSan does not handle OOB, I've made a revert pull request for this patch: #156148

Copy link
Collaborator

Choose a reason for hiding this comment

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

To be logically consistent with the position that MSan does not handle OOB, I've made a revert pull request for this patch: #156148

I prefer rather to compete the patch, and just provide https://llvm.org/docs/CodeReview.html#:~:text=including%20those%20requested%20during%20any%20post%2Dcommit%20review.

We have -fsanitize-memory-use-after-dtor which is very close to use after scope or free.

Msan has specific instrumentation, and we'd like to detect what ever this instrumentation detects, I see no point limiting those type of bugs to Asan.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://llvm.org/docs/CodeReview.html#:~:text=including%20those%20requested%20during%20any%20post%2Dcommit%20review.

The section immediately after that quote elaborates that authors should either address the post-commit feedback, or else it will be reverted:

'There is a strong expectation that authors respond promptly to post-commit feedback and address it. Failure to do so is cause for the patch to be reverted.

If a community member expresses a concern about a recent commit, and this concern would have been significant enough to warrant a conversation during pre-commit review (including around the need for more design discussions), they may ask for a revert to the original author who is responsible to revert the patch promptly.' (https://llvm.org/docs/CodeReview.html#:~:text=they%20may%20ask%20for%20a%20revert%20to%20the%20original%20author)

I have offered to revert.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Summarizing magical hallway conversation: I will implement poisoning of the padding, with origin-tracking noting that it is padding

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#157187

// For compatibility, the allocator converted 0-sized allocations into 1 byte
if (size == 0 && actually_allocated_size > 0 && flags()->poison_in_malloc)
__msan_poison(allocated, 1);

UnpoisonParam(2);
RunMallocHooks(allocated, size);
return allocated;
Expand Down
4 changes: 0 additions & 4 deletions compiler-rt/test/msan/zero_alloc.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,5 @@
// RUN: %clang_msan -Wno-alloc-size -fsanitize-recover=memory %s -o %t && not %run %t 2>&1 | FileCheck %s

// MSan doesn't catch this because internally it translates 0-byte allocations
// into 1-byte
// XFAIL: *

#include <malloc.h>
#include <stdio.h>

Expand Down
Loading