[demangler] Avoid nullptr UB

[urnathan]

Summary

this resolves a UB introduced by D122604

The microsoft demangler demangles string literal contents (sans
quotes), and can therefore demangle the empty string to nothing. With
lazy buffer allocation, this results in a null buffer and consequent
detected UB with a memcpy. However, the fault is earlier, with taking
a StringView of that null buffer. This adds an allocateBuffer member,
to ensure that there is a non-null buffer, even when it is not needed.

A unit test also requires this, as it commences by inserting
no-characters and then checking for an empty string.

An assert is added on theOutputBuffer's conversion to StringView, to
catch future cases.

(My assumption that demangling either failed or always produced at
least one character turned out to be false in this one cases. I do
not know why the Microsoft demangler behaves this way, as it appears
to decorate the demangled strings with quotes before presentation to
the user.)

[dblaikie]

Hmm - why does this need an allocation, though? A StringPiece(nullptr, nullptr) sounds valid to me? Is something using a distinction between StringPiece that's zero-length but non-null and StringPiece that's null?

[urnathan]

In D124524#3477537, @dblaikie wrote:

Hmm - why does this need an allocation, though? A StringPiece(nullptr, nullptr) sounds valid to me? Is something using a distinction between StringPiece that's zero-length but non-null and StringPiece that's null?

The UB is that we end up at memcpy (dst, SV.begin(), SV.size()), even though that's copying zero bytes, we have a null SV.begin(). C++20's string_view cannot be over a null pointer, and ISTM that the demangler's StringView (should) have the same restriction. StringView is a pair of pointers, along with a FIXME to move to std::string_view when C++17 is the implementation language. Hm, perhaps the assert belongs inside StringView?

This seemed the least-worst way of addressing it. Alternatively I guess we could unconditionally allocate in OutputBuffer's ctor, but that seemed less pleasant.

[h-vetinari]

2x "Buffer cannot by nullptr" -> "Buffer cannot be nullptr"

[urnathan]

alternatively could punt on the whole nullptr thing and just special case the zero-length memcpy and test comparison. thoughts?
(For amusement value, I do know MIPS' memcpy routines segfault on memcpy (dst, nullptr, 0))

[xbolva00]

Can you show some C++ rule why you think that

memcpy(nullptr, nullptr, 0) is UB?

cc @rjmccall

Some time ago I was told this usage is perfectly fine and LLVM cannot assume that dst and src are nonnull pointers.

[urnathan]

In D124524#3479799, @xbolva00 wrote:

Can you show some C++ rule why you think that

memcpy(nullptr, nullptr, 0) is UB?

cc @rjmccall

Some time ago I was told this usage is perfectly fine and LLVM cannot assume that dst and src are nonnull pointers.

llvm's sanitizer barfs on such uses.

glibc's header files mark the pointer as non-null:
extern void *memcpy (void *restrict dest, const void *restrict src,

size_t __n) throw () __attribute__ ((__nonnull__ (1, 2)));

c99: 7.21.2.1 "The memcpy function copies n characters from the object pointed to by s2 into the
object pointed to by s1. If copying takes place between objects that overlap, the behavior
is undefined."

null is not a pointer to an object

It is an unfortunate edge case, but it is what it is.

[xbolva00]

In D124524#3479827, @urnathan wrote:

In D124524#3479799, @xbolva00 wrote:

Can you show some C++ rule why you think that

memcpy(nullptr, nullptr, 0) is UB?

cc @rjmccall

Some time ago I was told this usage is perfectly fine and LLVM cannot assume that dst and src are nonnull pointers.

llvm's sanitizer barfs on such uses.

glibc's header files mark the pointer as non-null:
extern void *memcpy (void *restrict dest, const void *restrict src,

size_t __n) throw () __attribute__ ((__nonnull__ (1, 2)));

c99: 7.21.2.1 "The memcpy function copies n characters from the object pointed to by s2 into the
object pointed to by s1. If copying takes place between objects that overlap, the behavior
is undefined."

null is not a pointer to an object

It is an unfortunate edge case, but it is what it is.

Yeah, so SImplifyLibCalls can just drop this restriction and be more aggresive.

[dblaikie]

In D124524#3478021, @urnathan wrote:

In D124524#3477537, @dblaikie wrote:

Hmm - why does this need an allocation, though? A StringPiece(nullptr, nullptr) sounds valid to me? Is something using a distinction between StringPiece that's zero-length but non-null and StringPiece that's null?

The UB is that we end up at memcpy (dst, SV.begin(), SV.size()), even though that's copying zero bytes, we have a null SV.begin(). C++20's string_view cannot be over a null pointer, and ISTM that the demangler's StringView (should) have the same restriction. StringView is a pair of pointers, along with a FIXME to move to std::string_view when C++17 is the implementation language. Hm, perhaps the assert belongs inside StringView?

This seemed the least-worst way of addressing it. Alternatively I guess we could unconditionally allocate in OutputBuffer's ctor, but that seemed less pleasant.

Ah, OK - thanks for walking me through it. The other alternative could be to wrap the memcpy in "if (SV.data() != nullptr)"? That feels more direct/explicit/intentional about the issue, to me at least.

[dblaikie]

Yeah, I'd go with the fix at memcpy in D122604 instead.

I appreciate the argument from string_view's API design - though even string_view can be over nullptr, but can't be constructed with a null pointer (its default constructor creates a {nullptr, 0} string_view) so arguably the caller should be able to handle a null string_view.

[urnathan]

In D124524#3486242, @dblaikie wrote:

Yeah, I'd go with the fix at memcpy in D122604 instead.

I appreciate the argument from string_view's API design - though even string_view can be over nullptr, but can't be constructed with a null pointer (its default constructor creates a {nullptr, 0} string_view) so arguably the caller should be able to handle a null string_view.

Oh, I'd not noticed that default ctor behavior. I've also looked up to make sure that C++ makes well defined subtracting nullptrs and adding/subbing zero to/from a nullptr. It's just the memcpy issue.

libcxxabi/src/demangle/Utility.h

 public:
   OutputBuffer(char *StartBuf, size_t Size)
       : Buffer(StartBuf), CurrentPosition(0), BufferCapacity(Size) {}
   OutputBuffer() = default;
   // Non-copyable
   OutputBuffer(const OutputBuffer &) = delete;
   OutputBuffer &operator=(const OutputBuffer &) = delete;
 
-  operator StringView() const { return StringView(Buffer, CurrentPosition); }
+  operator StringView() const {
+    assert(Buffer && "Buffer cannot by nullptr");
+    return StringView(Buffer, CurrentPosition);
+  }
 
   void reset(char *Buffer_, size_t BufferCapacity_) {
     CurrentPosition = 0;
     Buffer = Buffer_;
     BufferCapacity = BufferCapacity_;
   }
 
   /// If a ParameterPackExpansion (or similar type) is encountered, the offset
public:
 
   size_t getCurrentPosition() const { return CurrentPosition; }
   void setCurrentPosition(size_t NewPos) { CurrentPosition = NewPos; }
 
   char back() const {
     return CurrentPosition ? Buffer[CurrentPosition - 1] : '\0';
   }
 
+  // Some uses require a buffer even when they write nothing.
+  void allocateBuffer() { grow(1); }
   bool empty() const { return CurrentPosition == 0; }
 
   char *getBuffer() { return Buffer; }
   char *getBufferEnd() { return Buffer + CurrentPosition - 1; }
   size_t getBufferCapacity() const { return BufferCapacity; }
 };
 
 template <class T> class ScopedOverride {

llvm/include/llvm/Demangle/Utility.h

 public:
   OutputBuffer(char *StartBuf, size_t Size)
       : Buffer(StartBuf), CurrentPosition(0), BufferCapacity(Size) {}
   OutputBuffer() = default;
   // Non-copyable
   OutputBuffer(const OutputBuffer &) = delete;
   OutputBuffer &operator=(const OutputBuffer &) = delete;
 
-  operator StringView() const { return StringView(Buffer, CurrentPosition); }
+  operator StringView() const {
+    assert(Buffer && "Buffer cannot by nullptr");
+    return StringView(Buffer, CurrentPosition);
+  }
 
   void reset(char *Buffer_, size_t BufferCapacity_) {
     CurrentPosition = 0;
     Buffer = Buffer_;
     BufferCapacity = BufferCapacity_;
   }
 
   /// If a ParameterPackExpansion (or similar type) is encountered, the offset
public:
 
   size_t getCurrentPosition() const { return CurrentPosition; }
   void setCurrentPosition(size_t NewPos) { CurrentPosition = NewPos; }
 
   char back() const {
     return CurrentPosition ? Buffer[CurrentPosition - 1] : '\0';
   }
 
+  // Some uses require a buffer even when they write nothing.
+  void allocateBuffer() { grow(1); }
   bool empty() const { return CurrentPosition == 0; }
 
   char *getBuffer() { return Buffer; }
   char *getBufferEnd() { return Buffer + CurrentPosition - 1; }
   size_t getBufferCapacity() const { return BufferCapacity; }
 };
 
 template <class T> class ScopedOverride {

llvm/lib/Demangle/MicrosoftDemangle.cpp

case '1':
     IsWcharT = true;
     DEMANGLE_FALLTHROUGH;
   case '0':
     break;
   default:
     goto StringLiteralError;
   }
 
+  // We can demangle to an empty string, and need to avoid a null buffer in that
+  // case.
+  OB.allocateBuffer();
+
   // Encoded Length
   std::tie(StringByteSize, IsNegative) = demangleNumber(MangledName);
   if (Error || IsNegative || StringByteSize < (IsWcharT ? 2 : 1))
     goto StringLiteralError;
 
   // CRC 32 (always 8 characters plus a terminator)
   CrcEndPos = MangledName.find('@');
   if (CrcEndPos == StringView::npos)

llvm/unittests/Demangle/OutputBufferTest.cpp

TEST(OutputBufferTest, Format) {
   EXPECT_EQ("?", printToString('?'));
 
   EXPECT_EQ("abc", printToString("abc"));
 }
 
 TEST(OutputBufferTest, Insert) {
   OutputBuffer OB;
 
+  OB.allocateBuffer();
+
   OB.insert(0, "", 0);
   EXPECT_EQ("", toString(OB));
 
   OB.insert(0, "abcd", 4);
   EXPECT_EQ("abcd", toString(OB));
 
   OB.insert(0, "x", 1);
   EXPECT_EQ("xabcd", toString(OB));