[flang] Avoid passing null pointers to nonnull parameters

[kparzysz]

Certain functions in glibc have "nonnull" attributes on pointer parameters (even in cases where passing a null pointer should be handled correctly). There are a few cases of such calls in flang: memcmp and memcpy with the length parameter set to 0.

Avoid passing a null pointer to these functions, since the conflict with the nonnull attribute could cause an undefined behavior.

This was detected by the undefined behavior sanitizer.

[llvmbot]

@llvm/pr-subscribers-flang-parser

@llvm/pr-subscribers-flang-runtime

Author: Krzysztof Parzyszek (kparzysz)

Changes

Certain functions in glibc have "nonnull" attributes on pointer parameters (even in cases where passing a null pointer should be handled correctly). There are a few cases of such calls in flang: memcmp and memcpy with the length parameter set to 0.

Avoid passing a null pointer to these functions, since the conflict with the nonnull attribute could cause an undefined behavior.

This was detected by the undefined behavior sanitizer.

---

Full diff: https://github.com/llvm/llvm-project/pull/84785.diff

3 Files Affected:

• (modified) flang/include/flang/Parser/char-block.h (+7)
• (modified) flang/runtime/buffer.h (+8-3)
• (modified) flang/runtime/temporary-stack.cpp (+7-2)

diff --git a/flang/include/flang/Parser/char-block.h b/flang/include/flang/Parser/char-block.h
index fc5de2607b51b9..acd8aee98bf8db 100644
--- a/flang/include/flang/Parser/char-block.h
+++ b/flang/include/flang/Parser/char-block.h
@@ -129,6 +129,13 @@ class CharBlock {
 
 private:
   int Compare(const CharBlock &that) const {
+    // "memcmp" in glibc has "nonnull" attributes on the input pointers.
+    // Avoid passing null pointers, since it would result in an undefined
+    // behavior.
+    if (size() == 0)
+      return that.size() == 0 ? 0 : -1;
+    if (that.size() == 0)
+      return 1;
     std::size_t bytes{std::min(size(), that.size())};
     int cmp{std::memcmp(static_cast<const void *>(begin()),
         static_cast<const void *>(that.begin()), bytes)};
diff --git a/flang/runtime/buffer.h b/flang/runtime/buffer.h
index a77a5a5dda5c78..93fda36f500d31 100644
--- a/flang/runtime/buffer.h
+++ b/flang/runtime/buffer.h
@@ -148,10 +148,15 @@ template <typename STORE, std::size_t minBuffer = 65536> class FileFrame {
       buffer_ =
           reinterpret_cast<char *>(AllocateMemoryOrCrash(terminator, size_));
       auto chunk{std::min<std::int64_t>(length_, oldSize - start_)};
-      std::memcpy(buffer_, old + start_, chunk);
+      // "memcpy" in glibc has a "nonnull" attribute on the source pointer.
+      // Avoid passing a null pointer, since it would result in an undefined
+      // behavior.
+      if (old != nullptr) {
+        std::memcpy(buffer_, old + start_, chunk);
+        std::memcpy(buffer_ + chunk, old, length_ - chunk);
+        FreeMemory(old);
+      }
       start_ = 0;
-      std::memcpy(buffer_ + chunk, old, length_ - chunk);
-      FreeMemory(old);
     }
   }
 
diff --git a/flang/runtime/temporary-stack.cpp b/flang/runtime/temporary-stack.cpp
index b4d7c6064457f2..667b10e04dbd29 100644
--- a/flang/runtime/temporary-stack.cpp
+++ b/flang/runtime/temporary-stack.cpp
@@ -93,8 +93,13 @@ void DescriptorStorage<COPY_VALUES>::resize(size_type newCapacity) {
   }
   Descriptor **newData =
       static_cast<Descriptor **>(AllocateMemoryOrCrash(terminator_, bytes));
-  memcpy(newData, data_, capacity_ * sizeof(Descriptor *));
-  FreeMemory(data_);
+  // "memcpy" in glibc has a "nonnull" attribute on the source pointer.
+  // Avoid passing a null pointer, since it would result in an undefined
+  // behavior.
+  if (data_ != nullptr) {
+    memcpy(newData, data_, capacity_ * sizeof(Descriptor *));
+    FreeMemory(data_);
+  }
   data_ = newData;
   capacity_ = newCapacity;
 }

[tblah]

Thanks for catching this. temporary-stack.cpp changes look good to me