From eb7d17e5c5f73c98bf1a8a1ae8d4465968aaa44e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Pawe=C5=82=20Dziepak?= <pdziepak@scylladb.com>
Date: Wed, 10 Jul 2019 18:19:24 +0100
Subject: [PATCH] lsa: make sure align_up_for_asan() doesn't cause reads past
 end of segment

In debug mode the LSA needs objects to be 8-byte aligned in order to
maximise coverage from the AddressSanitizer.

Usually `close_active()` creates a dummy objects that covers the end of
the segment being closed. However, it the last real objects ends in the
last eight bytes of the segment then that dummy won't be created because
of the alignment requirements. This broke exit conditions on loops
trying to read all objects in the segment and caused them to attempt to
dereference address at the end of the segment. This patch fixes that.

Fixes #4653.
---
 utils/logalloc.cc | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

diff --git a/utils/logalloc.cc b/utils/logalloc.cc
index 51c61b1524..e4b578479c 100644
--- a/utils/logalloc.cc
+++ b/utils/logalloc.cc
@@ -60,8 +60,8 @@ namespace debug {
 constexpr size_t logalloc_alignment = 8;
 }
 template<typename T>
-static void align_up_for_asan(T& val) {
-    val = align_up(val, size_t(8));
+[[nodiscard]] static T align_up_for_asan(T val) {
+    return align_up(val, size_t(8));
 }
 template<typename T>
 void poison(const T* addr, size_t size) {
@@ -74,8 +74,7 @@ void poison(const T* addr, size_t size) {
     // * end of segment
     // In all cases, we can align up the size to guarantee that asan
     // is able to poison this.
-    align_up_for_asan(size);
-    ASAN_POISON_MEMORY_REGION(addr, size);
+    ASAN_POISON_MEMORY_REGION(addr, align_up_for_asan(size));
 }
 void unpoison(const char *addr, size_t size) {
     ASAN_UNPOISON_MEMORY_REGION(addr, size);
@@ -85,7 +84,7 @@ namespace debug {
 constexpr size_t logalloc_alignment = 1;
 }
 template<typename T>
-static void align_up_for_asan(T& val) { }
+[[nodiscard]] static T align_up_for_asan(T val) { return val; }
 template<typename T>
 void poison(const T* addr, size_t size) { }
 void unpoison(const char *addr, size_t size) { }
@@ -1171,8 +1170,7 @@ private:
         auto desc = object_descriptor(migrator);
         auto desc_encoded_size = desc.encoded_size();
 
-        size_t obj_offset = align_up(_active_offset + desc_encoded_size, alignment);
-        align_up_for_asan(obj_offset);
+        size_t obj_offset = align_up_for_asan(align_up(_active_offset + desc_encoded_size, alignment));
         if (obj_offset + size > segment::size) {
             close_and_open();
             return alloc_small(migrator, size, alignment);
@@ -1186,7 +1184,7 @@ private:
         _active_offset = obj_offset + size;
 
         // Align the end of the value so that the next descriptor is aligned
-        align_up_for_asan(_active_offset);
+        _active_offset = align_up_for_asan(_active_offset);
         _active->record_alloc(_active_offset - old_active_offset);
         return pos;
     }
@@ -1197,9 +1195,8 @@ private:
 
         static_assert(std::is_same<void, std::result_of_t<Func(const object_descriptor*, void*)>>::value, "bad Func signature");
 
-        auto pos = seg->at<const char>(0);
+        auto pos = align_up_for_asan(seg->at<const char>(0));
         while (pos < seg->at<const char>(segment::size)) {
-            align_up_for_asan(pos);
             auto old_pos = pos;
             const auto desc = object_descriptor::decode_forwards(pos);
             if (desc.is_live()) {
@@ -1209,6 +1206,7 @@ private:
             } else {
                 pos = old_pos + desc.dead_size();
             }
+            pos = align_up_for_asan(pos);
         }
     }
 
@@ -1448,8 +1446,7 @@ public:
         auto pos = reinterpret_cast<const char*>(obj);
         auto old_pos = pos;
         auto desc = object_descriptor::decode_backwards(pos);
-        auto dead_size = size + (old_pos - pos);
-        align_up_for_asan(dead_size);
+        auto dead_size = align_up_for_asan(size + (old_pos - pos));
         desc = object_descriptor::make_dead(dead_size);
         auto npos = const_cast<char*>(pos);
         desc.encode(npos);
@@ -1570,7 +1567,6 @@ public:
 
         size_t offset = 0;
         while (offset < segment_size) {
-            align_up_for_asan(offset);
             auto pos = src->at<const char>(offset);
             auto dpos = dst->at<char>(offset);
             auto old_pos = pos;
@@ -1589,6 +1585,7 @@ public:
             } else {
                 offset += desc.dead_size();
             }
+            offset = align_up_for_asan(offset);
         }
         shard_segment_pool.on_segment_migration();
     }