[scudo] Untag BlockEnd in reallocate

[vitalybuka]

Summary

If we get here from reallocate, BlockEnd is tagged. Then we
will storeTag(UntaggedEnd) into the header of the next chunk.

Luckily header tag is 0 so unpatched code still works.

[pcc]

Can we untag BlockEnd in reallocate instead?

[vitalybuka]

In D105261#2853702, @pcc wrote:

Can we untag BlockEnd in reallocate instead?

Yes, but it looks a little bit inconsistent when untag End here

[vitalybuka]

In D105261#2853736, @vitalybuka wrote:

In D105261#2853702, @pcc wrote:

Can we untag BlockEnd in reallocate instead?

Yes, but it looks a little bit inconsistent when untag End here

I can change to make both arguments of storeEndMarker must be untagged

[pcc]

In D105261#2853749, @vitalybuka wrote:

In D105261#2853736, @vitalybuka wrote:

In D105261#2853702, @pcc wrote:

Can we untag BlockEnd in reallocate instead?

Yes, but it looks a little bit inconsistent when untag End here

That's because they are different pointers. End refers to chunks (tagged) and BlockEnd refers to blocks (untagged).

I can change to make both arguments of storeEndMarker must be untagged

That's not going to work, we may need to store the original tag in this function.

[vitalybuka]

In D105261#2853767, @pcc wrote:

In D105261#2853749, @vitalybuka wrote:

In D105261#2853736, @vitalybuka wrote:

In D105261#2853702, @pcc wrote:

Can we untag BlockEnd in reallocate instead?

Yes, but it looks a little bit inconsistent when untag End here

That's because they are different pointers. End refers to chunks (tagged) and BlockEnd refers to blocks (untagged).

Sure, if it does not bother you, I'll update this way.

I can change to make both arguments of storeEndMarker must be untagged

That's not going to work, we may need to store the original tag in this function.

Oh, I see.

[vitalybuka]

untag in reallocate

[pcc]

compiler-rt/lib/scudo/standalone/combined.h

1156–1157

No need to rename, we don't have both kinds of pointers in this function so no need for a long name to distinguish. The DCHECK you added below is documentation enough.

[vitalybuka]

no rename

[pcc]

LGTM

compiler-rt/lib/scudo/standalone/combined.h

if (reinterpret_cast<uptr>(OldTaggedPtr) + NewSize <= BlockEnd) {
                      : BlockEnd -
                            (reinterpret_cast<uptr>(OldTaggedPtr) + NewSize)) &
             Chunk::SizeOrUnusedBytesMask;
         Chunk::compareExchangeHeader(Cookie, OldPtr, &NewHeader, &OldHeader);
         if (UNLIKELY(useMemoryTagging<Params>(Options))) {
           if (ClassId) {
             resizeTaggedChunk(reinterpret_cast<uptr>(OldTaggedPtr) + OldSize,
                               reinterpret_cast<uptr>(OldTaggedPtr) + NewSize,
-                              NewSize, BlockEnd);
+                              NewSize, untagPointer(BlockEnd));
             storePrimaryAllocationStackMaybe(Options, OldPtr);
           } else {
             storeSecondaryAllocationStackMaybe(Options, OldPtr, NewSize);
           }
         }
         return OldTaggedPtr;
       }
     }
#endif
   // block and our block is at the end of a mapping. The tag of the next block's
   // header granule will be set to 0, so it will serve the purpose of catching
   // linear overflows in this case.
   //
   // For allocations of size 0 we do not end up storing the address tag to the
   // memory tag space, which getInlineErrorInfo() normally relies on to match
   // address tags against chunks. To allow matching in this case we store the
   // address tag in the first byte of the chunk.
   void storeEndMarker(uptr End, uptr Size, uptr BlockEnd) {
+    DCHECK_EQ(BlockEnd, untagPointer(BlockEnd));
     uptr UntaggedEnd = untagPointer(End);
     if (UntaggedEnd != BlockEnd) {
       storeTag(UntaggedEnd);
       if (Size == 0)
         *reinterpret_cast<u8 *>(UntaggedEnd) = extractTag(End);
     }
   }