lib/blob: do not assume realloc(NULL, 0) returns a not-NULL value

There is situation that num_extent_pages is zero and original pointer is
also NULL, the realloc() could return a Not NULL pointer.

Related UT has been added and updated.
1) In the default allocation (num_clusters == 0), the extent_pages is not allocated as expected.
2) In the thin provisioning allocation (num_clusters != 0), the extent_pages will be allocated if extent_table is used.

More related information as below:

The crux of the problem is that according to POSIX:

realloc: "If ptr is NULL, then the call is equivalent to malloc(size)"
malloc: "If size is 0, then malloc returns either NULL or a unique pointer value that can later be successfully passed to free"

blobstore was relying on realloc(NULL, 0) always return a unique pointer value, and not NULL.  This is not portable behavior.

Change-Id: Ibc28d9696f15a3c0e2aa6bb2371dc23576c28954
Signed-off-by: GangCao <gang.cao@intel.com>
Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/10470
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: Changpeng Liu <changpeng.liu@intel.com>
Reviewed-by: Jim Harris <james.r.harris@intel.com>

lib/blob/blobstore.c

@@ -669,22 +669,24 @@ blob_parse_page(const struct spdk_blob_md_page *page, struct spdk_blob *blob)
 				return -EINVAL;
 			}
 
-			blob->extent_table_found = true;
-
 			if (desc_extent_table->length == 0 ||
 			    (extent_pages_length % sizeof(desc_extent_table->extent_page[0]) != 0)) {
 				return -EINVAL;
 			}
 
+			blob->extent_table_found = true;
+
 			for (i = 0; i < extent_pages_length / sizeof(desc_extent_table->extent_page[0]); i++) {
 				num_extent_pages += desc_extent_table->extent_page[i].num_pages;
 			}
 
-			tmp = realloc(blob->active.extent_pages, num_extent_pages * sizeof(uint32_t));
+			if (num_extent_pages > 0) {
-			if (tmp == NULL) {
+				tmp = realloc(blob->active.extent_pages, num_extent_pages * sizeof(uint32_t));
-				return -ENOMEM;
+				if (tmp == NULL) {
+					return -ENOMEM;
+				}
+				blob->active.extent_pages = tmp;
 			}
-			blob->active.extent_pages = tmp;
 			blob->active.extent_pages_array_size = num_extent_pages;
 
 			blob->remaining_clusters_in_et = desc_extent_table->num_clusters;

test/unit/lib/blob/blob.c/blob_ut.c

@@ -442,6 +442,57 @@ blob_create(void)
 	CU_ASSERT(g_bserrno == -ENOSPC);
 }
 
+static void
+blob_create_zero_extent(void)
+{
+	struct spdk_blob_store *bs = g_bs;
+	struct spdk_blob *blob;
+	spdk_blob_id blobid;
+
+	/* Create blob with default options (opts == NULL) */
+	spdk_bs_create_blob_ext(bs, NULL, blob_op_with_id_complete, NULL);
+	poll_threads();
+	CU_ASSERT(g_bserrno == 0);
+	CU_ASSERT(g_blobid != SPDK_BLOBID_INVALID);
+	blobid = g_blobid;
+
+	spdk_bs_open_blob(bs, blobid, blob_op_with_handle_complete, NULL);
+	poll_threads();
+	CU_ASSERT(g_bserrno == 0);
+	SPDK_CU_ASSERT_FATAL(g_blob != NULL);
+	blob = g_blob;
+	CU_ASSERT(spdk_blob_get_num_clusters(blob) == 0);
+	CU_ASSERT(blob->extent_table_found == true);
+	CU_ASSERT(blob->active.extent_pages_array_size == 0);
+	CU_ASSERT(blob->active.extent_pages == NULL);
+
+	spdk_blob_close(blob, blob_op_complete, NULL);
+	poll_threads();
+	CU_ASSERT(g_bserrno == 0);
+
+	/* Create blob with NULL internal options  */
+	bs_create_blob(bs, NULL, NULL, blob_op_with_id_complete, NULL);
+	poll_threads();
+	CU_ASSERT(g_bserrno == 0);
+	CU_ASSERT(g_blobid != SPDK_BLOBID_INVALID);
+	blobid = g_blobid;
+
+	spdk_bs_open_blob(bs, blobid, blob_op_with_handle_complete, NULL);
+	poll_threads();
+	CU_ASSERT(g_bserrno == 0);
+	SPDK_CU_ASSERT_FATAL(g_blob != NULL);
+	blob = g_blob;
+	CU_ASSERT(TAILQ_FIRST(&blob->xattrs_internal) == NULL);
+	CU_ASSERT(spdk_blob_get_num_clusters(blob) == 0);
+	CU_ASSERT(blob->extent_table_found == true);
+	CU_ASSERT(blob->active.extent_pages_array_size == 0);
+	CU_ASSERT(blob->active.extent_pages == NULL);
+
+	spdk_blob_close(blob, blob_op_complete, NULL);
+	poll_threads();
+	CU_ASSERT(g_bserrno == 0);
+}
+
 /*
  * Create and delete one blob in a loop over and over again.  This helps ensure
  * that the internal bit masks tracking used clusters and md_pages are being
@@ -588,6 +639,7 @@ blob_create_internal(void)
 	CU_ASSERT(g_bserrno == 0);
 	SPDK_CU_ASSERT_FATAL(g_blob != NULL);
 	CU_ASSERT(TAILQ_FIRST(&g_blob->xattrs_internal) == NULL);
+	CU_ASSERT(spdk_blob_get_num_clusters(g_blob) == 0);
 
 	blob = g_blob;
 
@@ -627,6 +679,16 @@ blob_thin_provision(void)
 	blob = ut_blob_create_and_open(bs, &opts);
 	blobid = spdk_blob_get_id(blob);
 	CU_ASSERT(blob->invalid_flags & SPDK_BLOB_THIN_PROV);
+	/* In thin provisioning with num_clusters is set, if not using the
+	 * extent table, there is no allocation. If extent table is used,
+	 * there is related allocation happened. */
+	if (blob->extent_table_found == true) {
+		CU_ASSERT(blob->active.extent_pages_array_size > 0);
+		CU_ASSERT(blob->active.extent_pages != NULL);
+	} else {
+		CU_ASSERT(blob->active.extent_pages_array_size == 0);
+		CU_ASSERT(blob->active.extent_pages == NULL);
+	}
 
 	spdk_blob_close(blob, blob_op_complete, NULL);
 	CU_ASSERT(g_bserrno == 0);
@@ -7104,6 +7166,7 @@ int main(int argc, char **argv)
 	CU_ADD_TEST(suite_bs, blob_create_loop);
 	CU_ADD_TEST(suite_bs, blob_create_fail);
 	CU_ADD_TEST(suite_bs, blob_create_internal);
+	CU_ADD_TEST(suite_bs, blob_create_zero_extent);
 	CU_ADD_TEST(suite, blob_thin_provision);
 	CU_ADD_TEST(suite_bs, blob_snapshot);
 	CU_ADD_TEST(suite_bs, blob_clone);