From 792dec14a5bd6469d7f5b9d4dfc5f9dba5fe117b Mon Sep 17 00:00:00 2001 From: Jaroslav Bachorik Date: Tue, 17 Dec 2024 15:52:38 +0100 Subject: [PATCH 1/2] Fix Dwarf CFA encoding for negative offsets --- ddprof-lib/src/main/cpp/dwarf.cpp | 19 +++++++++++++------ ddprof-lib/src/main/cpp/dwarf.h | 5 +++-- ddprof-lib/src/main/cpp/stackWalker.cpp | 8 +++++++- 3 files changed, 23 insertions(+), 9 deletions(-) diff --git a/ddprof-lib/src/main/cpp/dwarf.cpp b/ddprof-lib/src/main/cpp/dwarf.cpp index eb7a169d..8ef79b83 100644 --- a/ddprof-lib/src/main/cpp/dwarf.cpp +++ b/ddprof-lib/src/main/cpp/dwarf.cpp @@ -15,7 +15,9 @@ */ #include "dwarf.h" +#include "common.h" #include "log.h" +#include #include enum { @@ -185,10 +187,10 @@ void DwarfParser::parseInstructions(u32 loc, const char *end) { int fp_off = DW_SAME_FP; int pc_off = -EMPTY_FRAME_SIZE; - u32 rem_cfa_reg; - int rem_cfa_off; - int rem_fp_off; - int rem_pc_off; + u32 rem_cfa_reg = DW_REG_SP; + int rem_cfa_off = EMPTY_FRAME_SIZE; + int rem_fp_off = DW_SAME_FP; + int rem_pc_off = -EMPTY_FRAME_SIZE; while (_ptr < end) { u8 op = get8(); @@ -396,14 +398,19 @@ int DwarfParser::parseExpression() { void DwarfParser::addRecord(u32 loc, u32 cfa_reg, int cfa_off, int fp_off, int pc_off) { - int cfa = cfa_reg | cfa_off << 8; + // sanity asserts for the values fitting into 16bit + assert(cfa_reg <= 0xffffffff); + assert(static_cast(cfa_off) <= 0xffffffff); + + // cfa_reg and cfa_off can be encoded to a single 32 bit value, considering the existing and supported systems + u32 cfa = static_cast(cfa_off) << 16 | static_cast(cfa_reg); if (_prev == NULL || (_prev->loc == loc && --_count >= 0) || _prev->cfa != cfa || _prev->fp_off != fp_off || _prev->pc_off != pc_off) { _prev = addRecordRaw(loc, cfa, fp_off, pc_off); } } -FrameDesc *DwarfParser::addRecordRaw(u32 loc, int cfa, int fp_off, int pc_off) { +FrameDesc *DwarfParser::addRecordRaw(u32 loc, u32 cfa, int fp_off, int pc_off) { if (_count >= _capacity) { FrameDesc *frameDesc = (FrameDesc *)realloc(_table, _capacity * 2 * sizeof(FrameDesc)); diff --git a/ddprof-lib/src/main/cpp/dwarf.h b/ddprof-lib/src/main/cpp/dwarf.h index 356bdc44..91e1746b 100644 --- a/ddprof-lib/src/main/cpp/dwarf.h +++ b/ddprof-lib/src/main/cpp/dwarf.h @@ -71,7 +71,7 @@ const int LINKED_FRAME_SIZE = 0; struct FrameDesc { u32 loc; - int cfa; + u32 cfa; int fp_off; int pc_off; @@ -106,6 +106,7 @@ class DwarfParser { } u8 get8() { return *_ptr++; } + // We are getting alignment issues when loading the 16-bit value // todo: are these relevant and well handled ? __attribute__((no_sanitize("undefined"))) u16 get16() { @@ -159,7 +160,7 @@ class DwarfParser { int parseExpression(); void addRecord(u32 loc, u32 cfa_reg, int cfa_off, int fp_off, int pc_off); - FrameDesc *addRecordRaw(u32 loc, int cfa, int fp_off, int pc_off); + FrameDesc *addRecordRaw(u32 loc, u32 cfa, int fp_off, int pc_off); public: DwarfParser(const char *name, const char *image_base, diff --git a/ddprof-lib/src/main/cpp/stackWalker.cpp b/ddprof-lib/src/main/cpp/stackWalker.cpp index 6df11e92..24b56f79 100644 --- a/ddprof-lib/src/main/cpp/stackWalker.cpp +++ b/ddprof-lib/src/main/cpp/stackWalker.cpp @@ -15,6 +15,7 @@ */ #include "stackWalker.h" +#include "common.h" #include "dwarf.h" #include "libraries.h" #include "profiler.h" @@ -170,7 +171,8 @@ int StackWalker::walkDwarf(void *ucontext, const void **callchain, cc != NULL ? cc->findFrameDesc(pc) : &FrameDesc::default_frame; u8 cfa_reg = (u8)f->cfa; - int cfa_off = f->cfa >> 8; + int cfa_off = f->cfa >> 16; // cfa is encoded in the upper 16 bits of the CFA value + if (cfa_reg == DW_REG_SP) { sp = sp + cfa_off; } else if (cfa_reg == DW_REG_FP) { @@ -225,6 +227,10 @@ int StackWalker::walkDwarf(void *ucontext, const void **callchain, return depth; } +#ifdef __aarch64__ +// we are seeing false alarms on aarch64 GHA runners due to 'heap-use-after-free' +__attribute__((no_sanitize("address"))) +#endif int StackWalker::walkVM(void *ucontext, ASGCT_CallFrame *frames, int max_depth, const void *_termination_frame_begin, const void *_termination_frame_end) { From 0f304e26608a18b0c9bc45ff8474827e976fd474 Mon Sep 17 00:00:00 2001 From: Jaroslav Bachorik Date: Tue, 17 Dec 2024 15:53:28 +0100 Subject: [PATCH 2/2] Avoid Asan false positives on aarch64 --- ddprof-lib/src/main/cpp/buffers.h | 59 +++++++++++++++++++++-- ddprof-lib/src/main/cpp/symbols_linux.cpp | 12 +++-- 2 files changed, 62 insertions(+), 9 deletions(-) diff --git a/ddprof-lib/src/main/cpp/buffers.h b/ddprof-lib/src/main/cpp/buffers.h index 4af7723e..1edf039b 100644 --- a/ddprof-lib/src/main/cpp/buffers.h +++ b/ddprof-lib/src/main/cpp/buffers.h @@ -3,6 +3,7 @@ #include #include +#include #include #include @@ -29,6 +30,7 @@ class Buffer { private: int _offset; static const int _limit = BUFFER_SIZE - sizeof(int); +protected: // this array is 'extended' by the RecordingBuffer // this will confuse sanitizers and most of the sane people but it seems to // work @@ -63,17 +65,29 @@ class Buffer { void reset() { _offset = 0; } + #ifdef __aarch64__ + // the trickery of RecordingBuffer extending Buffer::_data array may trip off asan on aarch64 + __attribute__((no_sanitize("bounds"))) + #endif void put(const char *v, u32 len) { assert(static_cast(_offset + len) < limit()); memcpy(_data + _offset, v, len); _offset += (int)len; } + #ifdef __aarch64__ + // the trickery of RecordingBuffer extending Buffer::_data array may trip off asan on aarch64 + __attribute__((no_sanitize("bounds"))) + #endif void put8(char v) { assert(_offset < limit()); _data[_offset++] = v; } + #ifdef __aarch64__ + // the trickery of RecordingBuffer extending Buffer::_data array may trip off asan on aarch64 + __attribute__((no_sanitize("bounds"))) + #endif void put16(short v) { assert(_offset + 2 < limit()); *(short *)(_data + _offset) = htons(v); @@ -83,7 +97,12 @@ class Buffer { // java-profiler/ddprof-lib/src/main/cpp/buffers.h:92:34: runtime error: // store to misaligned address 0x7f3c446ec81e for type 'int', which // requires 4 byte alignment 0x7f3c446ec81e: note: pointer points here - __attribute__((no_sanitize("undefined"))) void put32(int v) { + __attribute__((no_sanitize("undefined"))) + #ifdef __aarch64__ + // the trickery of RecordingBuffer extending Buffer::_data array may trip off asan on aarch64 + __attribute__((no_sanitize("bounds"))) + #endif + void put32(int v) { assert(_offset + 4 < limit()); *(int *)(_data + _offset) = htonl(v); _offset += 4; @@ -93,12 +112,19 @@ class Buffer { // runtime error: store to misaligned address 0x766bb5a1e814 for type // 'u64', which requires 8 byte alignment // 0x766bb5a1e814: note: pointer points here - __attribute__((no_sanitize("undefined"))) void put64(u64 v) { + __attribute__((no_sanitize("undefined"))) + #ifdef __aarch64__ + // the trickery of RecordingBuffer extending Buffer::_data array may trip off asan on aarch64 + __attribute__((no_sanitize("bounds"))) + #endif + void put64(u64 v) { assert(_offset + 8 < limit()); *(u64 *)(_data + _offset) = OS::hton64(v); _offset += 8; } + // the trickery of RecordingBuffer extending Buffer::_data array may trip off asan + __attribute__((no_sanitize("bounds"))) void putFloat(float v) { union { float f; @@ -109,6 +135,10 @@ class Buffer { put32(u.i); } + #ifdef __aarch64__ + // the trickery of RecordingBuffer extending Buffer::_data array may trip off asan on aarch64 + __attribute__((no_sanitize("bounds"))) + #endif void putVar32(u32 v) { assert(_offset + 5 < limit()); while (v > 0x7f) { @@ -118,6 +148,10 @@ class Buffer { _data[_offset++] = (char)v; } + #ifdef __aarch64__ + // the trickery of RecordingBuffer extending Buffer::_data array may trip off asan on aarch64 + __attribute__((no_sanitize("bounds"))) + #endif void putVar64(u64 v) { assert(_offset + 9 < limit()); int iter = 0; @@ -138,6 +172,8 @@ class Buffer { _data[_offset++] = (char)v; } + // the trickery of RecordingBuffer extending Buffer::_data array may trip off asan + __attribute__((no_sanitize("bounds"))) void putUtf8(const char *v) { if (v == NULL) { put8(0); @@ -147,6 +183,10 @@ class Buffer { } } + #ifdef __aarch64__ + // the trickery of RecordingBuffer extending Buffer::_data array may trip off asan on aarch64 + __attribute__((no_sanitize("bounds"))) + #endif void putUtf8(const char *v, u32 len) { len = len < MAX_STRING_LENGTH ? len : MAX_STRING_LENGTH; put8(3); @@ -154,8 +194,16 @@ class Buffer { put(v, len); } + #ifdef __aarch64__ + // the trickery of RecordingBuffer extending Buffer::_data array may trip off asan on aarch64 + __attribute__((no_sanitize("bounds"))) + #endif void put8(int offset, char v) { _data[offset] = v; } + #ifdef __aarch64__ + // the trickery of RecordingBuffer extending Buffer::_data array may trip off asan on aarch64 + __attribute__((no_sanitize("bounds"))) + #endif void putVar32(int offset, u32 v) { _data[offset] = v | 0x80; _data[offset + 1] = (v >> 7) | 0x80; @@ -170,13 +218,16 @@ class RecordingBuffer : public Buffer { static const int _limit = RECORDING_BUFFER_SIZE - sizeof(Buffer); // we reserve 8KiB to overflow in to in case event serialisers in // the flight recorder are buggy. If we ever use the overflow, - // which is sized to accomodate the largest possible string, we + // which is sized to accommodate the largest possible string, we // will truncate and may produce a corrupt recording, but we will // not write into arbitrary memory. char _buf[_limit + RECORDING_BUFFER_OVERFLOW]; public: - RecordingBuffer() : Buffer() { memset(_buf, 0, _limit); } + RecordingBuffer() : Buffer() { + new (_data + sizeof(_data)) char[sizeof(_buf)]; + memset(_buf, 0, _limit); + } int limit() const override { return _limit; } diff --git a/ddprof-lib/src/main/cpp/symbols_linux.cpp b/ddprof-lib/src/main/cpp/symbols_linux.cpp index 94ba54ab..891a4bec 100644 --- a/ddprof-lib/src/main/cpp/symbols_linux.cpp +++ b/ddprof-lib/src/main/cpp/symbols_linux.cpp @@ -17,6 +17,8 @@ #ifdef __linux__ #include "symbols_linux.h" + +#include "common.h" #include "dwarf.h" #include "log.h" #include "safeAccess.h" @@ -492,10 +494,10 @@ static int parseLibrariesCallback(struct dl_phdr_info *info, size_t size, } const char *map_end = map.end(); - CodeCache *cc = new CodeCache(map.file(), count, false, map_start, map_end); - // Do not try to parse pseudofiles like anon_inode:name, /memfd:name if (strchr(map.file(), ':') == NULL) { + CodeCache *cc = new CodeCache(map.file(), count, false, map_start, map_end); + TEST_LOG("Procesing library: %s", map.file()); u64 inode = u64(map.dev()) << 32 | map.inode(); if (inode != 0) { // Do not parse the same executable twice, e.g. on Alpine Linux @@ -516,10 +518,10 @@ static int parseLibrariesCallback(struct dl_phdr_info *info, size_t size, } else if (strcmp(map.file(), "[vdso]") == 0) { ElfParser::parseProgramHeaders(cc, map_start, map_end, true); } - } - cc->sort(); - array->add(cc); + cc->sort(); + array->add(cc); + } } free(str);