Work with raw char pointers to have correct pointer arithmetic and avoid buffer overflow.

gtsam/nonlinear/Expression-inl.h

@@ -189,15 +189,13 @@ T Expression<T>::valueAndDerivatives(const Values& values,
 
 template<typename T>
 T Expression<T>::traceExecution(const Values& values,
-    internal::ExecutionTrace<T>& trace, void* traceStorage) const {
+    internal::ExecutionTrace<T>& trace, char* traceStorage) const {
-  return root_->traceExecution(values, trace,
+  return root_->traceExecution(values, trace, traceStorage);
-      static_cast<internal::ExecutionTraceStorage*>(traceStorage));
 }
 
 // Allocate a single block of aligned memory using a unique_ptr.
 inline std::unique_ptr<internal::ExecutionTraceStorage[]> allocAligned(size_t size) {
   const size_t alignedSize = (size + internal::TraceAlignment - 1) / internal::TraceAlignment;
-  std::cerr << size << " : " << alignedSize << '\n';
   return std::unique_ptr<internal::ExecutionTraceStorage[]>(
       new internal::ExecutionTraceStorage[alignedSize]);
 }
@@ -214,7 +212,7 @@ T Expression<T>::valueAndJacobianMap(const Values& values,
     // with an execution trace, made up entirely of "Record" structs, see
     // the FunctionalNode class in expression-inl.h
     internal::ExecutionTrace<T> trace;
-    T value(this->traceExecution(values, trace, traceStorage.get()));
+    T value(this->traceExecution(values, trace, reinterpret_cast<char *>(traceStorage.get())));
 
     // We then calculate the Jacobians using reverse automatic differentiation (AD).
     trace.startReverseAD1(jacobians);

gtsam/nonlinear/Expression.h

@@ -197,7 +197,7 @@ protected:
 
   /// trace execution, very unsafe
   T traceExecution(const Values& values, internal::ExecutionTrace<T>& trace,
-      void* traceStorage) const;
+      char* traceStorage) const;
 
   /// brief Return value and derivatives, reverse AD version
   T valueAndJacobianMap(const Values& values,

gtsam/nonlinear/internal/ExpressionNode.h

@@ -110,7 +110,7 @@ public:
 
   /// Construct an execution trace for reverse AD
   virtual T traceExecution(const Values& values, ExecutionTrace<T>& trace,
-      ExecutionTraceStorage* traceStorage) const = 0;
+      char* traceStorage) const = 0;
 };
 
 //-----------------------------------------------------------------------------
@@ -146,7 +146,7 @@ public:
 
   /// Construct an execution trace for reverse AD
   T traceExecution(const Values& values, ExecutionTrace<T>& trace,
-      ExecutionTraceStorage* traceStorage) const override {
+      char* traceStorage) const override {
     return constant_;
   }
 
@@ -199,7 +199,7 @@ public:
 
   /// Construct an execution trace for reverse AD
   T traceExecution(const Values& values, ExecutionTrace<T>& trace,
-      ExecutionTraceStorage* traceStorage) const override {
+      char* traceStorage) const override {
     trace.setLeaf(key_);
     return values.at<T>(key_);
   }
@@ -276,7 +276,7 @@ public:
     A1 value1;
 
     /// Construct record by calling argument expression
-    Record(const Values& values, const ExpressionNode<A1>& expression1, ExecutionTraceStorage* ptr)
+    Record(const Values& values, const ExpressionNode<A1>& expression1, char* ptr)
         : value1(expression1.traceExecution(values, trace1, ptr + upAligned(sizeof(Record)))) {}
 
     /// Print to std::cout
@@ -308,7 +308,7 @@ public:
 
   /// Construct an execution trace for reverse AD
   T traceExecution(const Values& values, ExecutionTrace<T>& trace,
-      ExecutionTraceStorage* ptr) const override {
+      char* ptr) const override {
     assert(reinterpret_cast<size_t>(ptr) % TraceAlignment == 0);
 
     // Create a Record in the memory pointed to by ptr
@@ -399,7 +399,7 @@ public:
 
     /// Construct record by calling argument expressions
     Record(const Values& values, const ExpressionNode<A1>& expression1,
-           const ExpressionNode<A2>& expression2, ExecutionTraceStorage* ptr)
+           const ExpressionNode<A2>& expression2, char* ptr)
         : value1(expression1.traceExecution(values, trace1, ptr += upAligned(sizeof(Record)))),
           value2(expression2.traceExecution(values, trace2, ptr += expression1.traceSize())) {}
 
@@ -427,7 +427,7 @@ public:
 
   /// Construct an execution trace for reverse AD, see UnaryExpression for explanation
   T traceExecution(const Values& values, ExecutionTrace<T>& trace,
-      ExecutionTraceStorage* ptr) const override {
+      char* ptr) const override {
     assert(reinterpret_cast<size_t>(ptr) % TraceAlignment == 0);
     Record* record = new (ptr) Record(values, *expression1_, *expression2_, ptr);
     trace.setFunction(record);
@@ -498,6 +498,8 @@ public:
   // Inner Record Class
   struct Record: public CallRecordImplementor<Record, traits<T>::dimension> {
 
+    EIGEN_MAKE_ALIGNED_OPERATOR_NEW
+
     typename Jacobian<T, A1>::type dTdA1;
     typename Jacobian<T, A2>::type dTdA2;
     typename Jacobian<T, A3>::type dTdA3;
@@ -513,7 +515,7 @@ public:
     /// Construct record by calling 3 argument expressions
     Record(const Values& values, const ExpressionNode<A1>& expression1,
            const ExpressionNode<A2>& expression2,
-           const ExpressionNode<A3>& expression3, ExecutionTraceStorage* ptr)
+           const ExpressionNode<A3>& expression3, char* ptr)
         : value1(expression1.traceExecution(values, trace1, ptr += upAligned(sizeof(Record)))),
           value2(expression2.traceExecution(values, trace2, ptr += expression1.traceSize())),
           value3(expression3.traceExecution(values, trace3, ptr += expression2.traceSize())) {}
@@ -545,7 +547,7 @@ public:
 
   /// Construct an execution trace for reverse AD, see UnaryExpression for explanation
   T traceExecution(const Values& values, ExecutionTrace<T>& trace,
-                           ExecutionTraceStorage* ptr) const override {
+                           char* ptr) const override {
     assert(reinterpret_cast<size_t>(ptr) % TraceAlignment == 0);
     Record* record = new (ptr) Record(values, *expression1_, *expression2_, *expression3_, ptr);
     trace.setFunction(record);
@@ -625,7 +627,7 @@ class ScalarMultiplyNode : public ExpressionNode<T> {
 
   /// Construct an execution trace for reverse AD
   T traceExecution(const Values& values, ExecutionTrace<T>& trace,
-                           ExecutionTraceStorage* ptr) const override {
+                           char* ptr) const override {
     assert(reinterpret_cast<size_t>(ptr) % TraceAlignment == 0);
     Record* record = new (ptr) Record();
     ptr += upAligned(sizeof(Record));
@@ -718,13 +720,13 @@ class BinarySumNode : public ExpressionNode<T> {
 
   /// Construct an execution trace for reverse AD
   T traceExecution(const Values &values, ExecutionTrace<T> &trace,
-                   ExecutionTraceStorage *ptr) const override {
+                   char* ptr) const override {
     assert(reinterpret_cast<size_t>(ptr) % TraceAlignment == 0);
     Record *record = new (ptr) Record();
     trace.setFunction(record);
 
-    auto ptr1 = reinterpret_cast<ExecutionTraceStorage *>(reinterpret_cast<char *>(ptr) + upAligned(sizeof(Record)));
+    auto ptr1 = ptr + upAligned(sizeof(Record));
-    auto ptr2 = reinterpret_cast<ExecutionTraceStorage *>(reinterpret_cast<char *>(ptr1) + expression1_->traceSize());
+    auto ptr2 = ptr1 + expression1_->traceSize();
     return expression1_->traceExecution(values, record->trace1, ptr1) +
            expression2_->traceExecution(values, record->trace2, ptr2);
   }

tests/testExpressionFactor.cpp

@@ -188,9 +188,9 @@ TEST(ExpressionFactor, Binary) {
   values.insert(2, Point2(0, 0));
 
   // Check size
-  internal::ExecutionTrace<Point2> trace;
   auto traceStorage = allocAligned(binary.traceSize());
-  Point2 value = binary.traceExecution(values, trace, traceStorage.get());
+  internal::ExecutionTrace<Point2> trace;
+  Point2 value = binary.traceExecution(values, trace, reinterpret_cast<char *>(traceStorage.get()));
   EXPECT(assert_equal(Point2(0,0),value, 1e-9))
   // trace.print();
 
@@ -240,9 +240,9 @@ TEST(ExpressionFactor, Shallow) {
 
   // traceExecution of shallow tree
   typedef internal::UnaryExpression<Point2, Point3> Unary;
-  internal::ExecutionTrace<Point2> trace;
   auto traceStorage = allocAligned(expression.traceSize());
-  Point2 value = expression.traceExecution(values, trace, traceStorage.get());
+  internal::ExecutionTrace<Point2> trace;
+  Point2 value = expression.traceExecution(values, trace, reinterpret_cast<char *>(traceStorage.get()));
   EXPECT(assert_equal(Point2(0,0),value, 1e-9))
   // trace.print();