From 11326326273bc4e38bc8721b0a992391f8ca5d83 Mon Sep 17 00:00:00 2001
From: Saran Tunyasuvunakool <stunya@google.com>
Date: Tue, 13 Aug 2024 03:07:48 -0700
Subject: [PATCH] Fix for benign UBsan signed-integer-overflow complaints.

When counting nl and nc, the nnz is optionally also counted when in sparse mode. However, the functions mj_nl and mj_nc unconditionally counts nnz into a temporary accumulator, then only afterwards check whether the output nnz pointer is NULL.

When not in sparse mode, ten_J_rownnz does not get initialized by mj_tendon, however mj_nl and mj_nc are accumulating potentially spurious data from ten_J_rownnz into the temporary accumulator. While the spurious result eventually gets discarded, UBsan nevertheless detects an integer overflow in the temporary accumulator.

PiperOrigin-RevId: 662443910
Change-Id: I78a9a088d4db7f16be779c7d669167d6fb3e4401
---
 src/engine/engine_core_constraint.c | 21 +++++++--------------
 1 file changed, 7 insertions(+), 14 deletions(-)

diff --git a/src/engine/engine_core_constraint.c b/src/engine/engine_core_constraint.c
index 5c4d9cf0..b07cc0d2 100644
--- a/src/engine/engine_core_constraint.c
+++ b/src/engine/engine_core_constraint.c
@@ -1706,7 +1706,7 @@ static int mj_ne(const mjModel* m, mjData* d, int* nnz) {
 
 // count frictional constraints, count Jacobian nonzeros if nnz is not NULL
 static int mj_nf(const mjModel* m, const mjData* d, int *nnz) {
-  int nf = 0, nnzf = 0;
+  int nf = 0;
   int nv = m->nv, ntendon = m->ntendon;
 
   if (mjDISABLED(mjDSBL_FRICTIONLOSS)) {
@@ -1716,21 +1716,17 @@ static int mj_nf(const mjModel* m, const mjData* d, int *nnz) {
   for (int i=0; i < nv; i++) {
     if (m->dof_frictionloss[i] > 0) {
       nf += mj_addConstraintCount(m, 1, 1);
-      nnzf++;
+      if (nnz) *nnz += 1;
     }
   }
 
   for (int i=0; i < ntendon; i++) {
     if (m->tendon_frictionloss[i] > 0) {
       nf += mj_addConstraintCount(m, 1, d->ten_J_rownnz[i]);
-      nnzf += d->ten_J_rownnz[i];
+      if (nnz) *nnz += d->ten_J_rownnz[i];
     }
   }
 
-  if (nnz) {
-    *nnz += nnzf;
-  }
-
   return nf;
 }
 
@@ -1738,7 +1734,7 @@ static int mj_nf(const mjModel* m, const mjData* d, int *nnz) {
 
 // count limit constraints, count Jacobian nonzeros if nnz is not NULL
 static int mj_nl(const mjModel* m, const mjData* d, int *nnz) {
-  int nnzl = 0, nl = 0;
+  int nl = 0;
   int ntendon = m->ntendon;
   int side;
   mjtNum margin, value, dist;
@@ -1763,7 +1759,7 @@ static int mj_nl(const mjModel* m, const mjData* d, int *nnz) {
         dist = side * (m->jnt_range[2*i+(side+1)/2] - value);
         if (dist < margin) {
           nl += mj_addConstraintCount(m, 1, 1);
-          nnzl++;
+          if (nnz) *nnz += 1;
         }
       }
     }
@@ -1777,7 +1773,7 @@ static int mj_nl(const mjModel* m, const mjData* d, int *nnz) {
       dist = mju_max(m->jnt_range[2*i], m->jnt_range[2*i+1]) - value;
       if (dist < margin) {
         nl += mj_addConstraintCount(m, 1, 3);
-        nnzl += 3;
+        if (nnz) *nnz += 3;
       }
     }
   }
@@ -1792,15 +1788,12 @@ static int mj_nl(const mjModel* m, const mjData* d, int *nnz) {
         dist = side * (m->tendon_range[2*i+(side+1)/2] - value);
         if (dist < margin) {
           nl += mj_addConstraintCount(m, 1, d->ten_J_rownnz[i]);
-          nnzl += d->ten_J_rownnz[i];
+          if (nnz) *nnz += d->ten_J_rownnz[i];
         }
       }
     }
   }
 
-  if (nnz) {
-    *nnz += nnzl;
-  }
   return nl;
 }