libnbc: int overflow "offset = i * count * extent"

This bug was hit inside an MPI_Iallgather() test using 4 ranks
each contributing 800,000,000 MPI_LONG_LONG where the overflow
is near the top of nbc_allgather_init().

The code was
```rbuf = (char *) recvbuf + rank * recvcount * rcvext;```
where rank and recvcount are ints, and rcvext is an MPI_Aint.  But
the left part of the arithmetic happens first and overflows before
it gets to the Aint promotion.

So I changed this to
```ruf = (char *) recvbuf + (MPI_Aint) rcvext * rank * recvcount;```

The philosopy for styling the change this particular way is
1. readability: putting an explicit cast at the start of a
   multiplication makes it obvious at a glance that adequate
   int-sizing is in place to avoid overflow
2. performance: reordering it such that the first typecast is
   extraneous (the extents are already MPI_Aint) avoids any
   potential performance penalty vs for example if we put the
   typecast on "rank".  It would also work to just use
   "rcvext*rank*recvcount" since rcvext is already MPI_Aint, but
   I like the explicit typecast as it guarantees no overflow
   without having to double check each spot to make sure the
   extent variable really is an MPI_Aint as one would expect
   it to be.

I'm not 100% convinced that performance penalty is real, but it
was a concern raised by bosilca and I can't say for sure it's not
real.

I made the same change a handful of places, although I only
have a testcase specifically hitting the overflow that was at the
top of nbc_allgather_init().

I also took the liberty of changing "int ext" in red_sched_chain()
to "MPI_Aint ext".  I can't see any reason for it to just be int.

Signed-off-by: Mark Allen <[email protected]>

Author: markalle

ompi/mca/coll/libnbc/nbc_iallgather.c

@@ -92,7 +92,7 @@ static int nbc_allgather_init(const void* sendbuf, int sendcount, MPI_Datatype s
     sendcount = recvcount;
   } else if (!persistent) { /* for persistent, the copy must be scheduled */
     /* copy my data to receive buffer */
-    rbuf = (char *) recvbuf + rank * recvcount * rcvext;
+    rbuf = (char *) recvbuf + (MPI_Aint)rcvext * rank * recvcount;
     res = NBC_Copy (sendbuf, sendcount, sendtype, rbuf, recvcount, recvtype, comm);
     if (OPAL_UNLIKELY(OMPI_SUCCESS != res)) {
       return res;
@@ -121,7 +121,7 @@ static int nbc_allgather_init(const void* sendbuf, int sendcount, MPI_Datatype s
     if (persistent && !inplace) {
       /* for nonblocking, data has been copied already */
       /* copy my data to receive buffer (= send buffer of NBC_Sched_send) */
-      rbuf = (char *)recvbuf + rank * recvcount * rcvext;
+      rbuf = (char *)recvbuf + (MPI_Aint) rcvext * rank * recvcount;
       res = NBC_Sched_copy((void *)sendbuf, false, sendcount, sendtype,
                             rbuf, false, recvcount, recvtype, schedule, true);
       if (OPAL_UNLIKELY(OMPI_SUCCESS != res)) {
@@ -237,7 +237,7 @@ static int nbc_allgather_inter_init(const void* sendbuf, int sendcount, MPI_Data
   /* do rsize - 1 rounds */
   for (int r = 0 ; r < rsize ; ++r) {
     /* recv from rank r */
-    rbuf = (char *) recvbuf + r * recvcount * rcvext;
+    rbuf = (char *) recvbuf + (MPI_Aint) rcvext * r * recvcount;
     res = NBC_Sched_recv (rbuf, false, recvcount, recvtype, r, schedule, false);
     if (OPAL_UNLIKELY(OMPI_SUCCESS != res)) {
       OBJ_RELEASE(schedule);
@@ -303,12 +303,12 @@ static inline int allgather_sched_linear(
     ptrdiff_t rlb, rext;
 
     res = ompi_datatype_get_extent(rdtype, &rlb, &rext);
-    char *sbuf = (char *)recvbuf + rank * rcount * rext;
+    char *sbuf = (char *)recvbuf + (MPI_Aint) rext * rank * rcount;
 
     for (int remote = 0; remote < comm_size ; ++remote) {
         if (remote != rank) {
             /* Recv from rank remote */
-            char *rbuf = (char *)recvbuf + remote * rcount * rext;
+            char *rbuf = (char *)recvbuf + (MPI_Aint) rext * remote * rcount;
             res = NBC_Sched_recv(rbuf, false, rcount, rdtype, remote, schedule, false);
             if (OPAL_UNLIKELY(OMPI_SUCCESS != res)) { goto cleanup_and_return; }
 

ompi/mca/coll/libnbc/nbc_ialltoall.c

@@ -331,14 +331,14 @@ static int nbc_alltoall_inter_init (const void* sendbuf, int sendcount, MPI_Data
 
   for (int i = 0; i < rsize; i++) {
     /* post all sends */
-    sbuf = (char *) sendbuf + i * sendcount * sndext;
+    sbuf = (char *) sendbuf + (MPI_Aint) sndext * i * sendcount;
     res = NBC_Sched_send (sbuf, false, sendcount, sendtype, i, schedule, false);
     if (OPAL_UNLIKELY(OMPI_SUCCESS != res)) {
       break;
     }
 
     /* post all receives */
-    rbuf = (char *) recvbuf + i * recvcount * rcvext;
+    rbuf = (char *) recvbuf + (MPI_Aint) rcvext * i * recvcount;
     res = NBC_Sched_recv (rbuf, false, recvcount, recvtype, i, schedule, false);
     if (OPAL_UNLIKELY(OMPI_SUCCESS != res)) {
       break;
@@ -397,13 +397,13 @@ static inline int a2a_sched_pairwise(int rank, int p, MPI_Aint sndext, MPI_Aint
     int sndpeer = (rank + r) % p;
     int rcvpeer = (rank - r + p) % p;
 
-    char *rbuf = (char *) recvbuf + rcvpeer * recvcount * rcvext;
+    char *rbuf = (char *) recvbuf + (MPI_Aint) rcvext * rcvpeer * recvcount;
     res = NBC_Sched_recv (rbuf, false, recvcount, recvtype, rcvpeer, schedule, false);
     if (OPAL_UNLIKELY(OMPI_SUCCESS != res)) {
       return res;
     }
 
-    char *sbuf = (char *) sendbuf + sndpeer * sendcount * sndext;
+    char *sbuf = (char *) sendbuf + (MPI_Aint) sndext * sndpeer * sendcount;
     res = NBC_Sched_send (sbuf, false, sendcount, sendtype, sndpeer, schedule, true);
     if (OPAL_UNLIKELY(OMPI_SUCCESS != res)) {
       return res;
@@ -523,7 +523,7 @@ static inline int a2a_sched_diss(int rank, int p, MPI_Aint sndext, MPI_Aint rcve
 
   /* phase 3 - reorder - data is now in wrong order in tmpbuf - reorder it into recvbuf */
   for (int i = 0 ; i < p; ++i) {
-    rbuf = (char *) recvbuf + ((rank - i + p) % p) * recvcount * rcvext;
+    rbuf = (char *) recvbuf + (MPI_Aint) rcvext * ((rank - i + p) % p) * recvcount;
     res = NBC_Sched_unpack ((void *)(intptr_t) (i * datasize), true, recvcount, recvtype, rbuf, false, schedule,
                             false);
     if (OPAL_UNLIKELY(OMPI_SUCCESS != res)) {

ompi/mca/coll/libnbc/nbc_ibcast.c

@@ -327,11 +327,11 @@ static inline int bcast_sched_chain(int rank, int p, int root, NBC_Schedule *sch
   fragcount = count/numfrag;
 
   for (int fragnum = 0 ; fragnum < numfrag ; ++fragnum) {
-    buf = (char *) buffer + fragnum * fragcount * ext;
+    buf = (char *) buffer + (MPI_Aint)ext * fragnum * fragcount;
     thiscount = fragcount;
     if (fragnum == numfrag-1) {
       /* last fragment may not be full */
-      thiscount = count - fragcount * fragnum;
+      thiscount = count - (size_t)fragcount * fragnum;
     }
 
     /* root does not receive */

ompi/mca/coll/libnbc/nbc_igather.c

@@ -103,7 +103,7 @@ static int nbc_gather_init(const void* sendbuf, int sendcount, MPI_Datatype send
       }
     } else {
       for (int i = 0 ; i < p ; ++i) {
-        rbuf = (char *)recvbuf + i * recvcount * rcvext;
+        rbuf = (char *)recvbuf + (MPI_Aint) rcvext * i * recvcount;
         if (i == root) {
           if (!inplace) {
             /* if I am the root - just copy the message */
@@ -228,7 +228,7 @@ static int nbc_gather_inter_init (const void* sendbuf, int sendcount, MPI_Dataty
         }
     } else if (MPI_ROOT == root) {
         for (int i = 0 ; i < rsize ; ++i) {
-            rbuf = ((char *)recvbuf) + (i * recvcount * rcvext);
+            rbuf = ((char *)recvbuf) + ((MPI_Aint) rcvext * i * recvcount);
             /* root receives message to the right buffer */
             res = NBC_Sched_recv (rbuf, false, recvcount, recvtype, i, schedule, false);
             if (OPAL_UNLIKELY(OMPI_SUCCESS != res)) {

ompi/mca/coll/libnbc/nbc_ineighbor_allgather.c

@@ -86,7 +86,7 @@ static int nbc_neighbor_allgather_init(const void *sbuf, int scount, MPI_Datatyp
 
     for (int i = 0 ; i < indegree ; ++i) {
       if (MPI_PROC_NULL != srcs[i]) {
-        res = NBC_Sched_recv ((char *) rbuf + i * rcount * rcvext, true, rcount, rtype, srcs[i], schedule, false);
+        res = NBC_Sched_recv ((char *) rbuf + (MPI_Aint) rcvext * i * rcount, true, rcount, rtype, srcs[i], schedule, false);
         if (OPAL_UNLIKELY(OMPI_SUCCESS != res)) {
           break;
         }

ompi/mca/coll/libnbc/nbc_ineighbor_alltoall.c

@@ -89,7 +89,7 @@ static int nbc_neighbor_alltoall_init(const void *sbuf, int scount, MPI_Datatype
 
     for (int i = 0 ; i < indegree ; ++i) {
       if (MPI_PROC_NULL != srcs[i]) {
-        res = NBC_Sched_recv ((char *) rbuf + i * rcount * rcvext, true, rcount, rtype, srcs[i], schedule, false);
+        res = NBC_Sched_recv ((char *) rbuf + (MPI_Aint) rcvext * i * rcount, true, rcount, rtype, srcs[i], schedule, false);
         if (OPAL_UNLIKELY(OMPI_SUCCESS != res)) {
           break;
         }
@@ -106,7 +106,7 @@ static int nbc_neighbor_alltoall_init(const void *sbuf, int scount, MPI_Datatype
 
     for (int i = 0 ; i < outdegree ; ++i) {
       if (MPI_PROC_NULL != dsts[i]) {
-        res = NBC_Sched_send ((char *) sbuf + i * scount * sndext, false, scount, stype, dsts[i], schedule, false);
+        res = NBC_Sched_send ((char *) sbuf + (MPI_Aint) sndext * i * scount, false, scount, stype, dsts[i], schedule, false);
         if (OPAL_UNLIKELY(OMPI_SUCCESS != res)) {
           break;
         }

ompi/mca/coll/libnbc/nbc_ireduce.c

@@ -29,7 +29,7 @@
 static inline int red_sched_binomial (int rank, int p, int root, const void *sendbuf, void *redbuf, char tmpredbuf, int count, MPI_Datatype datatype,
                                       MPI_Op op, char inplace, NBC_Schedule *schedule, void *tmpbuf);
 static inline int red_sched_chain (int rank, int p, int root, const void *sendbuf, void *recvbuf, int count, MPI_Datatype datatype,
-                                   MPI_Op op, int ext, size_t size, NBC_Schedule *schedule, void *tmpbuf, int fragsize);
+                                   MPI_Op op, MPI_Aint ext, size_t size, NBC_Schedule *schedule, void *tmpbuf, int fragsize);
 
 static inline int red_sched_linear (int rank, int rsize, int root, const void *sendbuf, void *recvbuf, void *tmpbuf, int count, MPI_Datatype datatype,
                                     MPI_Op op, NBC_Schedule *schedule);
@@ -459,7 +459,7 @@ static inline int red_sched_binomial (int rank, int p, int root, const void *sen
 
 /* chain send ... */
 static inline int red_sched_chain (int rank, int p, int root, const void *sendbuf, void *recvbuf, int count, MPI_Datatype datatype,
-                                   MPI_Op op, int ext, size_t size, NBC_Schedule *schedule, void *tmpbuf, int fragsize) {
+                                   MPI_Op op, MPI_Aint ext, size_t size, NBC_Schedule *schedule, void *tmpbuf, int fragsize) {
   int res, vrank, rpeer, speer, numfrag, fragcount, thiscount;
   long offset;
 
@@ -479,11 +479,11 @@ static inline int red_sched_chain (int rank, int p, int root, const void *sendbu
   fragcount = count / numfrag;
 
   for (int fragnum = 0 ; fragnum < numfrag ; ++fragnum) {
-    offset = fragnum * fragcount * ext;
+    offset = (MPI_Aint) ext * fragnum * fragcount;
     thiscount = fragcount;
     if(fragnum == numfrag - 1) {
       /* last fragment may not be full */
-      thiscount = count - fragcount * fragnum;
+      thiscount = count - (size_t)fragcount * fragnum;
     }
 
     /* last node does not recv */

ompi/mca/coll/libnbc/nbc_ireduce_scatter_block.c

@@ -70,7 +70,7 @@ static int nbc_reduce_scatter_block_init(const void* sendbuf, void* recvbuf, int
 
   maxr = ceil_of_log2(p);
 
-  count = p * recvcount;
+  count = (size_t) p * recvcount;
 
   if (0 < count) {
     char *rbuf, *lbuf, *buf;
@@ -248,7 +248,7 @@ static int nbc_reduce_scatter_block_inter_init(const void *sendbuf, void *recvbu
     return res;
   }
 
-  count = rcount * lsize;
+  count = (size_t)rcount * lsize;
 
   span = opal_datatype_span(&dtype->super, count, &gap);
   span_align = OPAL_ALIGN(span, dtype->super.align, ptrdiff_t);

ompi/mca/coll/libnbc/nbc_iscatter.c

@@ -99,7 +99,7 @@ static int nbc_scatter_init (const void* sendbuf, int sendcount, MPI_Datatype se
       }
     } else {
       for (int i = 0 ; i < p ; ++i) {
-        sbuf = (char *) sendbuf + i * sendcount * sndext;
+        sbuf = (char *) sendbuf + (MPI_Aint) sndext * i * sendcount;
         if (i == root) {
           if (!inplace) {
             /* if I am the root - just copy the message */
@@ -222,7 +222,7 @@ static int nbc_scatter_inter_init (const void* sendbuf, int sendcount, MPI_Datat
         }
     } else if (MPI_ROOT == root) {
         for (int i = 0 ; i < rsize ; ++i) {
-            sbuf = ((char *)sendbuf) + (i * sendcount * sndext);
+            sbuf = ((char *)sendbuf) + ((MPI_Aint) sndext * i * sendcount);
             /* root sends the right buffer to the right receiver */
             res = NBC_Sched_send(sbuf, false, sendcount, sendtype, i, schedule, false);
             if (OPAL_UNLIKELY(OMPI_SUCCESS != res)) {