[PATCH seastar 0/2] Fix ASAN errors in debug builds

[Tomasz Grabiec]

Since 1ef66deddfaec1b0654aedd62d484e6f8a186e2c seastar installs
alternative signal handler stack. ASAN also does that and expects
that its stack is still installed when it cleans up, it tries to
unmap current stack.

Fix by restoring previous stack when reactor::run() returns.

Fixes the following error:

==31667==ERROR: AddressSanitizer failed to deallocate 0x2000 (8192) bytes at address 0x625000039900
==31667==AddressSanitizer CHECK failed: ../../../../libsanitizer/sanitizer_common/sanitizer_posix.cc:133 "(("unable to unmap" && 0)) != (0)" (0x0, 0x0)

Tomasz Grabiec (2):
util: Make deferred_action object movable
core/reactor: Restore previous alternative signal stack

util/defer.hh | 4 ++++
core/reactor.cc | 41 +++++++++++++++++++++++++++--------------
2 files changed, 31 insertions(+), 14 deletions(-)

--
2.5.5

[Tomasz Grabiec]

---
util/defer.hh | 4 ++++
1 file changed, 4 insertions(+)

diff --git a/util/defer.hh b/util/defer.hh
index 5ded312..d00e105 100644
--- a/util/defer.hh
+++ b/util/defer.hh
@@ -28,6 +28,10 @@ class deferred_action {
bool _cancelled = false;
public:
deferred_action(Func&& func) : _func(std::move(func)) {}
+ deferred_action(deferred_action&&) = default;
+ deferred_action(const deferred_action&) = default;
+ deferred_action& operator=(deferred_action&&) = default;
+ deferred_action& operator=(const deferred_action&) = default;
~deferred_action() { if (!_cancelled) { _func(); }; }
void cancel() { _cancelled = true; }
};
--
2.5.5

[Tomasz Grabiec]

Since 1ef66deddfaec1b0654aedd62d484e6f8a186e2c seastar installs
alternative signal handler stack. ASAN also does that and expects
that its stack is still installed when it cleans up, it tries to
unmap current stack.

Fix by restoring previous stack when reactor::run() returns.

Fixes the following error:

==31667==ERROR: AddressSanitizer failed to deallocate 0x2000 (8192) bytes at address 0x625000039900
==31667==AddressSanitizer CHECK failed: ../../../../libsanitizer/sanitizer_common/sanitizer_posix.cc:133 "(("unable to unmap" && 0)) != (0)" (0x0, 0x0)

---
core/reactor.cc | 41 +++++++++++++++++++++++++++--------------
1 file changed, 27 insertions(+), 14 deletions(-)

diff --git a/core/reactor.cc b/core/reactor.cc
index ee6f59d..56cc7c3 100644
--- a/core/reactor.cc
+++ b/core/reactor.cc
@@ -87,6 +87,7 @@
#endif

#include <xmmintrin.h>
+#include "util/defer.hh"

using namespace std::chrono_literals;

@@ -230,6 +231,30 @@ inline int task_quota_signal() {
return SIGRTMIN + 1;
}

+// Installs signal handler stack for current thread.
+// The stack remains installed as long as the returned object is kept alive.
+// When it goes out of scope the previous handler is restored.
+static decltype(auto) install_signal_handler_stack() {
+ size_t size = SIGSTKSZ;
+ auto mem = std::make_unique<char[]>(size);
+ stack_t stack;
+ stack_t prev_stack;
+ stack.ss_sp = mem.get();
+ stack.ss_flags = 0;
+ stack.ss_size = size;
+ auto r = sigaltstack(&stack, &prev_stack);
+ throw_system_error_on(r == -1);
+ return defer([mem = std::move(mem), prev_stack] () mutable {
+ try {
+ auto r = sigaltstack(&prev_stack, NULL);
+ throw_system_error_on(r == -1);
+ } catch (...) {
+ mem.release(); // We failed to restore previous stack, must leak it.
+ seastar_logger.error("Failed to restore signal stack: {}", std::current_exception());
+ }
+ });
+}
+
reactor::reactor()
: _backend()
#ifdef HAVE_OSV
@@ -2228,6 +2253,8 @@ static void print_backtrace_safe() noexcept {
}

int reactor::run() {
+ auto signal_stack = install_signal_handler_stack();
+
auto collectd_metrics = register_collectd_metrics();

#ifndef HAVE_OSV
@@ -3065,18 +3092,6 @@ void install_oneshot_signal_handler() {
throw_system_error_on(r == -1);
}

-static void install_signal_handler_stack() {
- size_t size = SIGSTKSZ;
- auto mem = std::make_unique<char[]>(size);
- stack_t stack;
- stack.ss_sp = mem.get();
- stack.ss_flags = 0;
- stack.ss_size = size;
- auto r = sigaltstack(&stack, NULL);
- throw_system_error_on(r == -1);
- mem.release();
-}
-
static void print_with_backtrace(const char* cause) noexcept {
print_safe(cause);
if (local_engine) {
@@ -3114,7 +3129,6 @@ void smp::configure(boost::program_options::variables_map configuration)
}
pthread_sigmask(SIG_BLOCK, &sigs, nullptr);

- install_signal_handler_stack();
install_oneshot_signal_handler<SIGSEGV, sigsegv_action>();
install_oneshot_signal_handler<SIGABRT, sigabrt_action>();
install_atomic_signal_handler<SIGQUIT, sigquit_action>();
@@ -3267,7 +3281,6 @@ void smp::configure(boost::program_options::variables_map configuration)
smp::pin(allocation.cpu_id);
}
memory::configure(allocation.mem, hugepages_path);
- install_signal_handler_stack();
sigset_t mask;
sigfillset(&mask);
for (auto sig : { SIGSEGV, SIGQUIT }) {
--
2.5.5

[Tomasz Grabiec]

Please ignore, there will be v2.

[Tomasz Grabiec]

Since 1ef66deddfaec1b0654aedd62d484e6f8a186e2c seastar installs
alternative signal handler stack. ASAN also does that and expects
that its stack is still installed when it cleans up, it tries to
unmap current stack.

Fix by restoring previous stack when reactor::run() returns.

Fixes the following error:

==31667==ERROR: AddressSanitizer failed to deallocate 0x2000 (8192) bytes at address 0x625000039900
==31667==AddressSanitizer CHECK failed: ../../../../libsanitizer/sanitizer_common/sanitizer_posix.cc:133 "(("unable to unmap" && 0)) != (0)" (0x0, 0x0)

Changes in v2:
- Fix deferred_action move constructor

Tomasz Grabiec (3):

util: Make deferred_action object movable

tests: Add defer_test

core/reactor: Restore previous alternative signal stack

configure.py | 2 ++
test.py | 1 +
util/defer.hh | 13 +++++++++-
core/reactor.cc | 41 +++++++++++++++++++----------
tests/defer_test.cc | 75 +++++++++++++++++++++++++++++++++++++++++++++++++++++
5 files changed, 117 insertions(+), 15 deletions(-)
create mode 100644 tests/defer_test.cc

--
2.5.5

[Tomasz Grabiec]

Currently deferred_action is not movable. Making it movable is useful
- it will allow passing a non-copyable lambda. To achieve this we also
need to make deferred_action non-copyable, which I think is fine. It
seems dangerous to allow it to be copied since defer() is intended to
be used for resource reclamation and we want it to run only once.
---
util/defer.hh | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/util/defer.hh b/util/defer.hh
index 5ded312..1d42396 100644
--- a/util/defer.hh
+++ b/util/defer.hh
@@ -27,7 +27,18 @@ class deferred_action {
Func _func;

bool _cancelled = false;
public:

- deferred_action(Func&& func) : _func(std::move(func)) {}
+ static_assert(std::is_nothrow_move_constructible<Func>::value, "Func(Func&&) must be noexcept");
+ deferred_action(Func&& func) noexcept : _func(std::move(func)) {}
+ deferred_action(deferred_action&& o) noexcept : _func(std::move(o._func)), _cancelled(o._cancelled) {
+ o._cancelled = true;
+ }
+ deferred_action& operator=(deferred_action&& o) noexcept {
+ if (this != &o) {
+ this->~deferred_action();
+ new (this) deferred_action(std::move(o));
+ }
+ }
+ deferred_action(const deferred_action&) = delete;

[Tomasz Grabiec]

---

configure.py | 2 ++
test.py | 1 +

tests/defer_test.cc | 75 +++++++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 78 insertions(+)
create mode 100644 tests/defer_test.cc

diff --git a/configure.py b/configure.py
index 73c21be..e5f8917 100755
--- a/configure.py
+++ b/configure.py
@@ -183,6 +183,7 @@ tests = [
'tests/udp_client',
'tests/blkdiscard_test',
'tests/sstring_test',
+ 'tests/defer_test',
'tests/httpd',
'tests/memcached/test_ascii_parser',
'tests/tcp_sctp_server',
@@ -386,6 +387,7 @@ deps = {
'apps/iotune/iotune': ['apps/iotune/iotune.cc', 'apps/iotune/fsqual.cc'] + core,
'tests/blkdiscard_test': ['tests/blkdiscard_test.cc'] + core,
'tests/sstring_test': ['tests/sstring_test.cc'] + core,
+ 'tests/defer_test': ['tests/defer_test.cc'] + core,
'tests/httpd': ['tests/httpd.cc'] + http + core + boost_test_lib,
'tests/allocator_test': ['tests/allocator_test.cc'] + core,
'tests/output_stream_test': ['tests/output_stream_test.cc'] + core + libnet + boost_test_lib,
diff --git a/test.py b/test.py
index 467ce8f..b578185 100755
--- a/test.py
+++ b/test.py
@@ -29,6 +29,7 @@ boost_tests = [
'thread_test',
'memcached/test_ascii_parser',
'sstring_test',
+ 'defer_test',
'output_stream_test',
'httpd',
'fstream_test',
diff --git a/tests/defer_test.cc b/tests/defer_test.cc
new file mode 100644
index 0000000..a0d209e
--- /dev/null
+++ b/tests/defer_test.cc
@@ -0,0 +1,75 @@
+/*
+ * This file is open source software, licensed to you under the terms
+ * of the Apache License, Version 2.0 (the "License"). See the NOTICE file
+ * distributed with this work for additional information regarding copyright
+ * ownership. You may not use this file except in compliance with the License.
+ *
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+/*
+ * Copyright 2016 ScyllaDB
+ */
+
+#define BOOST_TEST_DYN_LINK
+#define BOOST_TEST_MODULE core
+
+#include <boost/test/included/unit_test.hpp>
+#include "util/defer.hh"
+
+BOOST_AUTO_TEST_CASE(test_defer_does_not_run_when_canceled) {
+ bool ran = false;
+ {
+ auto d = defer([&] {
+ ran = true;
+ });
+ d.cancel();
+ }
+ BOOST_REQUIRE(!ran);
+}
+
+BOOST_AUTO_TEST_CASE(test_defer_runs) {
+ bool ran = false;
+ {
+ auto d = defer([&] {
+ ran = true;
+ });
+ }
+ BOOST_REQUIRE(ran);
+}
+
+BOOST_AUTO_TEST_CASE(test_defer_runs_once_when_moved) {
+ int ran = 0;
+ {
+ auto d = defer([&] {
+ ++ran;
+ });
+ {
+ auto d2 = std::move(d);
+ }
+ BOOST_REQUIRE_EQUAL(1, ran);
+ }
+ BOOST_REQUIRE_EQUAL(1, ran);
+}
+
+BOOST_AUTO_TEST_CASE(test_defer_does_not_run_when_moved_after_cancelled) {
+ int ran = 0;
+ {
+ auto d = defer([&] {
+ ++ran;
+ });
+ d.cancel();
+ {
+ auto d2 = std::move(d);
+ }
+ }
+ BOOST_REQUIRE_EQUAL(0, ran);
+}
--
2.5.5

[Tomasz Grabiec]

Since 1ef66deddfaec1b0654aedd62d484e6f8a186e2c seastar installs
alternative signal handler stack. ASAN also does that and expects
that its stack is still installed when it cleans up, it tries to
unmap current stack.

Fix by restoring previous stack when reactor::run() returns.

Fixes the following error:

==31667==ERROR: AddressSanitizer failed to deallocate 0x2000 (8192) bytes at address 0x625000039900
==31667==AddressSanitizer CHECK failed: ../../../../libsanitizer/sanitizer_common/sanitizer_posix.cc:133 "(("unable to unmap" && 0)) != (0)" (0x0, 0x0)

---
core/reactor.cc | 41 +++++++++++++++++++++++++++--------------
1 file changed, 27 insertions(+), 14 deletions(-)

diff --git a/core/reactor.cc b/core/reactor.cc

index 623218d..585be7c 100644

[Gleb Natapov]

decltype(auto) is a bit of overkill here, you know that you return an
object by value.

> + size_t size = SIGSTKSZ;
> + auto mem = std::make_unique<char[]>(size);
> + stack_t stack;
> + stack_t prev_stack;
> + stack.ss_sp = mem.get();
> + stack.ss_flags = 0;
> + stack.ss_size = size;
> + auto r = sigaltstack(&stack, &prev_stack);
> + throw_system_error_on(r == -1);
> + return defer([mem = std::move(mem), prev_stack] () mutable {
> + try {
> + auto r = sigaltstack(&prev_stack, NULL);
> + throw_system_error_on(r == -1);
> + } catch (...) {

Why throw and immediately catch instead of using if()?

> --
> You received this message because you are subscribed to the Google Groups "seastar-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to seastar-dev...@googlegroups.com.
> To post to this group, send email to seast...@googlegroups.com.
> Visit this group at https://groups.google.com/group/seastar-dev.
> To view this discussion on the web visit https://groups.google.com/d/msgid/seastar-dev/1475067862-7818-4-git-send-email-tgrabiec%40scylladb.com.
> For more options, visit https://groups.google.com/d/optout.

--
Gleb.

[Tomasz Grabiec]

I not only want to detect the error, but also log it in a standard way. The same way it would be if I didn't handle that error here. 

 

> To unsubscribe from this group and stop receiving emails from it, send an email to seastar-dev+unsubscribe@googlegroups.com.

[Gleb Natapov]

What about:
seastar_logger.error("... {}", std::system_error(errno, std::system_category()).what())

OTOH this is not performance critical path, so exception throwing does
not really matter, just code looking strange.

> > an email to seastar-dev...@googlegroups.com.

> > > To post to this group, send email to seast...@googlegroups.com.
> > > Visit this group at https://groups.google.com/group/seastar-dev.
> > > To view this discussion on the web visit https://groups.google.com/d/
> > msgid/seastar-dev/1475067862-7818-4-git-send-email-tgrabiec%40scylladb.com
> > .
> > > For more options, visit https://groups.google.com/d/optout.
> >
> > --
> > Gleb.
> >

--
Gleb.

[Tomasz Grabiec]

To me that looks more complex than just printing std::current_exception() in the exception handler. This requires this throw_system_error_on(r == -1) earlier, but that semantically belongs to the C++'ified version of sigaltstack. There is the same pattern a few lines earlier (sigaltstack+throw_system_error_on). It could be extracted into a function. Then perhaps the code would no longer look that strange:

try {

    change_signal_stack(prev_stack);

} catch (...) {

[Gleb Natapov]

Yes, will look nicer.

>
>
> >
> > OTOH this is not performance critical path, so exception throwing does
> > not really matter, just code looking strange.
> >

--
Gleb.

[Avi Kivity]

On 09/28/2016 04:04 PM, Tomasz Grabiec wrote:
> Since 1ef66deddfaec1b0654aedd62d484e6f8a186e2c seastar installs
> alternative signal handler stack. ASAN also does that and expects
> that its stack is still installed when it cleans up, it tries to
> unmap current stack.
>
> Fix by restoring previous stack when reactor::run() returns.
>
> Fixes the following error:
>
> ==31667==ERROR: AddressSanitizer failed to deallocate 0x2000 (8192) bytes at address 0x625000039900
> ==31667==AddressSanitizer CHECK failed: ../../../../libsanitizer/sanitizer_common/sanitizer_posix.cc:133 "(("unable to unmap" && 0)) != (0)" (0x0, 0x0)
>

Nice patchset. git url?

[Tomasz Grabiec]

On Thu, Sep 29, 2016 at 11:18 AM, Avi Kivity <a...@scylladb.com> wrote:

On 09/28/2016 04:04 PM, Tomasz Grabiec wrote:

Since 1ef66deddfaec1b0654aedd62d484e6f8a186e2c seastar installs
alternative signal handler stack. ASAN also does that and expects
that its stack is still installed when it cleans up, it tries to
unmap current stack.

Fix by restoring previous stack when reactor::run() returns.

Fixes the following error:

==31667==ERROR: AddressSanitizer failed to deallocate 0x2000 (8192) bytes at address 0x625000039900
==31667==AddressSanitizer CHECK failed: ../../../../libsanitizer/sanitizer_common/sanitizer_posix.cc:133 "(("unable to unmap" && 0)) != (0)" (0x0, 0x0)

Nice patchset.  git url?

dev tgrabiec/restore-signal-stack

[Commit Bot]

From: Tomasz Grabiec <tgra...@scylladb.com>
Committer: Tomasz Grabiec <tgra...@scylladb.com>
Branch: master

util: Make deferred_action object movable

Currently deferred_action is not movable. Making it movable is useful
- it will allow passing a non-copyable lambda. To achieve this we also
need to make deferred_action non-copyable, which I think is fine. It
seems dangerous to allow it to be copied since defer() is intended to
be used for resource reclamation and we want it to run only once.

---

diff --git a/util/defer.hh b/util/defer.hh

[Commit Bot]

From: Tomasz Grabiec <tgra...@scylladb.com>
Committer: Tomasz Grabiec <tgra...@scylladb.com>
Branch: master

tests: Add defer_test

---
diff --git a/configure.py b/configure.py
--- a/configure.py
+++ b/configure.py
@@ -183,6 +183,7 @@ def sanitize_vptr_flag(compiler):

'tests/udp_client',
'tests/blkdiscard_test',
'tests/sstring_test',
+ 'tests/defer_test',
'tests/httpd',
'tests/memcached/test_ascii_parser',
'tests/tcp_sctp_server',

@@ -385,6 +386,7 @@ def have_xen():

'apps/iotune/iotune':
['apps/iotune/iotune.cc', 'apps/iotune/fsqual.cc'] + core,
'tests/blkdiscard_test': ['tests/blkdiscard_test.cc'] + core,
'tests/sstring_test': ['tests/sstring_test.cc'] + core,
+ 'tests/defer_test': ['tests/defer_test.cc'] + core,
'tests/httpd': ['tests/httpd.cc'] + http + core + boost_test_lib,
'tests/allocator_test': ['tests/allocator_test.cc'] + core,
'tests/output_stream_test': ['tests/output_stream_test.cc'] + core +
libnet + boost_test_lib,
diff --git a/test.py b/test.py

--- a/test.py
+++ b/test.py
@@ -29,6 +29,7 @@

'thread_test',
'memcached/test_ascii_parser',
'sstring_test',
+ 'defer_test',
'output_stream_test',
'httpd',
'fstream_test',
diff --git a/tests/defer_test.cc b/tests/defer_test.cc

--- a/tests/defer_test.cc

[Commit Bot]

From: Tomasz Grabiec <tgra...@scylladb.com>
Committer: Tomasz Grabiec <tgra...@scylladb.com>
Branch: master

core/reactor: Restore previous alternative signal stack

Since 1ef66deddfaec1b0654aedd62d484e6f8a186e2c seastar installs
alternative signal handler stack. ASAN also does that and expects
that its stack is still installed when it cleans up, it tries to
unmap current stack.

Fix by restoring previous stack when reactor::run() returns.

Fixes the following error:

==31667==ERROR: AddressSanitizer failed to deallocate 0x2000 (8192) bytes
at address 0x625000039900
==31667==AddressSanitizer CHECK
failed: ../../../../libsanitizer/sanitizer_common/sanitizer_posix.cc:133 "(("unable
to unmap" && 0)) != (0)" (0x0, 0x0)

---
diff --git a/core/reactor.cc b/core/reactor.cc

--- a/core/reactor.cc
+++ b/core/reactor.cc
@@ -87,6 +87,7 @@
#endif

#include <xmmintrin.h>
+#include "util/defer.hh"

using namespace std::chrono_literals;

@@ -230,6 +231,30 @@ inline int task_quota_signal() {
return SIGRTMIN + 1;
}

+// Installs signal handler stack for current thread.
+// The stack remains installed as long as the returned object is kept
alive.
+// When it goes out of scope the previous handler is restored.
+static decltype(auto) install_signal_handler_stack() {

+ size_t size = SIGSTKSZ;
+ auto mem = std::make_unique<char[]>(size);
+ stack_t stack;
+ stack_t prev_stack;
+ stack.ss_sp = mem.get();
+ stack.ss_flags = 0;
+ stack.ss_size = size;
+ auto r = sigaltstack(&stack, &prev_stack);
+ throw_system_error_on(r == -1);
+ return defer([mem = std::move(mem), prev_stack] () mutable {
+ try {
+ auto r = sigaltstack(&prev_stack, NULL);
+ throw_system_error_on(r == -1);
+ } catch (...) {

+ mem.release(); // We failed to restore previous stack, must
leak it.
+ seastar_logger.error("Failed to restore signal stack: {}",
std::current_exception());
+ }
+ });
+}
+
reactor::reactor()
: _backend()
#ifdef HAVE_OSV
@@ -2228,6 +2253,8 @@ static void print_backtrace_safe() noexcept {
}

int reactor::run() {
+ auto signal_stack = install_signal_handler_stack();
+
auto collectd_metrics = register_collectd_metrics();

#ifndef HAVE_OSV

@@ -3050,18 +3077,6 @@ void install_oneshot_signal_handler() {

throw_system_error_on(r == -1);
}

-static void install_signal_handler_stack() {
- size_t size = SIGSTKSZ;
- auto mem = std::make_unique<char[]>(size);
- stack_t stack;
- stack.ss_sp = mem.get();
- stack.ss_flags = 0;
- stack.ss_size = size;
- auto r = sigaltstack(&stack, NULL);
- throw_system_error_on(r == -1);
- mem.release();
-}
-
static void print_with_backtrace(const char* cause) noexcept {
print_safe(cause);
if (local_engine) {

@@ -3095,7 +3110,6 @@ void

smp::configure(boost::program_options::variables_map configuration)
}
pthread_sigmask(SIG_BLOCK, &sigs, nullptr);

- install_signal_handler_stack();
install_oneshot_signal_handler<SIGSEGV, sigsegv_action>();
install_oneshot_signal_handler<SIGABRT, sigabrt_action>();

@@ -3247,7 +3261,6 @@ void

smp::configure(boost::program_options::variables_map configuration)
smp::pin(allocation.cpu_id);
}
memory::configure(allocation.mem, hugepages_path);
- install_signal_handler_stack();
sigset_t mask;
sigfillset(&mask);

for (auto sig : { SIGSEGV }) {