Repository navigation
Conversation
When the current chunk is full, msgpack_zone_malloc asks msgpack_zone_malloc_expand for size + MSGPACK_ZONE_ALIGN - 1 bytes and then rounded the returned pointer down instead of up into that padding. The pointer sits right after the new chunk's next link, so when MSGPACK_ZONE_ALIGN is stricter than the header's alignment the object started inside the header and its first bytes overwrote the link. That is the default 32-bit sparc build (MSGPACK_ZONE_ALIGN=8 with a 4-byte header) and any build that raises MSGPACK_ZONE_ALIGN; unpacking a large array was enough to make msgpack_zone_free follow the element's type tag as a chunk pointer. Round up, as the fast path and the C++ zone already do. The padding was already reserved, so allocation sizes are unchanged.
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## c_master #1201 +/- ##
============================================
+ Coverage 57.47% 62.33% +4.86%
============================================
Files 10 10
Lines 1124 1147 +23
Branches 129 140 +11
============================================
+ Hits 646 715 +69
+ Misses 478 432 -46 🚀 New features to boost your workflow:
|
|
@rajath201 thank you for sending the PR. The fix looks good to me and I confirmed the SEGV with One minor point on the new test: test/zone_align_c.cpp defines test/zone_align_c.cpp:5:9: error: 'MSGPACK_ZONE_ALIGN' redefined [-Werror] CMakeLists.txt only adds the sparc setting to the C flags, so CI and the #undef MSGPACK_ZONE_ALIGN before the #define would make the test robust against that. Fine to do it |
Building the tests with -DMSGPACK_ZONE_ALIGN=... in CXXFLAGS made the test's own #define a macro redefinition, which is an error under -Werror.
|
Good catch, added the |
msgpack_zone_mallocasksmsgpack_zone_malloc_expandforsize + MSGPACK_ZONE_ALIGN - 1bytes when the current chunk is full, then rounds the returned pointer down instead of up into that padding. That pointer sits just past the new chunk'snextlink, so wheneverMSGPACK_ZONE_ALIGNis stricter than the header's alignment the object starts inside the header and its first bytes overwrite the link. This covers the default 32-bit sparc build (CMakeLists.txt setsMSGPACK_ZONE_ALIGN=8and the header is 4 bytes) and any build that raises the alignment: reproducing the sparc layout withgcc -m32 -DMSGPACK_ZONE_ALIGN=8, unpacking a 603-byte array16 of 600 fixints turns the link into the first element's type tag andmsgpack_unpacked_destroydies indestroy_chunk_list(ASan SEGV on0x00000002).Round up, as the fast path above, the C++
zone::get_alignedand the ASan path in #1195 already do. The padding was already reserved by the expand call, so allocation sizes don't change and the in-chunk fast path is untouched. Sincemsgpack_zone_mallocis inline, the newzone_align_ctest gets its own translation unit withMSGPACK_ZONE_ALIGNset to2 * sizeof(void*); it fails on the current tree at-m32and 64-bit and passes with the fix, and ctest is 7/7 across the Linux CI patterns (gcc/clang, 32/64-bit, ASan/UBSan) locally.