Skip to content
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.

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

placement new UB/DSE #56

Open
stsp opened this issue Feb 6, 2019 · 12 comments
Open

placement new UB/DSE #56

stsp opened this issue Feb 6, 2019 · 12 comments

Comments

@stsp
Copy link
Member

stsp commented Feb 6, 2019

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 a21427f.
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 stsp changed the title placement new UD placement new UB Feb 6, 2019
@stsp
Copy link
Member Author

stsp commented Feb 8, 2019

One idea was to create wrappers with automatic
storage, and memcpy() the data back and forth.
Tried patch c9f76e3 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
Copy link
Member Author

stsp commented Feb 8, 2019

Yeah, still bound to wrapper size by __ASYM() macro
and glob_asmdefs.h. So tried patch 8c8a69e 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
Copy link
Member Author

stsp commented Feb 8, 2019

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

@stsp
Copy link
Member Author

stsp commented Feb 8, 2019

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

stsp added a commit that referenced this issue Feb 9, 2019
This avoids strict-aliasing violation casts and unbinds
us from the wrapper size.
@stsp
Copy link
Member Author

stsp commented Feb 11, 2019

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
Copy link
Member Author

stsp commented Feb 11, 2019

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
Copy link
Member Author

stsp commented Mar 14, 2019

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 stsp added the postponed label Apr 29, 2019
@stsp
Copy link
Member Author

stsp commented Nov 16, 2019

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 stsp added the c++23 label Mar 3, 2020
@stsp stsp changed the title placement new UB placement new UB/DSE May 13, 2023
@stsp
Copy link
Member Author

stsp commented May 13, 2023

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
Copy link
Member Author

stsp commented May 16, 2023

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 c9f76e3
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
Copy link
Member Author

stsp commented Jul 14, 2023

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
Copy link
Member Author

stsp commented Jul 14, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant