Skip to content

8342504: Remove NMT header and footer canaries #21560

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

Closed
wants to merge 4 commits into from
+13 −366
Closed

8342504: Remove NMT header and footer canaries #21560

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
90734f6
Remove canaries
jdksjolen Oct 17, 2024
838b80d
Rename flags to tags
jdksjolen Nov 18, 2024
440401a
Comment
jdksjolen Nov 21, 2024
aa49dab
Merge remote-tracking branch 'openjdk/master' into delete-canaries
jdksjolen Feb 20, 2025
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
54 changes: 10 additions & 44 deletions src/hotspot/share/nmt/mallocHeader.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,14 +39,11 @@ class outputStream;
* If NMT is active (state >= summary), we need to track allocations. A simple and cheap way to
* do this is by using malloc headers.
*
* The user allocation is preceded by a header and is immediately followed by a (possibly unaligned)
* footer canary:
*
* +--------------+------------- .... ------------------+-----+
* | header | user | can |
* | | allocation | ary |
* +--------------+------------- .... ------------------+-----+
* 16 bytes user size 2 byte
* +--------------+------------- .... ------------------+
* | header | user |
* | | allocation |
* +--------------+------------- .... ------------------+
* 16 bytes user size
*
* Alignment:
*
Expand All @@ -63,28 +60,20 @@ class outputStream;
*
* 8 9 10 11 12 13 14 15 16 ++
* +--------+--------+--------+--------+--------+--------+--------+--------+ ------------------------
* ... | malloc site table marker | flags | unused | canary | ... User payload ....
* ... | malloc site table marker | tags | unused | ... User payload ....
* +--------+--------+--------+--------+--------+--------+--------+--------+ ------------------------
*
* Layout on 32-bit:
*
* 0 1 2 3 4 5 6 7
* +--------+--------+--------+--------+--------+--------+--------+--------+
* | alt. canary | 32-bit size | ...
* | 32-bit size | ...
* +--------+--------+--------+--------+--------+--------+--------+--------+
*
* 8 9 10 11 12 13 14 15 16 ++
* +--------+--------+--------+--------+--------+--------+--------+--------+ ------------------------
* ... | malloc site table marker | flags | unused | canary | ... User payload ....
* ... | malloc site table marker | tags | unused | ... User payload ....
* +--------+--------+--------+--------+--------+--------+--------+--------+ ------------------------
*
* Notes:
* - We have a canary in the two bytes directly preceding the user payload. That allows us to
* catch negative buffer overflows.
* - On 32-bit, due to the smaller size_t, we have some bits to spare. So we also have a second
* canary at the very start of the malloc header (generously sized 32 bits).
* - The footer canary consists of two bytes. Since the footer location may be unaligned to 16 bits,
* the bytes are stored individually.
*/

class MallocHeader {
Expand All @@ -93,27 +82,13 @@ class MallocHeader {
const size_t _size;
const uint32_t _mst_marker;
const MemTag _mem_tag;
const uint8_t _unused;
uint16_t _canary;

static const uint16_t _header_canary_live_mark = 0xE99E;
static const uint16_t _header_canary_dead_mark = 0xD99D;
static const uint16_t _footer_canary_live_mark = 0xE88E;
static const uint16_t _footer_canary_dead_mark = 0xD88D;
NOT_LP64(static const uint32_t _header_alt_canary_live_mark = 0xE99EE99E;)
NOT_LP64(static const uint32_t _header_alt_canary_dead_mark = 0xD88DD88D;)
const uint8_t _unused[3];

// We discount sizes larger than these
static const size_t max_reasonable_malloc_size = LP64_ONLY(256 * G) NOT_LP64(3500 * M);

void print_block_on_error(outputStream* st, address bad_address) const;

static uint16_t build_footer(uint8_t b1, uint8_t b2) { return (uint16_t)(((uint16_t)b1 << 8) | (uint16_t)b2); }

uint8_t* footer_address() const { return ((address)this) + sizeof(MallocHeader) + _size; }
uint16_t get_footer() const { return build_footer(footer_address()[0], footer_address()[1]); }
void set_footer(uint16_t v) { footer_address()[0] = (uint8_t)(v >> 8); footer_address()[1] = (uint8_t)v; }

template<typename InTypeParam, typename OutTypeParam>
inline static OutTypeParam resolve_checked_impl(InTypeParam memblock);

Expand All @@ -127,7 +102,7 @@ class MallocHeader {

inline MallocHeader(size_t size, MemTag mem_tag, uint32_t mst_marker);

inline static size_t malloc_overhead() { return sizeof(MallocHeader) + sizeof(uint16_t); }
inline static size_t malloc_overhead() { return sizeof(MallocHeader); }
inline size_t size() const { return _size; }
inline MemTag mem_tag() const { return _mem_tag; }
inline uint32_t mst_marker() const { return _mst_marker; }
Expand All @@ -136,15 +111,6 @@ class MallocHeader {
FreeInfo free_info() {
return FreeInfo{this->size(), this->mem_tag(), this->mst_marker()};
}
inline void mark_block_as_dead();
inline void revive();


bool is_dead() const { return _canary == _header_canary_dead_mark; }
bool is_live() const { return _canary == _header_canary_live_mark; }

// Used for debugging purposes only. Check header if it could constitute a valid (live or dead) header.
inline bool looks_valid() const;

// If block is broken, fill in a short descriptive text in out,
// an option pointer to the corruption in p_corruption, and return false.
Expand Down
55 changes: 1 addition & 54 deletions src/hotspot/share/nmt/mallocHeader.inline.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,30 +36,11 @@

inline MallocHeader::MallocHeader(size_t size, MemTag mem_tag, uint32_t mst_marker)
: _size(size), _mst_marker(mst_marker), _mem_tag(mem_tag),
_unused(0), _canary(_header_canary_live_mark)
_unused()
{
assert(size < max_reasonable_malloc_size, "Too large allocation size?");
// On 32-bit we have some bits more, use them for a second canary
// guarding the start of the header.
NOT_LP64(_alt_canary = _header_alt_canary_live_mark;)
set_footer(_footer_canary_live_mark); // set after initializing _size
}

inline void MallocHeader::revive() {
assert(_canary == _header_canary_dead_mark, "must be dead");
assert(get_footer() == _footer_canary_dead_mark, "must be dead");
NOT_LP64(assert(_alt_canary == _header_alt_canary_dead_mark, "must be dead"));
_canary = _header_canary_live_mark;
NOT_LP64(_alt_canary = _header_alt_canary_live_mark);
set_footer(_footer_canary_live_mark);
}

// The effects of this method must be reversible with MallocHeader::revive()
inline void MallocHeader::mark_block_as_dead() {
_canary = _header_canary_dead_mark;
NOT_LP64(_alt_canary = _header_alt_canary_dead_mark);
set_footer(_footer_canary_dead_mark);
}

inline bool MallocHeader::is_valid_malloced_pointer(const void* payload, char* msg, size_t msglen) {
// Handle the pointer as an integral type
Expand Down Expand Up @@ -116,50 +97,16 @@ inline const MallocHeader* MallocHeader::resolve_checked(const void* memblock) {
return MallocHeader::resolve_checked_impl<const void*, const MallocHeader*>(memblock);
}


// Used for debugging purposes only. Check header if it could constitute a valid (live or dead) header.
inline bool MallocHeader::looks_valid() const {
// Note: we define these restrictions loose enough to also catch moderately corrupted blocks.
// E.g. we don't check footer canary.
return ( (_canary == _header_canary_live_mark NOT_LP64(&& _alt_canary == _header_alt_canary_live_mark)) ||
(_canary == _header_canary_dead_mark NOT_LP64(&& _alt_canary == _header_alt_canary_dead_mark)) ) &&
_size > 0 && _size < max_reasonable_malloc_size;
}

inline bool MallocHeader::check_block_integrity(char* msg, size_t msglen, address* p_corruption) const {
// Note: if you modify the error messages here, make sure you
// adapt the associated gtests too.

// Check header canary
if (_canary != _header_canary_live_mark) {
*p_corruption = (address)this;
jio_snprintf(msg, msglen, "header canary broken");
return false;
}

#ifndef _LP64
// On 32-bit we have a second canary, check that one too.
if (_alt_canary != _header_alt_canary_live_mark) {
*p_corruption = (address)this;
jio_snprintf(msg, msglen, "header canary broken");
return false;
}
#endif

// Does block size seems reasonable?
if (_size >= max_reasonable_malloc_size) {
*p_corruption = (address)this;
jio_snprintf(msg, msglen, "header looks invalid (weirdly large block size)");
return false;
}

// Check footer canary
if (get_footer() != _footer_canary_live_mark) {
*p_corruption = footer_address();
jio_snprintf(msg, msglen, "footer canary broken at " PTR_FORMAT " (buffer overflow?)",
p2i(footer_address()));
return false;
}
return true;
}

Expand Down
98 changes: 0 additions & 98 deletions src/hotspot/share/nmt/mallocTracker.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,8 +206,6 @@ void* MallocTracker::record_free_block(void* memblock) {

deaccount(header->free_info());

header->mark_block_as_dead();

return (void*)header;
}

Expand All @@ -217,99 +215,3 @@ void MallocTracker::deaccount(MallocHeader::FreeInfo free_info) {
MallocSiteTable::deallocation_at(free_info.size, free_info.mst_marker);
}
}

// Given a pointer, look for the containing malloc block.
// Print the block. Note that since there is very low risk of memory looking
// accidentally like a valid malloc block header (canaries and all) so this is not
// totally failproof and may give a wrong answer. It is safe in that it will never
// crash, even when encountering unmapped memory.
bool MallocTracker::print_pointer_information(const void* p, outputStream* st) {
Copy link

Choose a reason for hiding this comment

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

It looks like the only thing that had to go was candidate->looks_valid()

Isn't it useful to keep the rest of this code though?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We've also removed the is_live() call. So now we really can't make a reasonable guess whether a potential pointer really points to a MallocHeader. Since, in my opinion, we can't find a reasonable candidate (not sufficient clues), I decided to remove the code.

assert(MemTracker::enabled(), "NMT not enabled");

#if !INCLUDE_ASAN

address addr = (address)p;

if (p2u(addr) < MAX2(os::vm_min_address(), (size_t)16 * M)) {
return false; // bail out
}

// Carefully feel your way upwards and try to find a malloc header. Then check if
// we are within the block.
// We give preference to found live blocks; but if no live block had been found,
// but the pointer points into remnants of a dead block, print that instead.
const MallocHeader* likely_dead_block = nullptr;
const MallocHeader* likely_live_block = nullptr;
{
const size_t smallest_possible_alignment = sizeof(void*);
uintptr_t here = (uintptr_t)align_down(addr, smallest_possible_alignment);
uintptr_t end = MAX2(smallest_possible_alignment, here - (0x1000 + sizeof(MallocHeader))); // stop searching after 4k
for (; here >= end; here -= smallest_possible_alignment) {
// JDK-8306561: cast to a MallocHeader needs to guarantee it can reside in readable memory
if (!os::is_readable_range((void*)here, (void*)(here + sizeof(MallocHeader)))) {
break; // Probably OOB, give up
}
const MallocHeader* const candidate = (const MallocHeader*)here;
if (!candidate->looks_valid()) {
// This is definitely not a header, go on to the next candidate.
continue;
}

// fudge factor:
// We don't report blocks for which p is clearly outside of. That would cause us to return true and possibly prevent
// subsequent tests of p, see os::print_location(). But if p is just outside of the found block, this may be a
// narrow oob error and we'd like to know that.
const int fudge = 8;
const address start_block = (address)candidate;
const address start_payload = (address)(candidate + 1);
const address end_payload = start_payload + candidate->size();
const address end_payload_plus_fudge = end_payload + fudge;
if (addr >= start_block && addr < end_payload_plus_fudge) {
// We found a block the pointer is pointing into, or almost into.
// If its a live block, we have our info. If its a dead block, we still
// may be within the borders of a larger live block we have not found yet -
// continue search.
if (candidate->is_live()) {
likely_live_block = candidate;
break;
} else {
likely_dead_block = candidate;
continue;
}
}
}
}

// If we've found a reasonable candidate. Print the info.
const MallocHeader* block = likely_live_block != nullptr ? likely_live_block : likely_dead_block;
if (block != nullptr) {
const char* where = nullptr;
const address start_block = (address)block;
const address start_payload = (address)(block + 1);
const address end_payload = start_payload + block->size();
if (addr < start_payload) {
where = "into header of";
} else if (addr < end_payload) {
where = "into";
} else {
where = "just outside of";
}
st->print_cr(PTR_FORMAT " %s %s malloced block starting at " PTR_FORMAT ", size %zu, tag %s",
p2i(p), where,
(block->is_dead() ? "dead" : "live"),
p2i(block + 1), // lets print the payload start, not the header
block->size(), NMTUtil::tag_to_enum_name(block->mem_tag()));
if (MemTracker::tracking_level() == NMT_detail) {
NativeCallStack ncs;
if (MallocSiteTable::access_stack(ncs, *block)) {
ncs.print_on(st);
st->cr();
}
}
return true;
}

#endif // !INCLUDE_ASAN

return false;
}
4 changes: 1 addition & 3 deletions src/hotspot/share/nmt/memTracker.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,9 +122,7 @@ void MemTracker::final_report(outputStream* output) {
// Given an unknown pointer, check if it points into a known region; print region if found
// and return true; false if not found.
bool MemTracker::print_containing_region(const void* p, outputStream* out) {
return enabled() &&
(MallocTracker::print_pointer_information(p, out) ||
VirtualMemoryTracker::print_containing_region(p, out));
return enabled() && VirtualMemoryTracker::print_containing_region(p, out);
}

void MemTracker::report(bool summary_only, outputStream* output, size_t scale) {
Expand Down
4 changes: 0 additions & 4 deletions src/hotspot/share/runtime/os.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -718,15 +718,11 @@ void* os::realloc(void *memblock, size_t size, MemTag mem_tag, const NativeCallS
NMTUtil::tag_to_name(mem_tag), NMTUtil::tag_to_name(header->mem_tag()));
const MallocHeader::FreeInfo free_info = header->free_info();

header->mark_block_as_dead();

// the real realloc
ALLOW_C_FUNCTION(::realloc, void* const new_outer_ptr = ::realloc(header, new_outer_size);)

if (new_outer_ptr == nullptr) {
// realloc(3) failed and the block still exists.
// We have however marked it as dead, revert this change.
header->revive();
return nullptr;
}
// realloc(3) succeeded, variable header now points to invalid memory and we need to deaccount the old block.
Expand Down
Loading