Do not save/restore a pointer across an in-place realloc. (#1925)

For `p = realloc(p, sz)`, VisitBinaryOperator saved the pre-realloc
pointer -- both the primal p and its adjoint _d_p -- to restore it in
the reverse sweep. realloc frees the old block, so the restore installed
a dangling pointer that the sweep then read and wrote, and the cleanup
free() double-freed it. Valgrind reports the Invalid read/write and
Invalid free in Gradient/Pointers.C once the clang false positives are
suppressed.

A growing or same-size realloc preserves the reused contents, so the
reallocated pointer stays valid for every reverse access. Detect an
in-place realloc -- the LHS is realloc's own pointer argument -- and skip
both the primal and the derivative store/restore, leaving p and _d_p in
place. A shrinking in-place realloc stays unhandled (its reverse sweep
may read the freed tail out of bounds, as before); no test exercises it.

Author: vgvassilev

lib/Differentiator/ReverseModeVisitor.cpp

@@ -3146,9 +3146,30 @@ Expr* ReverseModeVisitor::getStdInitListSizeExpr(const Expr* E) {
         Lblock.erase(Lblock.begin());
       }
 
+      // For an in-place `p = realloc(p, sz)`, realloc frees the old block, so
+      // saving p to restore it in the reverse sweep would dangle (and be
+      // double-freed at cleanup). Keep the reallocated pointer instead, for
+      // both p and its adjoint _d_p. This is sound for a growing or same-size
+      // realloc, where every reverse access stays in bounds. A shrinking
+      // in-place realloc is not handled -- its reverse sweep may read the
+      // freed tail out of bounds, as it did before this change; no test
+      // exercises it.
+      bool isReallocAssignment = false;
+      if (opCode == BO_Assign)
+        if (auto* RCall = dyn_cast<CallExpr>(R->IgnoreParenCasts()))
+          if (const FunctionDecl* RFD = RCall->getDirectCallee())
+            if (RFD->getNameAsString() == "realloc" && RCall->getNumArgs()) {
+              // Only in-place: the LHS must be realloc's own pointer argument.
+              const auto* LDRE = dyn_cast<DeclRefExpr>(L->IgnoreParenCasts());
+              const auto* ArgDRE =
+                  dyn_cast<DeclRefExpr>(RCall->getArg(0)->IgnoreParenCasts());
+              isReallocAssignment =
+                  LDRE && ArgDRE && LDRE->getDecl() == ArgDRE->getDecl();
+            }
+
       // Store the value of the LHS of the assignment in the forward pass
       // and restore it in the reverse pass
-      if (m_DiffReq.shouldBeRecorded(L)) {
+      if (m_DiffReq.shouldBeRecorded(L) && !isReallocAssignment) {
         // Clone: LCloned is also consumed by the forward reconstruction, so the
         // store/restore must not share it.
         StmtDiff pushPop = StoreAndRestore(CloneNode(LCloned));
@@ -3167,8 +3188,8 @@ Expr* ReverseModeVisitor::getStdInitListSizeExpr(const Expr* E) {
       if (!ResultRef)
         return Clone(BinOp);
       // We need to store values of derivative pointer variables in forward pass
-      // and restore them in reverse pass.
-      if (isPointerOp) {
+      // and restore them in reverse pass (except across a realloc, see above).
+      if (isPointerOp && !isReallocAssignment) {
         // Ldiff.getExpr_dx() is reused below (ResultRef, the derivative op);
         // clone it for the store/restore so the node is not shared.
         StmtDiff pushPop = StoreAndRestore(CloneNode(Ldiff.getExpr_dx()));

test/Gradient/Pointers.C

@@ -2,8 +2,6 @@
 // RUN: ./Pointers.out | %filecheck_exec %s
 // RUN: %cladclang -Xclang -plugin-arg-clad -Xclang -disable-tbr -Xclang -plugin-arg-clad -Xclang -enable-va %s -I%S/../../include -oPointers.out
 // RUN: ./Pointers.out | %filecheck_exec %s
-// FIXME: real realloc use-after-free in reverse mode; drop valgrind when fixed.
-// XFAIL: target={{i586.*}}, valgrind
 
 #include "clad/Differentiator/Differentiator.h"
 
@@ -338,32 +336,26 @@ double cStyleMemoryAlloc(double x, size_t n) {
 // CHECK-NEXT:     *p = x;
 // CHECK-NEXT:     double _d_res = 0.;
 // CHECK-NEXT:     double res = t->x + *p;
-// CHECK-NEXT:     double *_t2 = p;
-// CHECK-NEXT:     double *_t3 = _d_p;
 // CHECK-NEXT:     _d_p = (double *)realloc(_d_p, 2 * sizeof(double));
 // CHECK-NEXT:     memset(_d_p, 0, 2 * sizeof(double));
 // CHECK-NEXT:     p = (double *)realloc(p, 2 * sizeof(double));
-// CHECK-NEXT:     double _t4 = p[1];
+// CHECK-NEXT:     double _t2 = p[1];
 // CHECK-NEXT:     p[1] = 2 * x;
-// CHECK-NEXT:     double _t5 = res;
+// CHECK-NEXT:     double _t3 = res;
 // CHECK-NEXT:     res += p[1];
 // CHECK-NEXT:     _d_res += 1;
 // CHECK-NEXT:     {
-// CHECK-NEXT:         res = _t5;
+// CHECK-NEXT:         res = _t3;
 // CHECK-NEXT:         double _r_d3 = _d_res;
 // CHECK-NEXT:         _d_p[1] += _r_d3;
 // CHECK-NEXT:     }
 // CHECK-NEXT:     {
-// CHECK-NEXT:         p[1] = _t4;
+// CHECK-NEXT:         p[1] = _t2;
 // CHECK-NEXT:         double _r_d2 = _d_p[1];
 // CHECK-NEXT:         _d_p[1] = 0.;
 // CHECK-NEXT:         *_d_x += 2 * _r_d2;
 // CHECK-NEXT:     }
 // CHECK-NEXT:     {
-// CHECK-NEXT:         p = _t2;
-// CHECK-NEXT:         _d_p = _t3;
-// CHECK-NEXT:     }
-// CHECK-NEXT:     {
 // CHECK-NEXT:         _d_t->x += _d_res;
 // CHECK-NEXT:         *_d_p += _d_res;
 // CHECK-NEXT:     }