[PATCH] D39244: [sanitizer] Random shuffling of chunks for the 32-bit Primary Allocator

[Aleksey Shlyapnikov via Phabricator via llvm-commits]

alekseyshl added inline comments.


================
Comment at: lib/sanitizer_common/sanitizer_allocator_primary32.h:273
+    char padding[kCacheLineSize - 2 * sizeof(uptr) -
                  sizeof(IntrusiveList<TransferBatch>)];
   };
----------------
cryptoad wrote:
> alekseyshl wrote:
> > How about using [kCacheLineSize - offsetof(SizeClassInfo, padding)] instead?
> I can't seem to make that general idea to work one way or another.
> The other possibility that some sanitizer parts use is just char padding[kCacheLinesize].
Ah, right, that should not work there, sorry. Why not make it explicit then:

kCacheLineSize -  sizeof(SpinMutex) - sizeof(IntrusiveList<TransferBatch> - sizeof(u32))

or 

  struct SizeClassInfoData {
      SpinMutex mutex;
      IntrusiveList<TransferBatch> free_list;
      u32 rand_state;
  }
  template <typename T, uptr N>
  struct Padded : public T {
  private:
    char padding[N - sizeof(T)];
  };
  typedef Padded<SizeClassInfoData, kCacheLineSize> SizeClassInfo;

but yeah, it's too much for the cause :)


================
Comment at: lib/sanitizer_common/sanitizer_allocator_primary32.h:341
+    const uptr kShuffleArraySize = 48;
+    uptr shuffle_array[kShuffleArraySize];
+    uptr j = 0;
----------------
cryptoad wrote:
> cryptoad wrote:
> > alekseyshl wrote:
> > > How about using the newly allocated region for the array? You can fill and shuffle all the pointers at once, create the batches and then zero reg out. Still two passes, but less stack and chunks will be farther apart. The code will be simpler too and I have a feeling that we can even avoid double copying for !kShuffleArraySize.
> > This is tricky because if kRandomShuffleChunks is not used with kUseSeparateSizeClassForBatch , then the batches can also use the region itself.
> > Meaning while we iterate through the chunks, we could create a batch that would overlap and corrupt the array we are iterating on.
> > We can't really assume any part of the region is "safe" to store the array either due to the randomness, so using the end of the region or the beginning doesn't change that.
> > If kUseSeparateSizeClassForBatch  is used, then we are golden and that can work.
> > Currently Scudo uses both, but I feel they should be treated distinctly nonetheless.
> Rectification: even with kUseSeparateSizeClassforBatch, there is the special case of kBatchClassID for which the batches are stored in the same region.
Doesn't this latter case create a loop? We're in PopulateFreeList because AllocateBatch figured that sci->free_list.empty(), and when PopulateFreeList calls CreateBatch, don't we get back to AllocateBatch with free list still empty?

Sorry for too many questions, just want to understand what's going on here.


https://reviews.llvm.org/D39244