lw_shared_ptr triggers false-positive use-after-free warning in GCC

[avelanarius]

The following correct lw_shared_ptr usecase triggers use-after-free warning when compiling with latest GCC (g++ (GCC) 12.0.1 20220404 (experimental)):

future<> repro() {
    auto nr_conn = make_lw_shared<size_t>(0);
    auto lambda = [nr_conn] () {
       return make_ready_future<>().then([nr_conn]{});
    };
    return lambda();
}

Error:

In destructor ‘seastar::lw_shared_ptr<T>::~lw_shared_ptr() [with T = long unsigned int]’,
    inlined from ‘seastar::future<> repro()’ at generic_server.cc:24:1:
/root/scylla/seastar/include/seastar/core/shared_ptr.hh:300:26: error: pointer used after ‘void operator delete(void*, std::size_t)’ [-Werror=use-after-free]
  300 |         if (_p && !--_p->_count) {
      |                      ~~~~^~~~~~
In destructor ‘seastar::lw_shared_ptr<T>::~lw_shared_ptr() [with T = long unsigned int]’,
    inlined from ‘repro()::<lambda()>::~<lambda>()’ at generic_server.cc:20:27,
    inlined from ‘seastar::future<> repro()’ at generic_server.cc:24:1:
/root/scylla/seastar/include/seastar/core/shared_ptr.hh:301:15: note: call to ‘void operator delete(void*, std::size_t)’ here
  301 |               delete static_cast<concrete_type*>(_p);
      |               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1plus: all warnings being treated as errors
ninja: build stopped: subcommand failed.

It looks like this is a false positive, as there doesn't seem to be any problem with Seastar code. With g++ (GCC) 11.2.1 20220127 (Red Hat 11.2.1-9) it compiles fine.

I have not yet reported the bug to GCC (no existing bug report one matches it 1:1) - I hope I'll have some free time to make a clean non-Seastar reproducer.

This problem prevents compilation of Scylla with the latest GCC, an example real-world error message:

[23/62] CXX build/dev/cdc/log.o
FAILED: build/dev/cdc/log.o 
g++ -MD -MT build/dev/cdc/log.o -MF build/dev/cdc/log.o.d -I/root/scylla/seastar/include -I/root/scylla/build/dev/seastar/gen/include -std=gnu++20 -U_FORTIFY_SOURCE -Wno-maybe-uninitialized -DSEASTAR_SSTRING -Werror=unused-result -fstack-clash-protection -DSEASTAR_API_LEVEL=6 -DSEASTAR_ENABLE_ALLOC_FAILURE_INJECTION -DSEASTAR_SCHEDULING_GROUPS_COUNT=16 -DSEASTAR_TYPE_ERASE_MORE -DFMT_LOCALE -DFMT_SHARED -I/usr/include/p11-kit-1  -DDEVEL -DSEASTAR_ENABLE_ALLOC_FAILURE_INJECTION -DSCYLLA_ENABLE_ERROR_INJECTION -O2 -Wstack-usage=21504 -Wno-error=stack-usage= -DSCYLLA_ENABLE_WASMTIME -iquote. -iquote build/dev/gen --std=gnu++20  -ffile-prefix-map=/root/scylla=.  -march=westmere -DBOOST_TEST_DYN_LINK   -Iabseil -fvisibility=hidden  -Wall -Werror -Wno-stringop-overread -Wno-mismatched-tags -Wno-maybe-uninitialized -Wno-tautological-compare -Wno-missing-braces -Wno-overflow -Wno-noexcept-type -Wno-nonnull-compare -Wno-error=cpp -Wno-ignored-attributes -Wno-overloaded-virtual -Wno-stringop-overflow -Wno-unused-variable -Wno-psabi -Wno-narrowing -Wno-array-bounds -Wno-nonnull -Wno-catch-value -Wno-error=deprecated-declarations -DXXH_PRIVATE_API -DSEASTAR_TESTING_MAIN -DHAVE_LZ4_COMPRESS_DEFAULT  -c -o build/dev/cdc/log.o cdc/log.cc
In file included from /root/scylla/seastar/include/seastar/util/backtrace.hh:32,
                 from /root/scylla/seastar/include/seastar/core/task.hh:26,
                 from /root/scylla/seastar/include/seastar/core/future.hh:24,
                 from /root/scylla/seastar/include/seastar/core/thread.hh:26,
                 from cdc/log.cc:14:
In destructor ‘seastar::lw_shared_ptr<T>::~lw_shared_ptr() [with T = compound_type<allow_prefixes::yes>]’,
    inlined from ‘compound_wrapper<clustering_key_prefix, clustering_key_prefix_view>::equality::~equality()’ at ./keys.hh:298:12,
    inlined from ‘cdc::transformer::transformer(cdc::db_context, schema_ptr, dht::decorated_key)’ at cdc/log.cc:1436:88:
/root/scylla/seastar/include/seastar/core/shared_ptr.hh:300:26: error: pointer used after ‘void operator delete(void*, std::size_t)’ [-Werror=use-after-free]
  300 |         if (_p && !--_p->_count) {
      |                      ~~~~^~~~~~
In static member function ‘static void seastar::internal::lw_shared_ptr_accessors_no_esft<T>::dispose(seastar::lw_shared_ptr_counter_base*) [with T = compound_type<allow_prefixes::yes>]’,
    inlined from ‘static void seastar::internal::lw_shared_ptr_accessors_no_esft<T>::dispose(seastar::lw_shared_ptr_counter_base*) [with T = compound_type<allow_prefixes::yes>]’ at /root/scylla/seastar/include/seastar/core/shared_ptr.hh:213:17,
    inlined from ‘seastar::lw_shared_ptr<T>::~lw_shared_ptr() [with T = compound_type<allow_prefixes::yes>]’ at /root/scylla/seastar/include/seastar/core/shared_ptr.hh:301:31,
    inlined from ‘compound_wrapper<clustering_key_prefix, clustering_key_prefix_view>::hashing::~hashing()’ at ./keys.hh:287:12,
    inlined from ‘cdc::transformer::transformer(cdc::db_context, schema_ptr, dht::decorated_key)’ at cdc/log.cc:1436:53:
/root/scylla/seastar/include/seastar/core/shared_ptr.hh:214:9: note: call to ‘void operator delete(void*, std::size_t)’ here
  214 |         delete static_cast<concrete_type*>(counter);
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1plus: all warnings being treated as errors

cc @avikivity

[avelanarius]

Just to update, the issue reduces to this:

struct counter_structure {
    int count;
};

counter_structure* __attribute__ ((noinline)) make_counter_structure() {
    // Works fine without "__attribute__ ((noinline))"
    auto ptr = new counter_structure;
    if (ptr) {
        ptr->count = 0;
    }
    return ptr;
}

class my_lw_shared_ptr {
    counter_structure* _p;

public:
    my_lw_shared_ptr() : _p(make_counter_structure()) {
        if (_p) {
            _p->count++;
            // Works fine with "_p->count = 1;"
        }
    }
    my_lw_shared_ptr(const my_lw_shared_ptr& c) : _p(c._p) {
        if (_p) {
            _p->count++;
        }
    }
    ~my_lw_shared_ptr() { 
        if (_p && --(_p->count) == 0) {
            delete _p;
        }
    }
};

void reproducer() {
    auto ptr = my_lw_shared_ptr{};
    auto ptr2 = ptr;
}

https://godbolt.org/z/GbezbqcYW

use-after-free warning was introduced in GCC 12, but it can't properly reason about _p->count in some cases (simulated by noinline), causing a bogus warning.

[michoecho]

You can reduce it to this, really:

int* make_int();
void reproducer() {
    auto p = make_int();
    if (*p == 0) {
        delete p;
    }
    ++*p;
}

If this is enough to cause false positives, then maybe you just have to deal with them, or disable the warning. I wonder what's the logic behind it though, because the above triggers the warning and the below doesn't:

int* make_int();
bool random_bool();
void reproducer() {
    auto p = make_int();
    if (random_bool()) {
        delete p;
    }
    ++*p;
}

[avelanarius]

You can reduce it to this, really:

int* make_int();
void reproducer() {
    auto p = make_int();
    if (*p == 0) {
        delete p;
    }
    ++*p;
}

To work around this false positive in that reproducer, you can use a secret technique called "Scylla is the best!":

#include <cstdio>
int* make_int();
void reproducer() {
    auto p = make_int();
    if (*p == 0) {
        delete p;
    }
    printf("Scylla is the best!"); // "Scylla is the best!" conquers -Wuse-after-free
    ++*p;
}

(https://godbolt.org/z/c3jzqzcPq)

[avelanarius]

I wonder what's the logic behind it though, because the above triggers the warning and the below doesn't:

Such modifications:

• Replace make_int() with new int
• Replace if (*p == 0) with if (true) or if (false) or remove if altogether

also don't trigger the warning.

[michoecho]

Such modifications:

Replace make_int() with new int
Replace if (*p == 0) with if (true) or if (false) or remove if altogether

also don't trigger the warning.

Those two cases are different and make sense, because they result in the compiler optimizing the use of *p away, possibly before the warning is checked. The case with printf also makes some sense, because an external function like printf could make the pointer valid again.

But in my second case you actually get this in the output:

        mov     rdi, rbx
        mov     esi, 4
        call    operator delete(void*, unsigned long)
        add     DWORD PTR [rbx], 1
        pop     rbx
        ret

and if that doesn't trigger the warning, then I don't know what does.

[avelanarius]

This warning seems to behave better if you use C free() instead of C++ delete ptr:

#include <cstdlib>

int* make_int();
void reproducer() {
    auto p = make_int();
    // delete p;
    free(p);
    ++*p;
}

(and in GCC they have only C tests of this warning as far as I checked, for example https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=671a283636de75f7ed638ee6b01ed2d44361b8b6)

[avikivity]

Please report to gcc and post the link here.

[avikivity]

Also, we can work around it with #pragma

[avelanarius]

Submitted bug report here: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105204

[avelanarius]

On that bug report, a GCC maintainer said that this is the expected behavior of that diagnostic. He suggested to use a workaround with __builtin_unreachable() allowing compiler to reason about refcount (not being 0).

However such workaround won't work for two reasons. One, empirically adding those __builtin_unreachable to lw_shared_ptr causes the warning to trigger for those added __builtin_unreachables:

error: pointer used after ‘void operator delete(void*, std::size_t)’ [-Werror=use-after-free]
  302 |         if (_p && _p->_count == 0) __builtin_unreachable();
      |                   ~~~~^~~~~~

Second, even in my simplified example, adding "distractions" can cause the compiler to "forget" about those added unreachables:

struct shared_ptr {
    size_t* ref_count;
    shared_ptr(const shared_ptr& other) : ref_count(other.ref_count) {
        if (*ref_count == 0) __builtin_unreachable();
        (*ref_count)++;
    }
    ~shared_ptr() {
        if (--(*ref_count) == 0) {
            free(ref_count);
        }
    }
};

int x(int y);
int example3(shared_ptr& sp) {
    shared_ptr sp2(sp);
    shared_ptr sp3(sp);

    // Distraction for -Wuse-after-free=1
    // int res = 0;
    // for (int i = 0; i < x(i); i++) {
    //     res += x(i + 3);
    // }
    // return res;

    // Without distraction, the warning does not trigger.
    // With distraction, the warning triggers even though the __builtin_unreachable().
    return 0;
}