From 22bb66ca73f98a7786aac79f1600b374e927addc Mon Sep 17 00:00:00 2001 From: Simon Wilkinson Date: Sun, 26 Dec 2010 14:14:38 +0000 Subject: [PATCH] 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. Reviewed-on: http://gerrit.openafs.org/3596 Reviewed-by: Derrick Brashear Tested-by: BuildBot (cherry picked from commit 02a2f6005042b9370350bdc03d4aab83355b205d) Change-Id: I1d3ec3929a8974d571e9b3712e53db4fceab4879 Reviewed-on: http://gerrit.openafs.org/3635 Tested-by: BuildBot Reviewed-by: Derrick Brashear Tested-by: Derrick Brashear --- src/volser/vsutils.c | 74 +++++++++++++++++++++++++++----------------- 1 file changed, 45 insertions(+), 29 deletions(-) diff --git a/src/volser/vsutils.c b/src/volser/vsutils.c index 6782d6e80..e3066827d 100644 --- a/src/volser/vsutils.c +++ b/src/volser/vsutils.c @@ -417,37 +417,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]); } @@ -463,16 +475,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; } @@ -496,31 +509,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; } @@ -559,11 +574,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; -- 2.39.5