[sanitizer] Do not use the alignment-rounded-up size when using the secondary

[cryptoad]

Summary

The combined allocator rounds up the requested size with regard to the
alignment, which makes sense when being serviced by the primary as it comes
with alignment guarantees, but not with the secondary. For the rare case of
large alignments, it wastes memory, and entices unnecessarily large fields for
the Scudo header. With this patch, we pass the non-alignement-rounded-up size
to the secondary, and adapt the Scudo code for this change.

[cryptoad]

Cleaner patch for the combined allocator.

[alekseyshl]

lib/sanitizer_common/sanitizer_allocator_combined.h

62 ↗

(On Diff #80326)

Is it safe to run this CHECK now, when secondary is using non-adjusted allocation size?

64 ↗

(On Diff #80326)

Same here, it tries to zero out the result up to the rounded up size, not the original size.

[cryptoad]

lib/sanitizer_common/sanitizer_allocator_combined.h

62 ↗

(On Diff #80326)

sanitizer_allocator_secondary.h adds alignment to the size prior to allocation and align the result properly.
It also does a similar CHECK there.
Same with the Scudo allocator, but the job is split between the frontend and the secondary.
So the CHECK is still safe.

64 ↗

(On Diff #80326)

If the allocation comes from the secondary, it's mmap backed and it's zero'd out, and we don't need to clear the memory again.
We need to zero out only when it comes from the primary, which uses the rounded size rather than the original size for its allocation.

[alekseyshl]

lib/sanitizer_common/sanitizer_allocator_combined.h

62 ↗

(On Diff #80326)

Understood, but this check feels a bit arbitrary now (to me, that is). Before your change it matched well with the similar if statement adjusting 'size', now it is puzzling why, while both allocators accept the alignment as a parameter, the resulting pointer is expected to be aligned only when alignment is greater than 8?
I bet there's a reason, but it's not clear from the code, which just become more obscure.

64 ↗

(On Diff #80326)

Ah, right, there's '&& from_primary' check... Then it seems to make sense to push 'cleared' flag down to allocators, since the particular allocator knows better the current state of the memory block. Anyway, not for this patch.

[cryptoad]

lib/sanitizer_common/sanitizer_allocator_combined.h

62 ↗

(On Diff #80326)

I agree it's a bit obscure, all the more since the primary doesn't take the alignment as parameter, but is expected to produced a 2^x aligned chunked when requested 2^x bytes (https://github.com/llvm-mirror/compiler-rt/blob/master/lib/sanitizer_common/sanitizer_allocator_combined.h#L20).
It sort of messes up the logic as the secondary is being passed the alignment as a parameter, and it actually matters for the secondary.
I am opened to suggestion as to how it would make it clearer for you.

[alekseyshl]

LGTM

lib/sanitizer_common/sanitizer_allocator_combined.h

62 ↗

(On Diff #80326)

Well, in the context of this patch, not much. Maybe a comment why if this is enforced only when alignment > 8?

And primary allocator does care about alignment, although indirectly, in CanAllocate call.

[cryptoad]

Addressing Aleksey's comments.

[cryptoad]

Added comments to the combined allocator Allocate to make clearer the expected behavior.

[kcc]

LGTM

[cryptoad]

Wording changes in the comments.

compiler-rt/trunk/lib/sanitizer_common/sanitizer_allocator_combined.h

public:
 
   void *Allocate(AllocatorCache *cache, uptr size, uptr alignment,
                  bool cleared = false, bool check_rss_limit = false) {
     // Returning 0 on malloc(0) may break a lot of code.
     if (size == 0)
       size = 1;
     if (size + alignment < size) return ReturnNullOrDieOnBadRequest();
     if (check_rss_limit && RssLimitIsExceeded()) return ReturnNullOrDieOnOOM();
+    uptr original_size = size;
+    // If alignment requirements are to be fulfilled by the frontend allocator
+    // rather than by the primary or secondary, passing an alignment lower than
+    // or equal to 8 will prevent any further rounding up, as well as the later
+    // alignment check.
     if (alignment > 8)
       size = RoundUpTo(size, alignment);
     void *res;
     bool from_primary = primary_.CanAllocate(size, alignment);
+    // The primary allocator should return a 2^x aligned allocation when
+    // requested 2^x bytes, hence using the rounded up 'size' when being
+    // serviced by the primary. The secondary takes care of the alignment
+    // without such requirement, and allocating 'size' would use extraneous
+    // memory, so we employ 'original_size'.
     if (from_primary)
       res = cache->Allocate(&primary_, primary_.ClassID(size));
     else
-      res = secondary_.Allocate(&stats_, size, alignment);
+      res = secondary_.Allocate(&stats_, original_size, alignment);
     if (alignment > 8)
       CHECK_EQ(reinterpret_cast<uptr>(res) & (alignment - 1), 0);
+    // When serviced by the secondary, the chunk comes from a mmap allocation
+    // and will be zero'd out anyway. We only need to clear our the chunk if
+    // it was serviced by the primary, hence using the rounded up 'size'.
     if (cleared && res && from_primary)
       internal_bzero_aligned16(res, RoundUpTo(size, 16));
     return res;
   }
 
   bool MayReturnNull() const {
     return atomic_load(&may_return_null_, memory_order_acquire);
   }

compiler-rt/trunk/lib/scudo/scudo_allocator_secondary.h

void *Allocate(AllocatorStats *Stats, uptr Size, uptr Alignment) {
     if (MapBeg == ~static_cast<uptr>(0))
       return ReturnNullOrDieOnOOM();
     // A page-aligned pointer is assumed after that, so check it now.
     CHECK(IsAligned(MapBeg, PageSize));
     uptr MapEnd = MapBeg + MapSize;
     uptr UserBeg = MapBeg + PageSize + HeadersSize;
     // In the event of larger alignments, we will attempt to fit the mmap area
     // better and unmap extraneous memory. This will also ensure that the
-    // offset field of the header stays small (it will always be 0).
+    // offset and unused bytes field of the header stay small.
     if (Alignment > MinAlignment) {
       if (UserBeg & (Alignment - 1))
         UserBeg += Alignment - (UserBeg & (Alignment - 1));
       CHECK_GE(UserBeg, MapBeg);
       uptr NewMapBeg = UserBeg - HeadersSize;
       NewMapBeg = RoundDownTo(NewMapBeg, PageSize) - PageSize;
       CHECK_GE(NewMapBeg, MapBeg);
-      uptr NewMapSize = RoundUpTo(MapSize - Alignment, PageSize);
-      uptr NewMapEnd = NewMapBeg + NewMapSize;
+      uptr NewMapEnd =
+          RoundUpTo(UserBeg + Size - Alignment - AlignedChunkHeaderSize,
+                    PageSize) + PageSize;
       CHECK_LE(NewMapEnd, MapEnd);
       // Unmap the extra memory if it's large enough.
       uptr Diff = NewMapBeg - MapBeg;
       if (Diff > PageSize)
         UnmapOrDie(reinterpret_cast<void *>(MapBeg), Diff);
       Diff = MapEnd - NewMapEnd;
       if (Diff > PageSize)
         UnmapOrDie(reinterpret_cast<void *>(NewMapEnd), Diff);
       MapBeg = NewMapBeg;
-      MapSize = NewMapSize;
       MapEnd = NewMapEnd;
+      MapSize = NewMapEnd - NewMapBeg;
     }
     uptr UserEnd = UserBeg - AlignedChunkHeaderSize + Size;
     // For larger alignments, Alignment was added by the frontend to Size.
     if (Alignment > MinAlignment)
       UserEnd -= Alignment;
     CHECK_LE(UserEnd, MapEnd - PageSize);
     CHECK_EQ(MapBeg + PageSize, reinterpret_cast<uptr>(
         MmapFixedOrDie(MapBeg + PageSize, MapSize - 2 * PageSize)));

compiler-rt/trunk/lib/scudo/scudo_utils.cpp

 //===----------------------------------------------------------------------===//
 
 #include "scudo_utils.h"
 
 #include <errno.h>
 #include <fcntl.h>
 #include <stdarg.h>
 #include <unistd.h>
-#include <cpuid.h>
+#if defined(__x86_64__) || defined(__i386__)
+# include <cpuid.h>
+#endif
 
 #include <cstring>
 
 // TODO(kostyak): remove __sanitizer *Printf uses in favor for our own less
 //                complicated string formatting code. The following is a
 //                temporary workaround to be able to use __sanitizer::VSNPrintf.
 namespace __sanitizer {