Commit 10f32b9f authored by GangCao's avatar GangCao Committed by Jim Harris
Browse files

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: default avatarGangCao <gang.cao@intel.com>
Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/10470


Tested-by: default avatarSPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: default avatarChangpeng Liu <changpeng.liu@intel.com>
Reviewed-by: default avatarJim Harris <james.r.harris@intel.com>
parent a4d132cd

lib/blob/blobstore.c

+8 −6
Original line number Diff line number Diff line
@@ -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;

			}



			if (num_extent_pages > 0) {

				tmp = realloc(blob->active.extent_pages, num_extent_pages * sizeof(uint32_t));

				if (tmp == NULL) {

					return -ENOMEM;

				}

				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

+63 −0
Original line number Diff line number Diff line
@@ -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);