[flang][runtime] Interoperable POINTER deallocation validation #96100
+64
−29
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Extend the runtime validation of deallocated pointers so that it also works when pointers are allocated &/or deallocated outside Fortran. Previously, bogus runtime errors would be reported for pointers allocated via CFI_allocate() and deallocated in Fortran, and CFI_deallocate() did not check that it was deallocating a whole contiguous pointer that was allocated as such.
llvmbot
added
flang:runtime
flang
|
@llvm/pr-subscribers-flang-runtime Author: Peter Klausler (klausler) ChangesExtend the runtime validation of deallocated pointers so that it also works when pointers are allocated &/or deallocated outside Fortran. Previously, bogus runtime errors would be reported for pointers allocated via CFI_allocate() and deallocated in Fortran, and CFI_deallocate() did not check that it was deallocating a whole contiguous pointer that was allocated as such. Full diff: https://github.com/llvm/llvm-project/pull/96100.diff 4 Files Affected:
diff --git a/flang/include/flang/Runtime/pointer.h b/flang/include/flang/Runtime/pointer.h
index 6ceb70ebb676d..704144f08114f 100644
--- a/flang/include/flang/Runtime/pointer.h
+++ b/flang/include/flang/Runtime/pointer.h
@@ -115,6 +115,11 @@ bool RTDECL(PointerIsAssociated)(const Descriptor &);
bool RTDECL(PointerIsAssociatedWith)(
const Descriptor &, const Descriptor *target);
+// Fortran POINTERs are allocated with an extra validation word after their
+// payloads in order to detect erroneous deallocations later.
+RT_API_ATTRS void *AllocateValidatedPointerPayload(std::size_t);
+RT_API_ATTRS bool ValidatePointerPayload(const ISO::CFI_cdesc_t &);
+
} // extern "C"
} // namespace Fortran::runtime
#endif // FORTRAN_RUNTIME_POINTER_H_
diff --git a/flang/runtime/ISO_Fortran_binding.cpp b/flang/runtime/ISO_Fortran_binding.cpp
index 99ba3aa56feee..fe22026f31f55 100644
--- a/flang/runtime/ISO_Fortran_binding.cpp
+++ b/flang/runtime/ISO_Fortran_binding.cpp
@@ -13,6 +13,7 @@
#include "terminator.h"
#include "flang/ISO_Fortran_binding_wrapper.h"
#include "flang/Runtime/descriptor.h"
+#include "flang/Runtime/pointer.h"
#include "flang/Runtime/type-code.h"
#include <cstdlib>
@@ -75,7 +76,7 @@ RT_API_ATTRS int CFI_allocate(CFI_cdesc_t *descriptor,
dim->sm = byteSize;
byteSize *= extent;
}
- void *p{byteSize ? std::malloc(byteSize) : std::malloc(1)};
+ void *p{runtime::AllocateValidatedPointerPayload(byteSize)};
if (!p && byteSize) {
return CFI_ERROR_MEM_ALLOCATION;
}
@@ -91,8 +92,11 @@ RT_API_ATTRS int CFI_deallocate(CFI_cdesc_t *descriptor) {
if (descriptor->version != CFI_VERSION) {
return CFI_INVALID_DESCRIPTOR;
}
- if (descriptor->attribute != CFI_attribute_allocatable &&
- descriptor->attribute != CFI_attribute_pointer) {
+ if (descriptor->attribute == CFI_attribute_pointer) {
+ if (!runtime::ValidatePointerPayload(*descriptor)) {
+ return CFI_INVALID_DESCRIPTOR;
+ }
+ } else if (descriptor->attribute != CFI_attribute_allocatable) {
// Non-interoperable object
return CFI_INVALID_DESCRIPTOR;
}
diff --git a/flang/runtime/descriptor.cpp b/flang/runtime/descriptor.cpp
index d8b51f1be0c5c..9b04cb4f8d0d0 100644
--- a/flang/runtime/descriptor.cpp
+++ b/flang/runtime/descriptor.cpp
@@ -199,7 +199,16 @@ RT_API_ATTRS int Descriptor::Destroy(
}
}
-RT_API_ATTRS int Descriptor::Deallocate() { return ISO::CFI_deallocate(&raw_); }
+RT_API_ATTRS int Descriptor::Deallocate() {
+ ISO::CFI_cdesc_t &descriptor{raw()};
+ if (!descriptor.base_addr) {
+ return CFI_ERROR_BASE_ADDR_NULL;
+ } else {
+ std::free(descriptor.base_addr);
+ descriptor.base_addr = nullptr;
+ return CFI_SUCCESS;
+ }
+}
RT_API_ATTRS bool Descriptor::DecrementSubscripts(
SubscriptValue *subscript, const int *permutation) const {
diff --git a/flang/runtime/pointer.cpp b/flang/runtime/pointer.cpp
index 08a1223764f39..aeed879f1a2e2 100644
--- a/flang/runtime/pointer.cpp
+++ b/flang/runtime/pointer.cpp
@@ -124,6 +124,23 @@ void RTDEF(PointerAssociateRemapping)(Descriptor &pointer,
}
}
+RT_API_ATTRS void *AllocateValidatedPointerPayload(std::size_t byteSize) {
+ // Add space for a footer to validate during deallocation.
+ constexpr std::size_t align{sizeof(std::uintptr_t)};
+ byteSize = ((byteSize / align) + 1) * align;
+ std::size_t total{byteSize + sizeof(std::uintptr_t)};
+ void *p{std::malloc(total)};
+ if (p) {
+ // Fill the footer word with the XOR of the ones' complement of
+ // the base address, which is a value that would be highly unlikely
+ // to appear accidentally at the right spot.
+ std::uintptr_t *footer{
+ reinterpret_cast<std::uintptr_t *>(static_cast<char *>(p) + byteSize)};
+ *footer = ~reinterpret_cast<std::uintptr_t>(p);
+ }
+ return p;
+}
+
int RTDEF(PointerAllocate)(Descriptor &pointer, bool hasStat,
const Descriptor *errMsg, const char *sourceFile, int sourceLine) {
Terminator terminator{sourceFile, sourceLine};
@@ -137,22 +154,12 @@ int RTDEF(PointerAllocate)(Descriptor &pointer, bool hasStat,
elementBytes = pointer.raw().elem_len = 0;
}
std::size_t byteSize{pointer.Elements() * elementBytes};
- // Add space for a footer to validate during DEALLOCATE.
- constexpr std::size_t align{sizeof(std::uintptr_t)};
- byteSize = ((byteSize + align - 1) / align) * align;
- std::size_t total{byteSize + sizeof(std::uintptr_t)};
- void *p{std::malloc(total)};
+ void *p{AllocateValidatedPointerPayload(byteSize)};
if (!p) {
return ReturnError(terminator, CFI_ERROR_MEM_ALLOCATION, errMsg, hasStat);
}
pointer.set_base_addr(p);
pointer.SetByteStrides();
- // Fill the footer word with the XOR of the ones' complement of
- // the base address, which is a value that would be highly unlikely
- // to appear accidentally at the right spot.
- std::uintptr_t *footer{
- reinterpret_cast<std::uintptr_t *>(static_cast<char *>(p) + byteSize)};
- *footer = ~reinterpret_cast<std::uintptr_t>(p);
int stat{StatOk};
if (const DescriptorAddendum * addendum{pointer.Addendum()}) {
if (const auto *derived{addendum->derivedType()}) {
@@ -176,6 +183,27 @@ int RTDEF(PointerAllocateSource)(Descriptor &pointer, const Descriptor &source,
return stat;
}
+static RT_API_ATTRS std::size_t GetByteSize(
+ const ISO::CFI_cdesc_t &descriptor) {
+ std::size_t rank{descriptor.rank};
+ const ISO::CFI_dim_t *dim{descriptor.dim};
+ std::size_t byteSize{descriptor.elem_len};
+ for (std::size_t j{0}; j < rank; ++j) {
+ byteSize *= dim[j].extent;
+ }
+ return byteSize;
+}
+
+bool RT_API_ATTRS ValidatePointerPayload(const ISO::CFI_cdesc_t &desc) {
+ std::size_t byteSize{GetByteSize(desc)};
+ constexpr std::size_t align{sizeof(std::uintptr_t)};
+ byteSize = ((byteSize / align) + 1) * align;
+ const void *p{desc.base_addr};
+ const std::uintptr_t *footer{reinterpret_cast<const std::uintptr_t *>(
+ static_cast<const char *>(p) + byteSize)};
+ return *footer == ~reinterpret_cast<std::uintptr_t>(p);
+}
+
int RTDEF(PointerDeallocate)(Descriptor &pointer, bool hasStat,
const Descriptor *errMsg, const char *sourceFile, int sourceLine) {
Terminator terminator{sourceFile, sourceLine};
@@ -185,20 +213,9 @@ int RTDEF(PointerDeallocate)(Descriptor &pointer, bool hasStat,
if (!pointer.IsAllocated()) {
return ReturnError(terminator, StatBaseNull, errMsg, hasStat);
}
- if (executionEnvironment.checkPointerDeallocation) {
- // Validate the footer. This should fail if the pointer doesn't
- // span the entire object, or the object was not allocated as a
- // pointer.
- std::size_t byteSize{pointer.Elements() * pointer.ElementBytes()};
- constexpr std::size_t align{sizeof(std::uintptr_t)};
- byteSize = ((byteSize + align - 1) / align) * align;
- void *p{pointer.raw().base_addr};
- std::uintptr_t *footer{
- reinterpret_cast<std::uintptr_t *>(static_cast<char *>(p) + byteSize)};
- if (*footer != ~reinterpret_cast<std::uintptr_t>(p)) {
- return ReturnError(
- terminator, StatBadPointerDeallocation, errMsg, hasStat);
- }
+ if (executionEnvironment.checkPointerDeallocation &&
+ !ValidatePointerPayload(pointer.raw())) {
+ return ReturnError(terminator, StatBadPointerDeallocation, errMsg, hasStat);
}
return ReturnError(terminator,
pointer.Destroy(/*finalize=*/true, /*destroyPointers=*/true, &terminator),
|
vzakhari
reviewed
Jun 24, 2024
| RT_API_ATTRS void *AllocateValidatedPointerPayload(std::size_t byteSize) { | ||
| // Add space for a footer to validate during deallocation. | ||
| constexpr std::size_t align{sizeof(std::uintptr_t)}; | ||
| byteSize = ((byteSize / align) + 1) * align; |
There was a problem hiding this comment.
Suggested change
| byteSize = ((byteSize / align) + 1) * align; | |
| byteSize = ((byteSize + align - 1) / align) * align; |
There was a problem hiding this comment.
If byteSize is initially zero, I want the result to be align, not zero.
There was a problem hiding this comment.
Can you explain why? If byteSize is initially zero, the total allocation will end up being 2 * align - isn't align bytes allocation enough?
There was a problem hiding this comment.
((0/align)+1)*align -> (0+1)*align -> 1*align -> align, yes?
There was a problem hiding this comment.
Yes, byteSize is equal to align, and then total (below) is equal to align + sizeof(std::uintptr_t) - this is how much memory is being malloc'ed. I am asking why we need to malloc more than align bytes of memory.
There was a problem hiding this comment.
We don't. Will fix. I was misunderstanding your comment.
AlexisPerry
pushed a commit
to llvm-project-tlp/llvm-project
that referenced
this pull request
Jul 9, 2024
…96100) Extend the runtime validation of deallocated pointers so that it also works when pointers are allocated &/or deallocated outside Fortran. Previously, bogus runtime errors would be reported for pointers allocated via CFI_allocate() and deallocated in Fortran, and CFI_deallocate() did not check that it was deallocating a whole contiguous pointer that was allocated as such.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Extend the runtime validation of deallocated pointers so that it also works when pointers are allocated &/or deallocated outside Fortran. Previously, bogus runtime errors would be reported for pointers allocated via CFI_allocate() and deallocated in Fortran, and CFI_deallocate() did not check that it was deallocating a whole contiguous pointer that was allocated as such.