Fix memory leak in mjCModel::Compile.

This is a very subtle leak caused interaction between longjmp and compiler optimisation. Specifically, at the point where the setjmp takes places, these pointers have never been reassigned from its nullptr initialization. Without the volatile keyword, the compiler is free to assume that these pointers remain nullptr when the setjmp returns, and therefore to pass nullptr directly to the mj_deleteModel and mj_deleteData calls in the subsequent catch block, without ever reading the actual pointer values.

PiperOrigin-RevId: 466649447
Change-Id: I51e82ec8b566deb144221e11a99d5124a8267de4

src/user/user_model.cc

@@ -2271,9 +2271,16 @@ static void warninghandler(const char* msg) {
 
 // compiler
 mjModel* mjCModel::Compile(const mjVFS* vfs) {
-  unsigned int i;
+  // The volatile keyword is necessary to prevent a possible memory leak due to
-  mjModel* m = 0;
+  // an interaction between longjmp and compiler optimization. Specifically, at
-  mjData* data = 0;
+  // the point where the setjmp takes places, these pointers have never been
+  // reassigned from their nullptr initialization. Without the volatile keyword,
+  // the compiler is free to assume that these pointers remain nullptr when the
+  // setjmp returns, and therefore to pass nullptr directly to the
+  // mj_deleteModel and mj_deleteData calls in the subsequent catch block,
+  // without ever reading the actual pointer values.
+  mjModel* volatile m = nullptr;
+  mjData* volatile data = nullptr;
 
   // save error and warning handlers
   void (*save_error)(const char*) = _mjPRIVATE__get_tls_error_fn();
@@ -2289,12 +2296,38 @@ mjModel* mjCModel::Compile(const mjVFS* vfs) {
   // init random number generator, to make textures reproducible
   srand(123);
 
-  // guard for exceptions
   try {
     if (setjmp(error_jmp_buf) != 0) {
+      // TryCompile resulted in an mju_error which was converted to a longjmp.
       throw mjCError(0, "engine error: %s", errortext);
     }
+    TryCompile(const_cast<mjModel**>(&m), const_cast<mjData**>(&data), vfs);
+  } catch (mjCError err) {
+    // deallocate everything allocated in Compile
+    mj_deleteModel(m);
+    mj_deleteData(data);
+    mjCBody* world = bodies[0];
+    Clear();
+    bodies.push_back(world);
 
+    // save error info
+    errInfo = err;
+
+    // restore handler, return 0
+    _mjPRIVATE__set_tls_error_fn(save_error);
+    _mjPRIVATE__set_tls_warning_fn(save_warning);
+    return nullptr;
+  }
+
+  // restore error handler, mark as compiled, return mjModel
+  _mjPRIVATE__set_tls_error_fn(save_error);
+  _mjPRIVATE__set_tls_warning_fn(save_warning);
+  compiled = true;
+  return m;
+}
+
+
+void mjCModel::TryCompile(mjModel** m, mjData** data, const mjVFS* vfs) {
   // check if nan test works
   double test = mjNAN;
   if (mjuu_defined(test)) {
@@ -2326,7 +2359,7 @@ mjModel* mjCModel::Compile(const mjVFS* vfs) {
   }
 
   // add missing keyframes
-    for (i=keys.size(); i<nkey; i++) {
+  for (int i=keys.size(); i<nkey; i++) {
     AddKey();
   }
 
@@ -2361,7 +2394,7 @@ mjModel* mjCModel::Compile(const mjVFS* vfs) {
   IndexAssets();
 
   // mark meshes that need convex hull
-    for (i=0; i<geoms.size(); i++) {
+  for (int i=0; i<geoms.size(); i++) {
     if (geoms[i]->meshid>=0 && geoms[i]->type==mjGEOM_MESH &&
         (geoms[i]->contype || geoms[i]->conaffinity)) {
       meshes[geoms[i]->meshid]->needhull = true;
@@ -2369,82 +2402,82 @@ mjModel* mjCModel::Compile(const mjVFS* vfs) {
   }
 
   // compile meshes (needed for geom compilation)
-    for (i=0; i<meshes.size(); i++) {
+  for (int i=0; i<meshes.size(); i++) {
     meshes[i]->Compile(vfs);
   }
 
   // automatically set nuser fields
   if (nuser_body == -1) {
     nuser_body = 0;
-      for (i=0; i<bodies.size(); i++) {
+    for (int i=0; i<bodies.size(); i++) {
       nuser_body = mjMAX(nuser_body, bodies[i]->userdata.size());
     }
   }
   if (nuser_jnt == -1) {
     nuser_jnt = 0;
-      for (i=0; i<joints.size(); i++) {
+    for (int i=0; i<joints.size(); i++) {
       nuser_jnt = mjMAX(nuser_jnt, joints[i]->userdata.size());
     }
   }
   if (nuser_geom == -1) {
     nuser_geom = 0;
-      for (i=0; i<geoms.size(); i++) {
+    for (int i=0; i<geoms.size(); i++) {
       nuser_geom = mjMAX(nuser_geom, geoms[i]->userdata.size());
     }
   }
   if (nuser_site == -1) {
     nuser_site = 0;
-      for (i=0; i<sites.size(); i++) {
+    for (int i=0; i<sites.size(); i++) {
       nuser_site = mjMAX(nuser_site, sites[i]->userdata.size());
     }
   }
   if (nuser_cam == -1) {
     nuser_cam = 0;
-      for (i=0; i<cameras.size(); i++) {
+    for (int i=0; i<cameras.size(); i++) {
       nuser_cam = mjMAX(nuser_cam, cameras[i]->userdata.size());
     }
   }
   if (nuser_tendon == -1) {
     nuser_tendon = 0;
-      for (i=0; i<tendons.size(); i++) {
+    for (int i=0; i<tendons.size(); i++) {
       nuser_tendon = mjMAX(nuser_tendon, tendons[i]->userdata.size());
     }
   }
   if (nuser_actuator == -1) {
     nuser_actuator = 0;
-      for (i=0; i<actuators.size(); i++) {
+    for (int i=0; i<actuators.size(); i++) {
       nuser_actuator = mjMAX(nuser_actuator, actuators[i]->userdata.size());
     }
   }
   if (nuser_sensor == -1) {
     nuser_sensor = 0;
-      for (i=0; i<sensors.size(); i++) {
+    for (int i=0; i<sensors.size(); i++) {
       nuser_sensor = mjMAX(nuser_sensor, sensors[i]->userdata.size());
     }
   }
 
   // compile objects in kinematic tree
-    for (i=0; i<bodies.size(); i++) {
+  for (int i=0; i<bodies.size(); i++) {
     bodies[i]->Compile();  // also compiles joints, geoms, sites, cameras, lights
   }
 
   // compile all other objects except for keyframes
-    for (i=0; i<skins.size(); i++) skins[i]->Compile(vfs);
+  for (int i=0; i<skins.size(); i++) skins[i]->Compile(vfs);
-    for (i=0; i<hfields.size(); i++) hfields[i]->Compile(vfs);
+  for (int i=0; i<hfields.size(); i++) hfields[i]->Compile(vfs);
-    for (i=0; i<textures.size(); i++) textures[i]->Compile(vfs);
+  for (int i=0; i<textures.size(); i++) textures[i]->Compile(vfs);
-    for (i=0; i<materials.size(); i++) materials[i]->Compile();
+  for (int i=0; i<materials.size(); i++) materials[i]->Compile();
-    for (i=0; i<pairs.size(); i++) pairs[i]->Compile();
+  for (int i=0; i<pairs.size(); i++) pairs[i]->Compile();
-    for (i=0; i<excludes.size(); i++) excludes[i]->Compile();
+  for (int i=0; i<excludes.size(); i++) excludes[i]->Compile();
-    for (i=0; i<equalities.size(); i++) equalities[i]->Compile();
+  for (int i=0; i<equalities.size(); i++) equalities[i]->Compile();
-    for (i=0; i<tendons.size(); i++) tendons[i]->Compile();
+  for (int i=0; i<tendons.size(); i++) tendons[i]->Compile();
-    for (i=0; i<actuators.size(); i++) actuators[i]->Compile();
+  for (int i=0; i<actuators.size(); i++) actuators[i]->Compile();
-    for (i=0; i<sensors.size(); i++) sensors[i]->Compile();
+  for (int i=0; i<sensors.size(); i++) sensors[i]->Compile();
-    for (i=0; i<numerics.size(); i++) numerics[i]->Compile();
+  for (int i=0; i<numerics.size(); i++) numerics[i]->Compile();
-    for (i=0; i<texts.size(); i++) texts[i]->Compile();
+  for (int i=0; i<texts.size(); i++) texts[i]->Compile();
-    for (i=0; i<tuples.size(); i++) tuples[i]->Compile();
+  for (int i=0; i<tuples.size(); i++) tuples[i]->Compile();
 
   // compile defaults: to enforce userdata length for writer
-    for (i=0; i<defaults.size(); i++) {
+  for (int i=0; i<defaults.size(); i++) {
     defaults[i]->Compile(this);
   }
 
@@ -2464,7 +2497,7 @@ mjModel* mjCModel::Compile(const mjVFS* vfs) {
   }
 
   // set nmocap and body.mocapid
-    for (i=0; i<bodies.size(); i++) {
+  for (int i=0; i<bodies.size(); i++) {
     if (bodies[i]->mocap) {
       bodies[i]->mocapid = nmocap;
       nmocap++;
@@ -2474,7 +2507,7 @@ mjModel* mjCModel::Compile(const mjVFS* vfs) {
   }
 
   // check body mass and inertia
-    for (i=1; i<bodies.size(); i++) {
+  for (int i=1; i<bodies.size(); i++) {
     mjCBody* b = bodies[i];
 
     // find moving body with small mass or inertia
@@ -2504,7 +2537,7 @@ mjModel* mjCModel::Compile(const mjVFS* vfs) {
   }
 
   // create low-level model
-    m = mj_makeModel(nq, nv, nu, na, nbody, njnt, ngeom, nsite, ncam, nlight,
+  *m = mj_makeModel(nq, nv, nu, na, nbody, njnt, ngeom, nsite, ncam, nlight,
                     nmesh, nmeshvert, nmeshtexvert, nmeshface, nmeshgraph,
                     nskin, nskinvert, nskintexvert, nskinface, nskinbone, nskinbonevert,
                     nhfield, nhfielddata, ntex, ntexdata, nmat, npair, nexclude,
@@ -2513,109 +2546,85 @@ mjModel* mjCModel::Compile(const mjVFS* vfs) {
                     ntuple, ntupledata, nkey, nmocap,
                     nuser_body, nuser_jnt, nuser_geom, nuser_site, nuser_cam,
                     nuser_tendon, nuser_actuator, nuser_sensor, nnames);
-    if (!m) {
+  if (!*m) {
     throw mjCError(0, "could not create mjModel");
   }
 
   // copy everything into low-level model
-    m->opt = option;
+  (*m)->opt = option;
-    m->vis = visual;
+  (*m)->vis = visual;
-    CopyNames(m);
+  CopyNames(*m);
-    CopyTree(m);
+  CopyTree(*m);
 
   // keyframe compilation needs access to nq, nv, na, nmocap, qpos0
-    for (i=0; i<keys.size(); i++) {
+  for (int i=0; i<keys.size(); i++) {
-      keys[i]->Compile(m);
+    keys[i]->Compile(*m);
   }
 
   // copy objects outsite kinematic tree (including keyframes)
-    CopyObjects(m);
+  CopyObjects(*m);
 
   // scale mass
   if (settotalmass>0) {
-      mj_setTotalmass(m, settotalmass);
+    mj_setTotalmass(*m, settotalmass);
   }
 
   // set stack size: user-specified or conservative heuristic
   if (nstack>0) {
-      m->nstack = nstack;
+    (*m)->nstack = nstack;
   } else {
-      m->nstack = mjMAX(1000,
+    (*m)->nstack = mjMAX(
-                        5*(m->njmax + m->neq + m->nv)*(m->njmax + m->neq + m->nv) +
+        1000,
-                        20*(m->nq + m->nv + m->nu + m->na + m->nbody + m->njnt +
+        5*((*m)->njmax + (*m)->neq + (*m)->nv)*((*m)->njmax + (*m)->neq + (*m)->nv) +
-                            m->ngeom + m->nsite + m->neq + m->ntendon +  m->nwrap));
+        20*((*m)->nq + (*m)->nv + (*m)->nu + (*m)->na + (*m)->nbody + (*m)->njnt +
+            (*m)->ngeom + (*m)->nsite + (*m)->neq + (*m)->ntendon +  (*m)->nwrap));
   }
 
   // create data
-    data = mj_makeData(m);
+  *data = mj_makeData(*m);
   if (!data) {
     throw mjCError(0, "could not create mjData");
   }
 
   // normalize keyframe quaternions
-    for (i=0; i<m->nkey; i++) {
+  for (int i=0; i<(*m)->nkey; i++) {
-      mj_normalizeQuat(m, m->key_qpos+i*m->nq);
+    mj_normalizeQuat(*m, (*m)->key_qpos+i*(*m)->nq);
   }
 
   // set constant fields
-    mj_setConst(m, data);
+  mj_setConst(*m, *data);
 
   // automatic spring-damper adjustment
-    AutoSpringDamper(m);
+  AutoSpringDamper(*m);
 
   // actuator lengthrange computation
-    LengthRange(m, data);
+  LengthRange(*m, *data);
 
   // override model statistics if defined by user
-    if (mjuu_defined(extent)) m->stat.extent = (mjtNum)extent;
+  if (mjuu_defined(extent)) (*m)->stat.extent = (mjtNum)extent;
-    if (mjuu_defined(meaninertia)) m->stat.meaninertia = (mjtNum)meaninertia;
+  if (mjuu_defined(meaninertia)) (*m)->stat.meaninertia = (mjtNum)meaninertia;
-    if (mjuu_defined(meanmass)) m->stat.meanmass = (mjtNum)meanmass;
+  if (mjuu_defined(meanmass)) (*m)->stat.meanmass = (mjtNum)meanmass;
-    if (mjuu_defined(meansize)) m->stat.meansize = (mjtNum)meansize;
+  if (mjuu_defined(meansize)) (*m)->stat.meansize = (mjtNum)meansize;
-    if (mjuu_defined(center[0])) copyvec(m->stat.center, center, 3);
+  if (mjuu_defined(center[0])) copyvec((*m)->stat.center, center, 3);
 
   // assert that model has valid references
-    const char* validationerr = mj_validateReferences(m);
+  const char* validationerr = mj_validateReferences(*m);
   if (validationerr) {  // SHOULD NOT OCCUR
     throw mjCError(0, validationerr);
   }
   // test forward simulation
-    mj_resetData(m, data);
+  mj_resetData(*m, *data);
-    mj_step(m, data);
+  mj_step(*m, *data);
 
   // delete data
-    mj_deleteData(data);
+  mj_deleteData(*data);
-    data = NULL;
+  data = nullptr;
 
   // pass warning back
   if (warningtext[0]) {
     mju::strcpy_arr(errInfo.message, warningtext);
     errInfo.warning = true;
   }
-  }
-
-  // handle mjCError exceptions
-  catch (mjCError err) {
-    // deallocate everything allocated in Compile
-    mj_deleteModel(m);
-    mj_deleteData(data);
-    mjCBody* world = bodies[0];
-    Clear();
-    bodies.push_back(world);
-
-    // save error info
-    errInfo = err;
-
-    // restore handler, return 0
-    _mjPRIVATE__set_tls_error_fn(save_error);
-    _mjPRIVATE__set_tls_warning_fn(save_warning);
-    return nullptr;
-  }
-
-  // restore error handler, mark as compiled, return mjModel
-  _mjPRIVATE__set_tls_error_fn(save_error);
-  _mjPRIVATE__set_tls_warning_fn(save_warning);
-  compiled = true;
-  return m;
 }
 
 

src/user/user_model.h

@@ -153,6 +153,8 @@ class mjCModel {
   int nuser_sensor;               // number of mjtNums in sensor_user
 
  private:
+  void TryCompile(mjModel** m, mjData** data, const mjVFS* vfs);
+
   void Clear(void);               // clear objects allocated by Compile
 
   template <class T>              // add object of any type