placement new does not return aligned data in/near GetStackingAllocator()

[RobertHenry6bev]

Description

runtime/src/coreclr/vm/methodtablebuilder.cpp

Line 1054 in cc4dadd

bmtParent->pSlotTable = new (GetStackingAllocator())

(and many other places) invokes a placement new on a call to method GetStackingAllocator(), which calls UnsafeAllocNoThrow, which returns the placed new. The placed new is not aligned to 0 mod 16.

UnsafeAllocNoThrow() is a chamber of horrors.

• It returns a cast -1 when the number of things requested is 0, which happens. (Perhaps this in itself is a bug.)
• -1 is not aligned to 0 mod 16. It also has the magic number "7" in the code, which should really be the desired alignment (a power of 2) -1. (The code now assumes the caller is happy with alignment of 8 bytes.)

Reproduction Steps

compile with clang-14 -g -O0 -fsanitize=undefined; run the code,; and wait for runtime errors.

Expected behavior

no runtime errors

Actual behavior

runtime errors from ubsan

Regression?

No response

Known Workarounds

No response

Configuration

No response

Other information

cc / @AaronRobinsonMSFT

[AaronRobinsonMSFT]

The placed new is not aligned to 0 mod 16.

@RobertHenry6bev Other than things like SIMD, I'm not aware of any 16-byte alignment requirements. Am I overlooking something that requires that alignment guarantee in the runtime?

[jakobbotsch]

From what I can understand, only the global new operators (operator new(size_t) and operator new[](size_t) are required to return memory aligned to __STDCPP_DEFAULT_NEW_ALIGNMENT__ (otherwise I don't see how things like std::vector<T> would be implemented). For the placement forms the requirement is to return something that is aligned to the input type, i.e. alignof(bmtMethodSlotTable) in this case. So maybe the problem is that the latter is not happening?

[AaronRobinsonMSFT]

required to return memory aligned to __STDCPP_DEFAULT_NEW_ALIGNMENT__

Ah. Thanks @jakobbotsch. Agreed, this doesn't seem to apply to placement new operators.

[mangod9]

is this required in 7?

[AaronRobinsonMSFT]

is this required in 7?

Not at this point.

[janvorli]

@RobertHenry6bev do you know where it was called from when it got the 0 size argument? Comment in the code when checking for the zero 0 says it returns -1 to indicate a failure. We cannot return NULL in that case, as we would interpret it as OOM. I would expect it to crash at a caller due to the invalid address returned in such case.

[RobertHenry6bev]

0 StackingAllocator::UnsafeAllocNoThrow (this=0x7fffffff87c0, Size=0) at /mnt/robhenry/dotnet/clang11/runtime/src/coreclr/vm/stackingallocator.h:129
#1 0x00007ffff51c800e in operator new[] (n=0, alloc=0x7fffffff87c0) at /mnt/robhenry/dotnet/clang11/runtime/src/coreclr/vm/stackingallocator.cpp:395
#2 0x00007ffff5081aaf in MethodTableBuilder::EnumerateClassFields (this=0x7fffffffad58) at /mnt/robhenry/dotnet/clang11/runtime/src/coreclr/vm/methodtablebuilder.cpp:3352
#3 0x00007ffff5077e8b in MethodTableBuilder::BuildMethodTableThrowing (this=0x7fffffffad58, pAllocator=0x7ffff6d2dac0 <g_pSystemDomainMemory+2320>, pLoaderModule=0x7fff74a54000, pModule=0x7fff74a54000, cl=33554527,
pBuildingInterfaceList=0x0, pLayoutRawFieldInfos=0x0, pParentMethodTable=0x0, bmtGenericsInfo=0x7fffffffaef0, parentInst=..., cBuildingInterfaceList=0)
at /mnt/robhenry/dotnet/clang11/runtime/src/coreclr/vm/methodtablebuilder.cpp:1594
#4 0x00007ffff50bb016 in ClassLoader::CreateTypeHandleForTypeDefThrowing (pModule=0x7fff74a54000, cl=33554527, inst=..., pamTracker=0x7fffffffb458)
at /mnt/robhenry/dotnet/clang11/runtime/src/coreclr/vm/methodtablebuilder.cpp:12396
#5 0x00007ffff4adf4c5 in ClassLoader::CreateTypeHandleForTypeKey (pKey=0x7fffffffc080, pamTracker=0x7fffffffb458) at /mnt/robhenry/dotnet/clang11/runtime/src/coreclr/vm/clsload.cpp:2902
#6 0x00007ffff4adf17f in ClassLoader::DoIncrementalLoad (pTypeKey=0x7fffffffc080, typeHnd=..., currentLevel=CLASS_LOAD_BEGIN) at /mnt/robhenry/dotnet/clang11/runtime/src/coreclr/vm/clsload.cpp:2842
#7 0x00007ffff4ae28c0 in ClassLoader::LoadTypeHandleForTypeKey_Body (this=0x5555556e9f30, pTypeKey=0x7fffffffc080, typeHnd=..., targetLevel=CLASS_LOAD_EXACTPARENTS)
at /mnt/robhenry/dotnet/clang11/runtime/src/coreclr/vm/clsload.cpp:3518
#8 0x00007ffff4ad5c32 in ClassLoader::LoadTypeHandleForTypeKey (this=0x5555556e9f30, pTypeKey=0x7fffffffc080, typeHnd=..., targetLevel=CLASS_LOADED, pInstContext=0x0)
at /mnt/robhenry/dotnet/clang11/runtime/src/coreclr/vm/clsload.cpp:3237
#9 0x00007ffff4ad9caa in ClassLoader::LoadTypeDefThrowing (pModule=0x7fff74a54000, typeDef=33554527, fNotFoundAction=ClassLoader::ReturnNullIfNotFound, fUninstantiated=ClassLoader::PermitUninstDefOrRef, tokenNotToLoad=0,
level=CLASS_LOADED, pTargetInstantiation=0x0) at /mnt/robhenry/dotnet/clang11/runtime/src/coreclr/vm/clsload.cpp:2218
#10 0x00007ffff4acd0a3 in ClassLoader::LoadTypeHandleThrowing (this=0x5555556e9f30, pName=0x7fffffffc5e0, level=CLASS_LOADED, pLookInThisModuleOnly=0x0) at /mnt/robhenry/dotnet/clang11/runtime/src/coreclr/vm/clsload.cpp:1465
#11 0x00007ffff4acc7c9 in ClassLoader::LoadTypeHandleThrowIfFailed (this=0x5555556e9f30, pName=0x7fffffffc5e0, level=CLASS_LOADED, pLookInThisModuleOnly=0x0) at /mnt/robhenry/dotnet/clang11/runtime/src/coreclr/vm/clsload.cpp:324
#12 0x00007ffff4acc617 in ClassLoader::LoadTypeByNameThrowing (pAssembly=0x5555556e9ed0, nameSpace=0x7ffff5ec30c1 "System", name=0x7ffff5ec6c91 "Object", fNotFound=ClassLoader::ThrowIfNotFound, fLoadTypes=ClassLoader::LoadTypes,
level=CLASS_LOADED) at /mnt/robhenry/dotnet/clang11/runtime/src/coreclr/vm/clsload.cpp:285

Here's a "list" (source code) at the top frames of interest, in the likely event that your notion of line number is a little off from my notion of line number:

0 StackingAllocator::UnsafeAllocNoThrow (this=0x7fffffff87c0, Size=0) at /mnt/robhenry/dotnet/clang11/runtime/src/coreclr/vm/stackingallocator.h:129
129 RETURN (void*)(-1LL);
(gdb) list
124
125 assert(Size != 0);
126 //special case, 0 size alloc, return non-null but invalid pointer
127 if (Size == 0)
128 {
129 RETURN (void*)(-1LL);
130 // RETURN (void*)(-1LL & ~(STDCPP_DEFAULT_NEW_ALIGNMENT - 1));
131 }
132
133 // Round size up to ensure alignment.
(gdb) up
#1 0x00007ffff51c800e in operator new[] (n=0, alloc=0x7fffffff87c0) at /mnt/robhenry/dotnet/clang11/runtime/src/coreclr/vm/stackingallocator.cpp:395
395 void *retval = alloc->UnsafeAllocNoThrow((unsigned)n);
(gdb) list
390 if(n > (size_t)(1<<31)) ThrowOutOfMemory();
391 #else
392 if(n == (size_t)-1) ThrowOutOfMemory(); // overflow occurred
393 #endif
394
395 void *retval = alloc->UnsafeAllocNoThrow((unsigned)n);
396 if (retval == NULL)
397 ThrowOutOfMemory();
398
399 return retval;
(gdb) up
#2 0x00007ffff5081aaf in MethodTableBuilder::EnumerateClassFields (this=0x7fffffffad58) at /mnt/robhenry/dotnet/clang11/runtime/src/coreclr/vm/methodtablebuilder.cpp:3352
3352 bmtMetaData->pFields = new (GetStackingAllocator()) mdToken[bmtMetaData->cFields];
(gdb) list
3347 }
3348
3349 bmtMetaData->cFields = hEnumField.EnumGetCount();
3350
3351 // Retrieve the fields and store them in a temp array.
3352 bmtMetaData->pFields = new (GetStackingAllocator()) mdToken[bmtMetaData->cFields];
3353 bmtMetaData->pFieldAttrs = new (GetStackingAllocator()) DWORD[bmtMetaData->cFields];
3354
3355 DWORD dwFieldLiteralInitOnly = fdLiteral | fdInitOnly;
3356 DWORD dwMaxFieldDefRid = pMDInternalImport->GetCountWithTokenKind(mdtFieldDef);
(gdb)

[janvorli]

Ok, it looks like this is benign. The bmtMetaData->pFields and bmtMetaData->pFieldAttrs are never accessed if the bmtMetaData->cFields is zero.

[RobertHenry6bev]

I suspect that ubsan modifies the IL to check the address of the memory to be passed to the constructor assuming that if the constructor is invoked as new type_t[n] that n > 0. Perhaps this is a bug or over-generalization in ubsan.

Indeed, when new Type[0] is run, the clang++-14 built binaries complain at runtime with a runtime error, but g++-12 does not.

[RobertHenry6bev]

I opened issue

llvm/llvm-project#57032