[PATCH v2 0/1] MR10471: Draft: unwind: fix some invalid `reinterpret_cast`s
Performing type `reinterpret-cast`s between pointers to types that are not _pointers-interconvertible_ and then using the results is incorrect usage. Only the “array of N unsigned char” or of type “array of N std::byte” types can used as storage (in general case). To convert a pointer to a structure object containing a byte array into a pointer to an object explicitly created in that storage, std::launder must be called on that "newly typed" pointer. Accessing an explicitly created object in such a byte store through a pointer to an object of the structure type that contains the given byte array requires calling std::launder. ```cpp struct S { unsigned char data[_LIBUNWIND_CURSOR_SIZE * sizeof(uint64_t)]; } s; struct S2 { int i; }; new(s.data) S2(42); auto s2_ptr1 = reinterpret_cast<S2*>(s.data); auto s2_ptr2 = reinterpret_cast<S2*>(s); // this pointer is not changed ([expr.static.cast#12]) and points to s (struct S) auto s2_ptr3 = std::launder(reinterpret_cast<S2*>(s)); int i1 = s2_ptr1->i; // OK int i3 = s2_ptr3->i; // OK (?) int i2 = s2_ptr2->i; // this is undefined behavior ``` Also a pointer to object of type uint16\[2\] cannot be reinterpreted as uint32 -- this is undefined behavior (https://godbolt.org/z/vY1zKEvo6). https://eel.is/c++draft/intro.object#3 https://eel.is/c++draft/basic.compound#5 https://eel.is/c++draft/basic.compound#6 https://eel.is/c++draft/basic.lval#11 https://eel.is/c++draft/expr.static.cast#12 https://eel.is/c++draft/expr.reinterpret.cast#7 https://en.cppreference.com/w/cpp/utility/launder.html https://en.cppreference.com/w/cpp/language/reinterpret_cast.html -- v2: unwind: fix some invalid `reinterpret_cast`s https://gitlab.winehq.org/wine/wine/-/merge_requests/10471
From: Safocl Stollmannovic <safocl88@gmail.com> Some `reinterpret_cast`s were invalid. https://eel.is/c++draft/basic.compound#5 https://eel.is/c++draft/basic.compound#6 https://eel.is/c++draft/basic.lval#11 https://eel.is/c++draft/expr.static.cast#12 https://eel.is/c++draft/expr.reinterpret.cast#7 https://en.cppreference.com/w/cpp/utility/launder.html --- libs/unwind/include/libunwind.h | 8 +++++++- libs/unwind/src/Unwind-seh.cpp | 21 +++++++++------------ libs/unwind/src/UnwindCursor.hpp | 16 +++++++++++++--- libs/unwind/src/libunwind.cpp | 9 ++++----- 4 files changed, 33 insertions(+), 21 deletions(-) diff --git a/libs/unwind/include/libunwind.h b/libs/unwind/include/libunwind.h index df596ee0bfd..9d12be47ae2 100644 --- a/libs/unwind/include/libunwind.h +++ b/libs/unwind/include/libunwind.h @@ -68,7 +68,13 @@ struct unw_context_t { typedef struct unw_context_t unw_context_t; struct unw_cursor_t { - uint64_t data[_LIBUNWIND_CURSOR_SIZE]; + /* Only the “array of N unsigned char” or of type “array of N std::byte” types can used as + storage (https://eel.is/c++draft/intro.object#3) */ + union { + unsigned char data[_LIBUNWIND_CURSOR_SIZE * sizeof(uint64_t)]; + /* `aligner` exists only for alignment */ + uint64_t aligner__[_LIBUNWIND_CURSOR_SIZE]; + }; }; typedef struct unw_cursor_t unw_cursor_t; diff --git a/libs/unwind/src/Unwind-seh.cpp b/libs/unwind/src/Unwind-seh.cpp index 078edc97d03..588a9d4283c 100644 --- a/libs/unwind/src/Unwind-seh.cpp +++ b/libs/unwind/src/Unwind-seh.cpp @@ -442,21 +442,18 @@ _Unwind_GetRegionStart(struct _Unwind_Context *context) { static int _unw_init_seh(unw_cursor_t *cursor, CONTEXT *context) { #ifdef _LIBUNWIND_TARGET_X86_64 - new ((void *)cursor) UnwindCursor<LocalAddressSpace, Registers_x86_64>( + auto *co = new (cursor->data) UnwindCursor<LocalAddressSpace, Registers_x86_64>( context, LocalAddressSpace::sThisAddressSpace); - auto *co = reinterpret_cast<AbstractUnwindCursor *>(cursor); co->setInfoBasedOnIPRegister(); return UNW_ESUCCESS; #elif defined(_LIBUNWIND_TARGET_ARM) - new ((void *)cursor) UnwindCursor<LocalAddressSpace, Registers_arm>( + auto *co = new (cursor->data) UnwindCursor<LocalAddressSpace, Registers_arm>( context, LocalAddressSpace::sThisAddressSpace); - auto *co = reinterpret_cast<AbstractUnwindCursor *>(cursor); co->setInfoBasedOnIPRegister(); return UNW_ESUCCESS; #elif defined(_LIBUNWIND_TARGET_AARCH64) - new ((void *)cursor) UnwindCursor<LocalAddressSpace, Registers_arm64>( + auto *co = new (cursor->data) UnwindCursor<LocalAddressSpace, Registers_arm64>( context, LocalAddressSpace::sThisAddressSpace); - auto *co = reinterpret_cast<AbstractUnwindCursor *>(cursor); co->setInfoBasedOnIPRegister(); return UNW_ESUCCESS; #else @@ -467,11 +464,11 @@ _unw_init_seh(unw_cursor_t *cursor, CONTEXT *context) { static DISPATCHER_CONTEXT * _unw_seh_get_disp_ctx(unw_cursor_t *cursor) { #ifdef _LIBUNWIND_TARGET_X86_64 - return reinterpret_cast<UnwindCursor<LocalAddressSpace, Registers_x86_64> *>(cursor)->getDispatcherContext(); + return reinterpret_cast<UnwindCursor<LocalAddressSpace, Registers_x86_64> *>(cursor->data)->getDispatcherContext(); #elif defined(_LIBUNWIND_TARGET_ARM) - return reinterpret_cast<UnwindCursor<LocalAddressSpace, Registers_arm> *>(cursor)->getDispatcherContext(); + return reinterpret_cast<UnwindCursor<LocalAddressSpace, Registers_arm> *>(cursor->data)->getDispatcherContext(); #elif defined(_LIBUNWIND_TARGET_AARCH64) - return reinterpret_cast<UnwindCursor<LocalAddressSpace, Registers_arm64> *>(cursor)->getDispatcherContext(); + return reinterpret_cast<UnwindCursor<LocalAddressSpace, Registers_arm64> *>(cursor->data)->getDispatcherContext(); #else return nullptr; #endif @@ -480,11 +477,11 @@ _unw_seh_get_disp_ctx(unw_cursor_t *cursor) { static void _unw_seh_set_disp_ctx(unw_cursor_t *cursor, DISPATCHER_CONTEXT *disp) { #ifdef _LIBUNWIND_TARGET_X86_64 - reinterpret_cast<UnwindCursor<LocalAddressSpace, Registers_x86_64> *>(cursor)->setDispatcherContext(disp); + reinterpret_cast<UnwindCursor<LocalAddressSpace, Registers_x86_64> *>(cursor->data)->setDispatcherContext(disp); #elif defined(_LIBUNWIND_TARGET_ARM) - reinterpret_cast<UnwindCursor<LocalAddressSpace, Registers_arm> *>(cursor)->setDispatcherContext(disp); + reinterpret_cast<UnwindCursor<LocalAddressSpace, Registers_arm> *>(cursor->data)->setDispatcherContext(disp); #elif defined(_LIBUNWIND_TARGET_AARCH64) - reinterpret_cast<UnwindCursor<LocalAddressSpace, Registers_arm64> *>(cursor)->setDispatcherContext(disp); + reinterpret_cast<UnwindCursor<LocalAddressSpace, Registers_arm64> *>(cursor->data)->setDispatcherContext(disp); #endif } diff --git a/libs/unwind/src/UnwindCursor.hpp b/libs/unwind/src/UnwindCursor.hpp index 52439f9b545..7a157c7f8b7 100644 --- a/libs/unwind/src/UnwindCursor.hpp +++ b/libs/unwind/src/UnwindCursor.hpp @@ -1772,9 +1772,19 @@ bool UnwindCursor<A, R>::getInfoFromSEH(pint_t pc) { // these structures.) // N.B. UNWIND_INFO structs are DWORD-aligned. uint32_t lastcode = (xdata->CountOfCodes + 1) & ~1; - const uint32_t *handler = reinterpret_cast<uint32_t *>(&xdata->UnwindCodes[lastcode]); - _info.lsda = reinterpret_cast<unw_word_t>(handler+1); - if (*handler) { + // NOTE: lastcode can be equal to or greater than 2, then accessing UnwindCodes[lastcode] is + // out of bound (i.e. undefined behavior). The external memory outside the class object + // cannot be reached from a pointer to a subobject. The only valid case is when CountOfCodes + // is 0, and then lastcode will be equal 0. + // However, `reinterpret_cast` from a pointer to uint16[2] to a pointer to uint32 is not + // allowed in any case. (https://godbolt.org/z/vY1zKEvo6) + // It turns out that here we just need to check each of the array elements for zero. + const uint16_t hi = xdata->UnwindCodes[0]; + const uint16_t low = xdata->UnwindCodes[1]; +#warning "invalid reinterpret-cast" + // FIXME: Does `xdata` actually point to an array of UNWIND_INFO? + _info.lsda = reinterpret_cast<unw_word_t>(xdata+1); + if (hi || low) { _info.handler = reinterpret_cast<unw_word_t>(__libunwind_seh_personality); } else _info.handler = 0; diff --git a/libs/unwind/src/libunwind.cpp b/libs/unwind/src/libunwind.cpp index 5afe6235c4c..b5421136768 100644 --- a/libs/unwind/src/libunwind.cpp +++ b/libs/unwind/src/libunwind.cpp @@ -73,10 +73,9 @@ _LIBUNWIND_EXPORT int unw_init_local(unw_cursor_t *cursor, # error Architecture not supported #endif // Use "placement new" to allocate UnwindCursor in the cursor buffer. - new ((void *)cursor) UnwindCursor<LocalAddressSpace, REGISTER_KIND>( + auto *co = new (cursor->data) UnwindCursor<LocalAddressSpace, REGISTER_KIND>( context, LocalAddressSpace::sThisAddressSpace); #undef REGISTER_KIND - AbstractUnwindCursor *co = (AbstractUnwindCursor *)cursor; co->setInfoBasedOnIPRegister(); return UNW_ESUCCESS; @@ -94,17 +93,17 @@ _LIBUNWIND_EXPORT int unw_init_remote_thread(unw_cursor_t *cursor, // use "placement new" to allocate UnwindCursor in the cursor buffer switch (as->cpuType) { case CPU_TYPE_I386: - new ((void *)cursor) + new (cursor->data) UnwindCursor<RemoteAddressSpace<Pointer32<LittleEndian>>, Registers_x86>(((unw_addr_space_i386 *)as)->oas, arg); break; case CPU_TYPE_X86_64: - new ((void *)cursor) + new (cursor->data) UnwindCursor<RemoteAddressSpace<Pointer64<LittleEndian>>, Registers_x86_64>(((unw_addr_space_x86_64 *)as)->oas, arg); break; case CPU_TYPE_POWERPC: - new ((void *)cursor) + new (cursor->data) UnwindCursor<RemoteAddressSpace<Pointer32<BigEndian>>, Registers_ppc>(((unw_addr_space_ppc *)as)->oas, arg); break; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/10471
On Fri Mar 27 13:51:01 2026 +0000, Safocl Stollmannovic wrote:
But the LLVM project doesn't use the placement-new-operator to create new objects in storage. They use the corresponding compiler intrinsics (https://github.com/llvm/llvm-project/blob/abc0674f839ab4b932f5cc6e15da045668...). However, here (and in several other code snippets) the placement-new operator is used: https://gitlab.winehq.org/wine/wine/-/blob/master/libs/unwind/src/libunwind.... Oh, I think I understand what you were talking about – the mistakenly set AND instead of OR. I've corrected it – thanks.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/10471#note_134067
it's difficult to predict what was actually intended
If you're taking 'every per-the-spec UB is unreachable code' as an axiom, then you live in the utopian world of academia, and not in the messy world of programming in practice. In this case, I strongly suspect that struct UNWIND_INFO contains some kind of variable-size flexible array member, meaning that accessing it as if it was bigger will do the right thing in practice. (Also, fair chance it's UB per the C++ spec, but the compilers choose to implement it as doing the obvious memory operations.) And yes, the && || thing you fixed is the other change I'm thinking of. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/10471#note_134068
On Fri Mar 27 14:08:55 2026 +0000, Safocl Stollmannovic wrote:
Oh, I think I understand what you were talking about – the mistakenly set AND instead of OR. I've corrected it – thanks. __unw_init_local isn't a compiler intrinsic, it's just a normal function with a funny name.
Its implementation is just a placement new. https://github.com/llvm/llvm-project/blob/52a880d30ba6cba38579ad7f51289d9ebe... -- https://gitlab.winehq.org/wine/wine/-/merge_requests/10471#note_134071
(Also, fair chance it's UB per the C++ spec, but the compilers choose to implement it as doing the obvious memory operations.)
As far as I can tell, no compiler guarantees normal access to memory outside an object through a pointer to its data subobject. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/10471#note_134087
On Fri Mar 27 14:20:31 2026 +0000, Alfred Agrell wrote:
__unw_init_local isn't a compiler intrinsic, it's just a normal function with a funny name. Its implementation is just a placement new. https://github.com/llvm/llvm-project/blob/52a880d30ba6cba38579ad7f51289d9ebe... I created a question:
https://github.com/llvm/llvm-project/issues/189040 -- https://gitlab.winehq.org/wine/wine/-/merge_requests/10471#note_134096
I didn't initially assume that this code was copied directly from the compiler. In that case, I would have addressed it directly. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/10471#note_134097
On Fri Mar 27 15:08:32 2026 +0000, Safocl Stollmannovic wrote:
(Also, fair chance it's UB per the C++ spec, but the compilers choose to implement it as doing the obvious memory operations.) As far as I can tell, no compiler guarantees normal access to memory outside an object through a pointer to its data subobject. As far as I can tell, all compilers do, in fact, perform the obvious memory ops if given a struct with a trailing array. None of them deem it UB. https://godbolt.org/z/17hrvMrv4
If you're saying that's a missed-optimization bug and could change in future versions, I won't disagree, but then you're in https://gitlab.winehq.org/wine/wine/-/merge_requests/10471#note_134053 territory. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/10471#note_134175
On Fri Mar 27 22:56:13 2026 +0000, Alfred Agrell wrote:
As far as I can tell, all compilers do, in fact, perform the obvious memory ops if given a struct with a trailing array. None of them deem it UB. https://godbolt.org/z/17hrvMrv4 If you're saying that's a missed-optimization bug and could change in future versions, I won't disagree, but then you're in https://gitlab.winehq.org/wine/wine/-/merge_requests/10471#note_134053 territory. This is more of a philosophical discussion, but I will still say that any program behavior is possible for such code -- including as in the specific case -- there are no guarantees of predictability and implementation requirements from the standard.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/10471#note_134185
None of them deem it UB. https://godbolt.org/z/17hrvMrv4
```asm bar(foo*, unsigned int): xor eax, eax test esi, esi mov esi, esi setne al sal eax, 16 add eax, DWORD PTR [rdi+4+rsi*4] ret ``` As far as I understand, only one branch is used here. So, the manifestation of UB is obvious. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/10471#note_134186
On Sat Mar 28 00:01:53 2026 +0000, Safocl Stollmannovic wrote:
None of them deem it UB. https://godbolt.org/z/17hrvMrv4
bar(foo*, unsigned int): xor eax, eax test esi, esi mov esi, esi setne al sal eax, 16 add eax, DWORD PTR [rdi+4+rsi*4] retAs far as I understand, only one branch is used here. So, the manifestation of UB is obvious. UB is not a thing at asm level. The closest you'll get is a race condition, and even that will only corrupt the affected memory address, nothing else.
If the compilers deemed n!=0 to be UB and optimized accordingly, then the resulting asm would be `mov eax, [rdi+4]; ret` and nothing else. I have no idea what you think that second branch would do. Do you want buffer overflows to jump to `abort` or [something](https://godbolt.org/z/3dnab7d9q)? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/10471#note_134188
UB is not a thing at asm level.
but we don't write this code in assembly language - it's written in C or C++. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/10471#note_134192
On Sat Mar 28 01:31:24 2026 +0000, Safocl Stollmannovic wrote:
UB is not a thing at asm level. but we don't write this code in assembly language - it's written in C or C++. no one and nothing should guarantee that a program with undefined behavior should do
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/10471#note_134194
participants (3)
-
Alfred Agrell (＠Alcaro) -
Safocl Stollmannovic
-
Safocl Stollmannovic (＠safocl88)