Skip to content

Commit 3c63318

Browse files
crackcommavikivity
authored andcommitted
treewide: avoid 'static sstring' in favor of 'constexpr string_view'
I had problems with fuzzer+address sanitizer `-fsanitize=fuzzer,address`. It worked fine with `SEASTAR_SSTRING` undefined. Enabling `SEASTAR_ASAN_ENABLED` didn't help. What I gathered is that it was static sstring destructors being called at exit. IIUC, there is no reason to use `sstring` for static constants, except to avoid copies like in `tls.cc`. Luckily, it doesn't seem like the `_blobs` map stores any keys that are external to `tls.cc` itself, which means it should be safe to use `std::string_view` instead. * Replaced `extern sstring magic_key_suffix` in the websocket header with `constexpr std::string_view` in the `.cc` file. * Replaced `static const sstring` in `tls.cc` with `constexpr std::string_view`. * Replaced the `sstring` key in the `credentials_builder::_blobs` map with `std::string_view`. The Seastar memory allocator + ASAN is quite a complex interaction I can't entirely grasp. The issue might be deeper and the solution should be different, but this patch seems good to me anyway. Fixes the following issue: ``` munmap_chunk(): invalid pointer ==350803== ERROR: libFuzzer: deadly signal #0 0x560609c161aa in __sanitizer_print_stack_trace (iobuf_fuzz_test+0x1df1aa) #1 0x560609b0fc50 in fuzzer::PrintStackTrace() (iobuf_fuzz_test+0xd8c50) scylladb#2 0x560609aea8b6 in fuzzer::Fuzzer::CrashCallback() (.part.0) (iobuf_fuzz_test+0xb38b6) scylladb#3 0x560609aea97a in fuzzer::Fuzzer::StaticCrashSignalCallback() (iobuf_fuzz_test+0xb397a) scylladb#4 0x7f96d4c4146f (/lib/libc.so.6+0x4146f) scylladb#5 0x7f96d4c99cdb in __pthread_kill_implementation (/lib/libc.so.6+0x99cdb) scylladb#6 0x7f96d4c413c5 in gsignal (/lib/libc.so.6+0x413c5) scylladb#7 0x7f96d4c28939 in abort (/lib/libc.so.6+0x28939) scylladb#8 0x7f96d4c299a2 in __libc_message_impl.cold (/lib/libc.so.6+0x299a2) scylladb#9 0x7f96d4ca44e6 in malloc_printerr (/lib/libc.so.6+0xa44e6) scylladb#10 0x7f96d4ca471b in munmap_chunk (/lib/libc.so.6+0xa471b) scylladb#11 0x7f96d4ca91df in cfree@GLIBC_2.2.5 (/lib/libc.so.6+0xa91df) scylladb#12 0x7f96d5d5cba7 in seastar::memory::cpu_pages::do_foreign_free(void*) (libseastar.so+0x35cba7) scylladb#13 0x7f96d5d5a901 in seastar::basic_sstring<char, unsigned int, 15u, true>::~basic_sstring() (libseastar.so+0x35a901) scylladb#14 0x7f96d4c436a8 in __cxa_finalize (/lib/libc.so.6+0x436a8) scylladb#15 0x7f96d5cf2516 in __do_global_dtors_aux (libseastar.so+0x2f2516) ``` ``` (lldb) rbreak seastar::basic_sstring<.*>::~basic_sstring (lldb) r [...] * thread #1, name = 'iobuf_fuzz_test', stop reason = breakpoint 1.1 frame #0: 0x00007ffff775a870 libseastar.so`seastar::basic_sstring<char, unsigned int, 15u, true>::~basic_sstring() libseastar.so`seastar::basic_sstring<char, unsigned int, 15u, true>::~basic_sstring: -> 0x7ffff775a870 <+0>: pushq %rbp 0x7ffff775a871 <+1>: movq %rsp, %rbp 0x7ffff775a874 <+4>: pushq %r14 0x7ffff775a876 <+6>: pushq %rbx (lldb) bt * thread #1, name = 'iobuf_fuzz_test', stop reason = breakpoint 1.1 * frame #0: 0x00007ffff775a870 libseastar.so`seastar::basic_sstring<char, unsigned int, 15u, true>::~basic_sstring() frame #1: 0x00007ffff68436a9 libc.so.6`__cxa_finalize + 361 frame scylladb#2: 0x00007ffff76f2517 libseastar.so`__do_global_dtors_aux + 39 frame scylladb#3: 0x00007ffff7fc60f2 ld-linux-x86-64.so.2`_dl_call_fini + 82 frame scylladb#4: 0x00007ffff7fc946e ld-linux-x86-64.so.2`_dl_fini + 494 frame scylladb#5: 0x00007ffff6843bf1 libc.so.6`__run_exit_handlers + 433 frame scylladb#6: 0x00007ffff6843cb0 libc.so.6`exit + 32 frame scylladb#7: 0x0000555555598567 iobuf_fuzz_test`fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) + 12311 frame scylladb#8: 0x00005555555d79f3 iobuf_fuzz_test`main + 35 frame scylladb#9: 0x00007ffff682a47e libc.so.6`__libc_start_call_main + 126 frame scylladb#10: 0x00007ffff682a539 libc.so.6`__libc_start_main@@GLIBC_2.34 + 137 frame scylladb#11: 0x0000555555581e25 iobuf_fuzz_test`_start + 37 (lldb) register read rdi rdi = 0x00007ffff7a57558 libseastar.so`seastar::experimental::websocket::magic_key_suffix ``` Closes scylladb#3159
1 parent 913fcf1 commit 3c63318

File tree

5 files changed

+26
-24
lines changed

5 files changed

+26
-24
lines changed

include/seastar/net/tls.hh

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -350,7 +350,7 @@ namespace tls {
350350
private:
351351
friend class reloadable_credentials_base;
352352

353-
std::multimap<sstring, std::any> _blobs;
353+
std::multimap<std::string_view, std::any> _blobs;
354354
client_auth _client_auth = client_auth::NONE;
355355
session_resume_mode _session_resume_mode = session_resume_mode::NONE;
356356
sstring _priority;

include/seastar/websocket/common.hh

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -30,8 +30,6 @@
3030

3131
namespace seastar::experimental::websocket {
3232

33-
extern sstring magic_key_suffix;
34-
3533
using handler_t = std::function<future<>(input_stream<char>&, output_stream<char>&)>;
3634

3735
class server;

src/net/tls.cc

Lines changed: 18 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,8 @@ module seastar;
6969

7070
namespace seastar {
7171

72+
using namespace std::string_view_literals;
73+
7274
class net::get_impl {
7375
public:
7476
static std::unique_ptr<connected_socket_impl> get(connected_socket s) {
@@ -627,12 +629,12 @@ void tls::server_credentials::set_alpn_protocols(const std::vector<sstring>& pro
627629
_impl->set_alpn_protocols(protocols);
628630
}
629631

630-
static const sstring dh_level_key = "dh_level";
631-
static const sstring x509_trust_key = "x509_trust";
632-
static const sstring x509_crl_key = "x509_crl";
633-
static const sstring x509_key_key = "x509_key";
634-
static const sstring pkcs12_key = "pkcs12";
635-
static const sstring system_trust = "system_trust";
632+
constexpr auto dh_level_key = "dh_level"sv;
633+
constexpr auto x509_trust_key = "x509_trust"sv;
634+
constexpr auto x509_crl_key = "x509_crl"sv;
635+
constexpr auto x509_key_key = "x509_key"sv;
636+
constexpr auto pkcs12_key = "pkcs12"sv;
637+
constexpr auto system_trust = "system_trust"sv;
636638

637639
using buffer_type = std::basic_string<tls::blob::value_type, tls::blob::traits_type, std::allocator<tls::blob::value_type>>;
638640

@@ -744,7 +746,7 @@ void tls::credentials_builder::set_alpn_protocols(const std::vector<sstring>& pr
744746

745747
template<typename Blobs, typename Visitor>
746748
static void visit_blobs(Blobs& blobs, Visitor&& visitor) {
747-
auto visit = [&](const sstring& key, auto* vt) {
749+
auto visit = [&](const std::string_view& key, auto* vt) {
748750
auto tr = blobs.equal_range(key);
749751
for (auto& p : boost::make_iterator_range(tr.first, tr.second)) {
750752
auto* v = std::any_cast<std::decay_t<decltype(*vt)>>(&p.second);
@@ -760,17 +762,17 @@ static void visit_blobs(Blobs& blobs, Visitor&& visitor) {
760762
void tls::credentials_builder::apply_to(certificate_credentials& creds) const {
761763
// Could potentially be templated down, but why bother...
762764
visit_blobs(_blobs, make_visitor(
763-
[&](const sstring& key, const x509_simple& info) {
765+
[&](const std::string_view& key, const x509_simple& info) {
764766
if (key == x509_trust_key) {
765767
creds.set_x509_trust(info.data, info.format);
766768
} else if (key == x509_crl_key) {
767769
creds.set_x509_crl(info.data, info.format);
768770
}
769771
},
770-
[&](const sstring&, const x509_key& info) {
772+
[&](const std::string_view&, const x509_key& info) {
771773
creds.set_x509_key(info.cert, info.key, info.format);
772774
},
773-
[&](const sstring&, const pkcs12_simple& info) {
775+
[&](const std::string_view&, const pkcs12_simple& info) {
774776
creds.set_simple_pkcs12(info.data, info.format, info.password);
775777
}
776778
));
@@ -842,14 +844,14 @@ class tls::reloadable_credentials_base {
842844
future<> init() {
843845
std::vector<future<>> futures;
844846
visit_blobs(_blobs, make_visitor(
845-
[&](const sstring&, const x509_simple& info) {
847+
[&](const std::string_view&, const x509_simple& info) {
846848
_all_files.emplace(info.file.filename);
847849
},
848-
[&](const sstring&, const x509_key& info) {
850+
[&](const std::string_view&, const x509_key& info) {
849851
_all_files.emplace(info.cert_file.filename);
850852
_all_files.emplace(info.key_file.filename);
851853
},
852-
[&](const sstring&, const pkcs12_simple& info) {
854+
[&](const std::string_view&, const pkcs12_simple& info) {
853855
_all_files.emplace(info.file.filename);
854856
}
855857
));
@@ -942,14 +944,14 @@ class tls::reloadable_credentials_base {
942944
++num_changed;
943945
};
944946
visit_blobs(_blobs, make_visitor(
945-
[&](const sstring&, x509_simple& info) {
947+
[&](const std::string_view&, x509_simple& info) {
946948
maybe_reload(info.file.filename, info.data);
947949
},
948-
[&](const sstring&, x509_key& info) {
950+
[&](const std::string_view&, x509_key& info) {
949951
maybe_reload(info.cert_file.filename, info.cert);
950952
maybe_reload(info.key_file.filename, info.key);
951953
},
952-
[&](const sstring&, pkcs12_simple& info) {
954+
[&](const std::string_view&, pkcs12_simple& info) {
953955
maybe_reload(info.file.filename, info.data);
954956
}
955957
));

src/websocket/common.cc

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,6 @@
2929

3030
namespace seastar::experimental::websocket {
3131

32-
sstring magic_key_suffix = "258EAFA5-E914-47DA-95CA-C5AB0DC85B11";
3332
logger websocket_logger("websocket");
3433

3534
future<> connection::handle_ping() {

src/websocket/server.cc

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -28,12 +28,15 @@
2828

2929
namespace seastar::experimental::websocket {
3030

31-
static sstring http_upgrade_reply_template =
31+
using namespace std::string_view_literals;
32+
33+
constexpr auto magic_key_suffix = "258EAFA5-E914-47DA-95CA-C5AB0DC85B11"sv;
34+
constexpr auto http_upgrade_reply_template =
3235
"HTTP/1.1 101 Switching Protocols\r\n"
3336
"Upgrade: websocket\r\n"
3437
"Connection: Upgrade\r\n"
3538
"Sec-WebSocket-Version: 13\r\n"
36-
"Sec-WebSocket-Accept: ";
39+
"Sec-WebSocket-Accept: "sv;
3740

3841
void server::listen(socket_address addr, listen_options lo) {
3942
_listeners.push_back(seastar::listen(addr, lo));
@@ -135,14 +138,14 @@ future<> server_connection::read_http_upgrade_request() {
135138
sstring sec_key = req->get_header("Sec-Websocket-Key");
136139
sstring sec_version = req->get_header("Sec-Websocket-Version");
137140

138-
sstring sha1_input = sec_key + magic_key_suffix;
141+
auto sha1_input = fmt::format("{}{}", sec_key, magic_key_suffix);
139142

140143
websocket_logger.debug("Sec-Websocket-Key: {}, Sec-Websocket-Version: {}", sec_key, sec_version);
141144

142145
std::string sha1_output = sha1_base64(sha1_input);
143146
websocket_logger.debug("SHA1 output: {} of size {}", sha1_output, sha1_output.size());
144147

145-
co_await _write_buf.write(http_upgrade_reply_template);
148+
co_await _write_buf.write(http_upgrade_reply_template.data(), http_upgrade_reply_template.size());
146149
co_await _write_buf.write(sha1_output);
147150
if (!_subprotocol.empty()) {
148151
co_await _write_buf.write("\r\nSec-WebSocket-Protocol: ", 26);

0 commit comments

Comments
 (0)