volser: Fix broken bulk conversion

The converstions between the original, N and U bulk list return
values were all broken in various ways:

1/ Shifting from malloc to xdr_alloc() (in 4f1efdc8b73ed)
   subtly changed the behaviour when handling an empty list. The
   correct XDR representation of an empty list is {0, NULL}, not
   {0, &memZero}. Fix the code so that if the source list is empty,
   an empty destination list is returned.
2/ The destination list length was never being filled in. This means
   that xdr_free() could not be safely used on this list, as the wrong
   length would be passed to the allocator. Fill in the destination
   list length as part of the conversion.
3/ xdr_free(...) is a no-op when called with an empty list - there's
   no need to check before calling it. Remove these checks to improve
   the code's readability.
4/ xdr_free(...) should only be called when the RPC returned
   sucessfully. The stub is responsible for freeing data should the call
   fail mid way through unmarshalling.
5/ Where an RPC returns the number of entries independently of the
   length of a counted array, it is unsafe to use that length to
   iterate the array without checking that it is within the array
   bounds. Instead, just use the array length when performing
   conversions.

Change-Id: Ied9a77849062e5a1e506f13d4d90d116cefe41e4
Reviewed-on: http://gerrit.openafs.org/3596
Reviewed-by: Derrick Brashear <shadow@dementia.org>
Tested-by: BuildBot <buildbot@rampaginggeek.com>

diff --git a/src/volser/vsutils.c b/src/volser/vsutils.c
index b9f9ad2..447d39a 100644 (file)
--- a/src/volser/vsutils.c
+++ b/src/volser/vsutils.c
@@ -418,37 +418,49 @@ VLDB_ReplaceEntryU(afs_uint32 volid, afs_int32 voltype, struct uvldbentry *entry
 }
 
 static void
-convertBulkToNBulk(bulkentries *bulk, nbulkentries *nbulk, int *entriesp) {
+convertBulkToNBulk(bulkentries *bulk, nbulkentries *nbulk) {
     int i;
 
+    if (bulk->bulkentries_len == 0)
+       return;
+
+    nbulk->nbulkentries_len = bulk->bulkentries_len;
     nbulk->nbulkentries_val =
-       (nvldbentry *) xdr_alloc(*entriesp * sizeof(struct nvldbentry));
+       xdr_alloc(bulk->bulkentries_len * sizeof(struct nvldbentry));
 
-    for (i = 0; i < *entriesp; i++) {  /* process each entry */
+    for (i = 0; i < bulk->bulkentries_len; i++) {
        ovlentry_to_nvlentry(&bulk->bulkentries_val[i],
                             &nbulk->nbulkentries_val[i]);
     }
 }
 
 static void
-convertBulkToUBulk(bulkentries *bulk, ubulkentries *ubulk, int *entriesp) {
+convertBulkToUBulk(bulkentries *bulk, ubulkentries *ubulk) {
     int i;
 
+    if (bulk->bulkentries_len == 0)
+       return;
+
+    ubulk->ubulkentries_len = bulk->bulkentries_len;
     ubulk->ubulkentries_val =
-       (uvldbentry *) xdr_alloc(*entriesp * sizeof(struct uvldbentry));
-    for (i = 0; i < *entriesp; i++) {  /* process each entry */
+       xdr_alloc(bulk->bulkentries_len * sizeof(struct uvldbentry));
+    for (i = 0; i < bulk->bulkentries_len; i++) {
        ovlentry_to_uvlentry(&bulk->bulkentries_val[i],
                             &ubulk->ubulkentries_val[i]);
     }
 }
 
 static void
-convertNBulkToUBulk(nbulkentries *nbulk, ubulkentries *ubulk, int *entriesp) {
+convertNBulkToUBulk(nbulkentries *nbulk, ubulkentries *ubulk) {
     int i;
 
+    if (nbulk->nbulkentries_len == 0)
+       return;
+
+    ubulk->ubulkentries_len = nbulk->nbulkentries_len;
     ubulk->ubulkentries_val =
-        (uvldbentry *) xdr_alloc(*entriesp * sizeof(struct uvldbentry));
-    for (i = 0; i < *entriesp; i++) {  /* process each entry */
+        xdr_alloc(nbulk->nbulkentries_len * sizeof(struct uvldbentry));
+    for (i = 0; i < nbulk->nbulkentries_len; i++) {    /* process each entry */
         nvlentry_to_uvlentry(&nbulk->nbulkentries_val[i],
                              &ubulk->ubulkentries_val[i]);
     }
@@ -464,16 +476,17 @@ VLDB_ListAttributes(VldbListByAttributes *attrp,
     if (newvlserver == vltype_old) {
         bulkentries arrayEntries;
       tryold:
-       memset(&arrayEntries, 0, sizeof(arrayEntries)); /*initialize to hint the stub  to alloc space */
+       memset(&arrayEntries, 0, sizeof(arrayEntries));
        code =
            ubik_VL_ListAttributes(cstruct, 0, attrp, entriesp,
                      &arrayEntries);
 
-       if (!code)
-           convertBulkToNBulk(&arrayEntries, blkentriesp, entriesp);
+       if (code)
+           return code;
+
+       convertBulkToNBulk(&arrayEntries, blkentriesp);
 
-       if (arrayEntries.bulkentries_val)
-           xdr_free((xdrproc_t) xdr_bulkentries, &arrayEntries);
+       xdr_free((xdrproc_t) xdr_bulkentries, &arrayEntries);
        return code;
     }
 
@@ -497,31 +510,33 @@ VLDB_ListAttributesU(VldbListByAttributes *attrp,
     if (newvlserver == vltype_old) {
         bulkentries arrayEntries;
       tryold:
-       memset(&arrayEntries, 0, sizeof(arrayEntries)); /*initialize to hint the stub  to alloc space */
+       memset(&arrayEntries, 0, sizeof(arrayEntries));
        code =
            ubik_VL_ListAttributes(cstruct, 0, attrp, entriesp,
                      &arrayEntries);
-       if (!code)
-           convertBulkToUBulk(&arrayEntries, blkentriesp, entriesp);
+       if (code)
+           return code;
+
+       convertBulkToUBulk(&arrayEntries, blkentriesp);
 
-       if (arrayEntries.bulkentries_val)
-           xdr_free((xdrproc_t) xdr_bulkentries, &arrayEntries);
+       xdr_free((xdrproc_t) xdr_bulkentries, &arrayEntries);
        return code;
     }
 
-    memset(&narrayEntries, 0, sizeof(narrayEntries));  /*initialize to hint the stub  to alloc space */
+    memset(&narrayEntries, 0, sizeof(narrayEntries));
     code =
         ubik_VL_ListAttributesN(cstruct, 0, attrp, entriesp, &narrayEntries);
     if (code == RXGEN_OPCODE) {
         newvlserver = vltype_old;      /* Doesn't support new interface */
         goto tryold;
     }
-    if (!code) {
-       convertNBulkToUBulk(&narrayEntries, blkentriesp, entriesp);
 
-    }
-    if (narrayEntries.nbulkentries_val)
-        xdr_free((xdrproc_t) xdr_bulkentries, &narrayEntries);
+    if (code)
+       return code;
+
+    convertNBulkToUBulk(&narrayEntries, blkentriesp);
+
+    xdr_free((xdrproc_t) xdr_bulkentries, &narrayEntries);
     return code;
 }
 
@@ -560,11 +575,12 @@ VLDB_ListAttributesN2U(VldbListByAttributes *attrp,
         code =
             ubik_VL_ListAttributesN2(cstruct, 0, attrp, (name ? name : ""),
                                      thisindex, nentriesp, &narrayEntries, nextindexp);
-        if (!code)
-           convertNBulkToUBulk(&narrayEntries, blkentriesp, nentriesp);
+        if (code)
+           return code;
+
+       convertNBulkToUBulk(&narrayEntries, blkentriesp);
 
-       if (narrayEntries.nbulkentries_val)
-           xdr_free((xdrproc_t) xdr_bulkentries, &narrayEntries);
+       xdr_free((xdrproc_t) xdr_bulkentries, &narrayEntries);
        return code;
     }
     return code;