ned14 / status-code

Proposed SG14 status_code for the C++ standard
Other
64 stars 13 forks source link

type erasure construction from erased codes should not be allowed #50

Closed BurningEnlightenment closed 1 year ago

BurningEnlightenment commented 1 year ago

https://github.com/ned14/status-code/blob/16617997f9d5054b5f96a220c0a6477dfdda1304/include/status-code/status_code.hpp#L608-L621 These type erasing constructors need to be further constrained to exclude status_code<erased<>>, because currently one can easily cause use after free issues:

#include <https://raw.githubusercontent.com/ned14/status-code/master/single-header/system_error2.hpp>

using namespace system_error2;

auto do_evil2(posix_code c) -> system_code
{
    auto p = make_status_code_ptr(c);
    system_code c1 = p;
    return p;
}

with clang 15 and -std=c++20 -O2 we get:

do_evil2(system_error2::status_code<system_error2::_posix_code_domain>): # @do_evil2(system_error2::status_code<system_error2::_posix_code_domain>)
        push    rbx
        mov     edi, 16
        call    operator new(unsigned long)@PLT
        mov     rbx, rax
        mov     rdi, rax
        call    operator delete(void*)@PLT
        lea     rax, [rip + system_error2::detail::_indirecting_domain<system_error2::status_code<system_error2::_posix_code_domain> >]
        mov     rdx, rbx
        pop     rbx
        ret

(optimizers are really amazing, the operator delete call happens due to c1 being destructed; see reprex on godbolt)


Btw. what are the exact type requirements for a domain's value_type to be erasable? Is a std::shared_ptr/ref ptr/com ptr for rich error information within a value_type permissible/ought to be supported? I'm asking, because the union bit_cast fails to compile if From has a destructor...

BurningEnlightenment commented 1 year ago

@ned14 did you miss this?

ned14 commented 1 year ago

No, just waiting for a morning which has been rained out so I can look at it properly. Sorry for the delay, blame recent good weather in Ireland :)

ned14 commented 1 year ago

We had this implicit self-wrapping problem in Outcome, which was solved by disabling implicit construction from self-related types and adding a tagged in_place_type<T> overload for those users who really do actually want a status code whose payload is a status code.

You can have a crack at that if you want? Or we can wait for my next window of free time. There may be an opportunity in two weeks as work lets me have two days per month to write standards papers now, and my current paper is the next revision of P1028.

ned14 commented 1 year ago

Today is that day from work I am allowed work on standards papers, so I looked into this one and yeah, that's a real nasty bug which also affects our work code base. This is why employers should fund standards papers! :)

To answer your question:

Btw. what are the exact type requirements for a domain's value_type to be erasable? Is a std::shared_ptr/ref ptr/com ptr for rich error information within a value_type permissible/ought to be supported? I'm asking, because the union bit_cast fails to compile if From has a destructor...

You can't construct an erased status code from a status code with a non-trivial destructor. So, if the payload has a non trivial destructor, those status codes cannot be erased.

But, we do need to support non-trivial destructors so we can wrap smart pointers! So we have this technique where the domain can define how to destroy an erased status code. Erased status codes just then need to ensure they only ever move, and never copy, and all is good.

The copy constructor you identified did not filter out erased status code inputs, and so allowed a copy, and that in turn broke everything. Only moves should be allowed there.