placement new UB/DSE

[stsp]

https://github.com/stsp/fdpp/blob/master/fdpp/farptr.hpp#L98 This placement new can trigger DSE: https://gcc.gnu.org/gcc-6/porting_to.html#flifetime-dse The problem was caused by a21427fd0. Clang doesn't seem to support -fno-lifetime-dse option. I wonder if the patch should be reverted. But the revert will likely cause strict aliasing violation.

[stsp]

One idea was to create wrappers with automatic storage, and memcpy() the data back and forth. Tried patch c9f76e33d but it fails to compile with

blockio.cc:396:23: error: expression is not assignable
    IoReqHdr.r_length = sizeof(request);
    ~~~~~~~~~~~~~~~~~ ^
blockio.cc:397:21: error: expression is not assignable
    IoReqHdr.r_unit = dpbp->dpb_subunit;
    ~~~~~~~~~~~~~~~ ^
blockio.cc:404:30: error: expression is not assignable
          IoReqHdr.r_command = C_OUTVFY;
          ~~~~~~~~~~~~~~~~~~ ^

etc.

All expressions became unassignable, because, if you return some aggregate as rvalue, you can't modify its fields (but you can if some member function returns an lvalue reference to the field you want to modify). Another C++ gotcha. Of course as a POC I can use new (but not placement new) and return an lvalue by dereferencing the obtained pointer. This would produce the memory leak, so not very practical, but should in theory work. But for some reason it doesn't. Maybe I am still bound to the size of the wrapper object, even if I don't see why. Would be nice to find out why, but... This ticket is probably not worth investing the time into.

[stsp]

Yeah, still bound to wrapper size by __ASYM() macro and glob_asmdefs.h. So tried patch 8c8a69eb5 to use new. This boots a bit but crashes. The problem is here: https://github.com/stsp/fdpp/blob/no_pnew/fdpp/farptr.hpp#L418 Obviously this pointer arithmetic can't work.

[stsp]

Got it to work with 1db08dd1, but it still doesn't fully boot, as with new the dtor is never called that way. Non-functional idea, it seems.

[stsp]

Turns out gcc allows to modify rvalue members with -fpermissive, but clang doesn't. :(

[stsp]

Added 2 tickets to clang, asking for gcc-compatible extensions: https://bugs.llvm.org/show_bug.cgi?id=40694 https://bugs.llvm.org/show_bug.cgi?id=40670 Of course it is very unlikely that they will be implemented. It seems clang doesn't like language extensions.

[stsp]

http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0476r2.html This paper may provide an alternative to the currently used placement new.

[stsp]

This paper may provide an alternative to the

Nope, it gives us nothing. But this does something very interesting: http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0593r2.html

[stsp]

In C++23 there will be std::start_lifetime_as(): http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p0593r5.html So this UB should wait 4 years or more. I would be surprised though if DSE can actually break our code because of this placement new. And I really don't anticipate placement new to start zeroing out memory or something like that. So the real breakage is very unlikely, and it may be better to concentrate on other problems.

[stsp]

https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2022/p2590r2.pdf This is the updated proposal on explicit object lifetime management that we need here.

[stsp]

This ticket seems to cover multiple topics. Summarizing it for the future version of myself some 10 years later when I get back to this.

DSE problem is going to be solved with the aforementioned paper in c++23. But there are the alternative methods discussed in this ticket. Even if the c++23 solution is applied, the alternatives will remain still interesting, for example from the gcc porting stand-point. They will then migrate to #225 All alternatives boils down to getting rid of a placement new, one way or another. If we have, for example, the overridable operator dot or member access via delegation, then we can just use pointers to the DOS memory without any extra headache with the packing of (non-)PODs. Unfortunately all such c++ proposals are stalled since 2017, so there is no hope in that method any more. The alternative discussed in this ticket, was to replace the placement new with the memcopy to the SymWrp with an automatic storage. This can work only if destructors run properly, which is not the case when you have eg wrp_type_s& get_wrp() { return *new(get_buf()) wrp_type_s; } which is what the current code does. So the idea in https://github.com/dosemu2/fdpp/commit/c9f76e33d27d0ad6e83b2ac76d1ff6a4c3f4c847 was to do wrp_type get_wrp() { return wrp_type(get_buf(), get_far()); } but that fails on clang and requires -fpermissive on gcc. The problem is that this returns a temporary object, and the later code is trying to assign the members of such temporary. Unfortunately the c++ standard currently disallows that. :( The test can be done this way:

diff --git a/fdpp/clang.mak b/fdpp/clang.mak
index 3c9dbe6..352006f 100644
--- a/fdpp/clang.mak
+++ b/fdpp/clang.mak
@@ -45,7 +45,8 @@ USE_UBSAN ?= 0

 IFLAGS = -iquote $(srcdir)/../hdr
 CPPFLAGS = $(IFLAGS) -DFDPP
-WFLAGS = -Wall -Werror=packed-non-pod -Wno-unknown-warning-option
+WFLAGS = -Wall -Werror=packed-non-pod -Wno-unknown-warning-option \
+-Wno-address-of-temporary
 ifneq ($(CLANG_VER),16)
 WFLAGS += -Wpacked
 endif
diff --git a/fdpp/farptr.hpp b/fdpp/farptr.hpp
index d34ccb8..b08779a 100644
--- a/fdpp/farptr.hpp
+++ b/fdpp/farptr.hpp
@@ -432,7 +432,7 @@ template<typename T, const int *st>
 class SymWrp : public T {
 public:
     SymWrp() = default;
-    SymWrp(const SymWrp&) = delete;
+//    SymWrp(const SymWrp&) = delete;
     SymWrp<T, st>& operator =(T& f) { *(T *)this = f; return *this; }
     FarPtr<T> operator &() const { return _MK_F(FarPtr<T>,
             lookup_far(st, this)); }
@@ -445,7 +445,7 @@ class SymWrp2 {

 public:
     SymWrp2() = default;
-    SymWrp2(const SymWrp2&) = delete;
+//    SymWrp2(const SymWrp2&) = delete;
     SymWrp2<T, st>& operator =(const T& f) { sym = f; return *this; }
     FarPtr<T> operator &() const { return _MK_F(FarPtr<T>,
             lookup_far(st, this)); }
@@ -711,7 +711,7 @@ public:
 #define __ASMREF(f) f.get_ref()
 #define __ASMADDR(v) __##v.get_addr()
 #define __ASMCALL(f) AsmCSym f
-#define __ASYM(x) x.get_sym()
+#define __ASYM(x) ({ x.get_sym(); })
 #define ASMREF(t) AsmRef<t>
 #define DUMMY_MARK(p, n) \
     static constexpr int off_##n(void) { return offsetof(p, n); }

This converts the result of __ASYM() macro to a temporary and shows that we also need to deal with the address-of applied to it. For example:

blockio.cc:110:19: error: taking the address of a temporary object of type 'FarPtrBase<buffer>' [-Waddress-of-temporary]
        *(UWORD *)&firstbuf = FP_OFF(bp);

But this is not a problem because the destructor of a temporary can do the trick, so in this example the error is suppressed. Only the "unassignable expressions" remain. Unfortunately I don't see any c++ proposals on that front, and clang guys are not too keen about implementing gcc's -fpermissive.

[stsp]

https://gcc.gnu.org/bugzilla//show_bug.cgi?id=106658 https://gcc.gnu.org/bugzilla//show_bug.cgi?id=101641

But I think clang doesn't do so aggressive DSE, so for a time we might be safe.

https://en.cppreference.com/w/cpp/memory/start_lifetime_as This is neither available in gcc nor in clang at the time of writing that. As soon as this is available. we need to switch to C++23 to finally plug that hole.

[stsp]

Only the "unassignable expressions" remain. Unfortunately I don't see any c++ proposals on that front, and clang guys are not too keen about implementing gcc's -fpermissive.

Smart references may circumvent that deficiency by relaying an access via internal pointers. That will probably work even with temporaries. See #129 But it seems smart references are not happening in c++23, so -fpermissive retains the viability.