From: Sage Weil <sage@redhat.com>
Date: Wed, 9 Aug 2017 21:25:12 +0000 (-0400)
Subject: crush/builder: fix ENOENT when removing last bucket item
X-Git-Tag: v13.0.0~109^2~3
X-Git-Url: http://git.apps.os.sepia.ceph.com/?a=commitdiff_plain;h=5a982050c1daddee33bb2b5d63a5936723283765;p=ceph.git

crush/builder: fix ENOENT when removing last bucket item

We were decrementing size and then breaking out ENOENT condition check.
Fix by decrementing size only after we break out of the loop and verify
we found the item.

Fix a follow-on bug by avoiding realloc when we have 0 items left.  This case
was never exercised before due to the ENOENT issue; now we return explicitly.
It's really not necessary to realloc at all, probably, since these are very
small arrays, but in any case leaving a single item allocation there in place of
a 0-length allocation is fine.  (And the 0-length allocation behvaior on realloc
is undefined.. may either return a pointer or NULL.)

Signed-off-by: Sage Weil <sage@redhat.com>
---

diff --git a/src/crush/builder.c b/src/crush/builder.c
index 3b4ba25b277b4..f492a27d3819b 100644
--- a/src/crush/builder.c
+++ b/src/crush/builder.c
@@ -1030,12 +1030,11 @@ int crush_remove_straw_bucket_item(struct crush_map *map,
 
 	for (i = 0; i < bucket->h.size; i++) {
 		if (bucket->h.items[i] == item) {
-			bucket->h.size--;
 			if (bucket->item_weights[i] < bucket->h.weight)
 				bucket->h.weight -= bucket->item_weights[i];
 			else
 				bucket->h.weight = 0;
-			for (j = i; j < bucket->h.size; j++) {
+			for (j = i; j < bucket->h.size - 1; j++) {
 				bucket->h.items[j] = bucket->h.items[j+1];
 				bucket->item_weights[j] = bucket->item_weights[j+1];
 			}
@@ -1044,7 +1043,11 @@ int crush_remove_straw_bucket_item(struct crush_map *map,
 	}
 	if (i == bucket->h.size)
 		return -ENOENT;
-	
+	bucket->h.size--;
+	if (bucket->h.size == 0) {
+		/* don't bother reallocating */
+		return 0;
+	}
 	void *_realloc = NULL;
 
 	if ((_realloc = realloc(bucket->h.items, sizeof(__s32)*newsize)) == NULL) {
@@ -1074,12 +1077,11 @@ int crush_remove_straw2_bucket_item(struct crush_map *map,
 
 	for (i = 0; i < bucket->h.size; i++) {
 		if (bucket->h.items[i] == item) {
-			bucket->h.size--;
 			if (bucket->item_weights[i] < bucket->h.weight)
 				bucket->h.weight -= bucket->item_weights[i];
 			else
 				bucket->h.weight = 0;
-			for (j = i; j < bucket->h.size; j++) {
+			for (j = i; j < bucket->h.size - 1; j++) {
 				bucket->h.items[j] = bucket->h.items[j+1];
 				bucket->item_weights[j] = bucket->item_weights[j+1];
 			}
@@ -1089,6 +1091,12 @@ int crush_remove_straw2_bucket_item(struct crush_map *map,
 	if (i == bucket->h.size)
 		return -ENOENT;
 
+	bucket->h.size--;
+	if (!newsize) {
+		/* don't bother reallocating a 0-length array. */
+		return 0;
+	}
+
 	void *_realloc = NULL;
 
 	if ((_realloc = realloc(bucket->h.items, sizeof(__s32)*newsize)) == NULL) {