From cc3ddbff420cf2513b52efa6f465ef9442d8ad52 Mon Sep 17 00:00:00 2001 From: Bart Van Assche Date: Sun, 3 Nov 2019 14:59:11 -0800 Subject: [PATCH] Improve robustness of the code that restores .queue_pdu() In my tests I hit a stack overflow caused by infinite recursion of .queue_pdu() calls, caused by .queue_pdu() not being restored in an error path. Make the approach for restoring .queue_pdu() more robust. Signed-off-by: Bart Van Assche --- test-tool/iscsi-test-cu.c | 4 ++++ test-tool/iscsi-test-cu.h | 2 ++ .../test_compareandwrite_invalid_dataout_size.c | 7 +------ test-tool/test_iscsi_chap.c | 8 +------- test-tool/test_iscsi_cmdsn_toohigh.c | 7 +------ test-tool/test_iscsi_cmdsn_toolow.c | 7 +------ test-tool/test_iscsi_datasn_invalid.c | 15 ++++++--------- test-tool/test_sanitize_block_erase_reserved.c | 7 +------ test-tool/test_sanitize_crypto_erase_reserved.c | 7 +------ test-tool/test_sanitize_overwrite_reserved.c | 7 +------ 10 files changed, 19 insertions(+), 52 deletions(-) diff --git a/test-tool/iscsi-test-cu.c b/test-tool/iscsi-test-cu.c index 7a795e4..7a94c59 100644 --- a/test-tool/iscsi-test-cu.c +++ b/test-tool/iscsi-test-cu.c @@ -730,6 +730,7 @@ static struct test_family families[] = { */ struct scsi_task *task; unsigned char *read_write_buf; +int (*orig_queue_pdu)(struct iscsi_context *iscsi, struct iscsi_pdu *pdu); static void print_usage(void) @@ -825,11 +826,14 @@ test_setup(void) { task = NULL; read_write_buf = NULL; + orig_queue_pdu = sd->iscsi_ctx ? sd->iscsi_ctx->drv->queue_pdu : NULL; } void test_teardown(void) { + if (sd->iscsi_ctx) + sd->iscsi_ctx->drv->queue_pdu = orig_queue_pdu; free(read_write_buf); read_write_buf = NULL; scsi_free_scsi_task(task); diff --git a/test-tool/iscsi-test-cu.h b/test-tool/iscsi-test-cu.h index 2cf26a5..4951080 100644 --- a/test-tool/iscsi-test-cu.h +++ b/test-tool/iscsi-test-cu.h @@ -35,6 +35,8 @@ /* globals between setup, tests, and teardown */ extern struct scsi_task *task; extern unsigned char *read_write_buf; +extern int (*orig_queue_pdu)(struct iscsi_context *iscsi, + struct iscsi_pdu *pdu); #ifndef HAVE_CU_SUITEINFO_PSETUPFUNC /* libcunit version 1 */ diff --git a/test-tool/test_compareandwrite_invalid_dataout_size.c b/test-tool/test_compareandwrite_invalid_dataout_size.c index fbb32a5..99cf8ed 100644 --- a/test-tool/test_compareandwrite_invalid_dataout_size.c +++ b/test-tool/test_compareandwrite_invalid_dataout_size.c @@ -27,7 +27,6 @@ static int new_tl; -static struct iscsi_transport iscsi_drv_orig; static int my_iscsi_queue_pdu(struct iscsi_context *iscsi, struct iscsi_pdu *pdu) { @@ -45,7 +44,7 @@ static int my_iscsi_queue_pdu(struct iscsi_context *iscsi, struct iscsi_pdu *pdu break; } out: - return iscsi_drv_orig.queue_pdu(iscsi, pdu); + return orig_queue_pdu(iscsi, pdu); } void @@ -58,7 +57,6 @@ test_compareandwrite_invalid_dataout_size(void) CHECK_FOR_ISCSI(sd); /* override transport queue_pdu callback for PDU manipulation */ - iscsi_drv_orig = *sd->iscsi_ctx->drv; sd->iscsi_ctx->drv->queue_pdu = my_iscsi_queue_pdu; logging(LOG_VERBOSE, LOG_BLANK_LINE); @@ -86,7 +84,4 @@ test_compareandwrite_invalid_dataout_size(void) scratch, 4 * block_size, block_size, 0, 0, 0, 0, EXPECT_STATUS_GENERIC_BAD); - - /* restore transport callbacks */ - *(sd->iscsi_ctx->drv) = iscsi_drv_orig; } diff --git a/test-tool/test_iscsi_chap.c b/test-tool/test_iscsi_chap.c index ebefc1e..4708963 100644 --- a/test-tool/test_iscsi_chap.c +++ b/test-tool/test_iscsi_chap.c @@ -29,8 +29,6 @@ #include "scsi-lowlevel.h" #include "iscsi-test-cu.h" -static struct iscsi_transport iscsi_drv_orig; - static int test_iscsi_strip_tag(struct iscsi_context *iscsi, struct iscsi_pdu *pdu, const char *tag) @@ -103,10 +101,8 @@ chap_mod_strip_replace_queue(struct iscsi_context *iscsi, struct iscsi_pdu *pdu, return ret; } logging(LOG_VERBOSE, "replaced Login PDU CHAP_A setting with %s", new_chap_a); - /* restore drv */ - *iscsi->drv = iscsi_drv_orig; out: - return iscsi_drv_orig.queue_pdu(iscsi, pdu); + return orig_queue_pdu(iscsi, pdu); } @@ -157,7 +153,6 @@ test_iscsi_chap_login(int (*test_queue_pdu)(struct iscsi_context *iscsi, iscsi_url->passwd); /* override transport queue_pdu callback for PDU manipulation */ - iscsi_drv_orig = *iscsi->drv; iscsi->drv->queue_pdu = test_queue_pdu; ret = iscsi_full_connect_sync(iscsi, iscsi_url->portal, iscsi_url->lun); @@ -169,7 +164,6 @@ test_iscsi_chap_login(int (*test_queue_pdu)(struct iscsi_context *iscsi, ret = 0; err_url_destroy: iscsi_destroy_url(iscsi_url); - /* XXX no need to restore iscsi_drv_orig */ err_iscsi_destroy: iscsi_destroy_context(iscsi); return ret; diff --git a/test-tool/test_iscsi_cmdsn_toohigh.c b/test-tool/test_iscsi_cmdsn_toohigh.c index 175f015..e87171f 100644 --- a/test-tool/test_iscsi_cmdsn_toohigh.c +++ b/test-tool/test_iscsi_cmdsn_toohigh.c @@ -25,7 +25,6 @@ #include "iscsi-test-cu.h" static int change_cmdsn; -static struct iscsi_transport iscsi_drv_orig; static int my_iscsi_queue_pdu(struct iscsi_context *iscsi, struct iscsi_pdu *pdu) { @@ -44,7 +43,7 @@ static int my_iscsi_queue_pdu(struct iscsi_context *iscsi, struct iscsi_pdu *pdu } change_cmdsn = 0; - return iscsi_drv_orig.queue_pdu(iscsi, pdu); + return orig_queue_pdu(iscsi, pdu); } void test_iscsi_cmdsn_toohigh(void) @@ -63,7 +62,6 @@ void test_iscsi_cmdsn_toohigh(void) sd->iscsi_ctx->use_immediate_data = ISCSI_IMMEDIATE_DATA_NO; sd->iscsi_ctx->target_max_recv_data_segment_length = block_size; /* override transport queue_pdu callback for PDU manipulation */ - iscsi_drv_orig = *sd->iscsi_ctx->drv; sd->iscsi_ctx->drv->queue_pdu = my_iscsi_queue_pdu; change_cmdsn = 1; /* we don't want autoreconnect since some targets will incorrectly @@ -85,7 +83,4 @@ void test_iscsi_cmdsn_toohigh(void) logging(LOG_VERBOSE, "Send a TESTUNITREADY with CMDSN == EXPCMDSN. should work again"); TESTUNITREADY(sd, EXPECT_STATUS_GOOD); - - /* restore transport callbacks */ - *(sd->iscsi_ctx->drv) = iscsi_drv_orig; } diff --git a/test-tool/test_iscsi_cmdsn_toolow.c b/test-tool/test_iscsi_cmdsn_toolow.c index 803e429..83032ed 100644 --- a/test-tool/test_iscsi_cmdsn_toolow.c +++ b/test-tool/test_iscsi_cmdsn_toolow.c @@ -25,7 +25,6 @@ #include "iscsi-test-cu.h" static int change_cmdsn; -static struct iscsi_transport iscsi_drv_orig; static int my_iscsi_queue_pdu(struct iscsi_context *iscsi, struct iscsi_pdu *pdu) { @@ -44,7 +43,7 @@ static int my_iscsi_queue_pdu(struct iscsi_context *iscsi, struct iscsi_pdu *pdu } change_cmdsn = 0; - return iscsi_drv_orig.queue_pdu(iscsi, pdu); + return orig_queue_pdu(iscsi, pdu); } void test_iscsi_cmdsn_toolow(void) @@ -63,7 +62,6 @@ void test_iscsi_cmdsn_toolow(void) sd->iscsi_ctx->use_immediate_data = ISCSI_IMMEDIATE_DATA_NO; sd->iscsi_ctx->target_max_recv_data_segment_length = block_size; /* override transport queue_pdu callback for PDU manipulation */ - iscsi_drv_orig = *sd->iscsi_ctx->drv; sd->iscsi_ctx->drv->queue_pdu = my_iscsi_queue_pdu; change_cmdsn = 1; /* we don't want autoreconnect since some targets will incorrectly @@ -85,7 +83,4 @@ void test_iscsi_cmdsn_toolow(void) logging(LOG_VERBOSE, "Send a TESTUNITREADY with CMDSN == EXPCMDSN. should work again"); TESTUNITREADY(sd, EXPECT_STATUS_GOOD); - - /* restore transport callbacks */ - *(sd->iscsi_ctx->drv) = iscsi_drv_orig; } diff --git a/test-tool/test_iscsi_datasn_invalid.c b/test-tool/test_iscsi_datasn_invalid.c index 3a04b3c..9afaf50 100644 --- a/test-tool/test_iscsi_datasn_invalid.c +++ b/test-tool/test_iscsi_datasn_invalid.c @@ -25,12 +25,15 @@ #include "iscsi-test-cu.h" static int change_datasn; -static struct iscsi_transport iscsi_drv_orig; static int my_iscsi_queue_pdu(struct iscsi_context *iscsi, struct iscsi_pdu *pdu) { uint32_t datasn; + /* avoid changing DataSN during reconnect */ + if (!iscsi->no_auto_reconnect) + goto out; + if (pdu->outdata.data[0] != ISCSI_PDU_DATA_OUT) { goto out; } @@ -54,7 +57,7 @@ static int my_iscsi_queue_pdu(struct iscsi_context *iscsi, struct iscsi_pdu *pdu break; } out: - return iscsi_drv_orig.queue_pdu(iscsi, pdu); + return orig_queue_pdu(iscsi, pdu); } void test_iscsi_datasn_invalid(void) @@ -73,7 +76,6 @@ void test_iscsi_datasn_invalid(void) sd->iscsi_ctx->use_immediate_data = ISCSI_IMMEDIATE_DATA_NO; sd->iscsi_ctx->target_max_recv_data_segment_length = block_size; /* override transport queue_pdu callback for PDU manipulation */ - iscsi_drv_orig = *sd->iscsi_ctx->drv; sd->iscsi_ctx->drv->queue_pdu = my_iscsi_queue_pdu; iscsi_set_noautoreconnect(sd->iscsi_ctx, 1); iscsi_set_timeout(sd->iscsi_ctx, 3); @@ -90,8 +92,6 @@ void test_iscsi_datasn_invalid(void) } CU_ASSERT_NOT_EQUAL(ret, 0); - /* avoid changing DataSN during reconnect */ - *(sd->iscsi_ctx->drv) = iscsi_drv_orig; iscsi_set_noautoreconnect(sd->iscsi_ctx, 0); logging(LOG_VERBOSE, "Send Data-Out PDU with DataSN==27. Should fail"); @@ -108,7 +108,6 @@ void test_iscsi_datasn_invalid(void) EXPECT_STATUS_GOOD); CU_ASSERT_NOT_EQUAL(ret, 0); - *(sd->iscsi_ctx->drv) = iscsi_drv_orig; iscsi_set_noautoreconnect(sd->iscsi_ctx, 0); logging(LOG_VERBOSE, "Send Data-Out PDU with DataSN==-1. Should fail"); @@ -125,7 +124,6 @@ void test_iscsi_datasn_invalid(void) EXPECT_STATUS_GOOD); CU_ASSERT_NOT_EQUAL(ret, 0); - *(sd->iscsi_ctx->drv) = iscsi_drv_orig; iscsi_set_noautoreconnect(sd->iscsi_ctx, 0); logging(LOG_VERBOSE, "Send Data-Out PDU's in reverse order (DataSN == 1,0). Should fail"); @@ -142,7 +140,6 @@ void test_iscsi_datasn_invalid(void) EXPECT_STATUS_GOOD); CU_ASSERT_NOT_EQUAL(ret, 0); out_ctx_restore: - /* restore transport callbacks and autoreconnect */ - *(sd->iscsi_ctx->drv) = iscsi_drv_orig; + /* restore autoreconnect */ iscsi_set_noautoreconnect(sd->iscsi_ctx, 0); } diff --git a/test-tool/test_sanitize_block_erase_reserved.c b/test-tool/test_sanitize_block_erase_reserved.c index 0b11c22..43a7c0c 100644 --- a/test-tool/test_sanitize_block_erase_reserved.c +++ b/test-tool/test_sanitize_block_erase_reserved.c @@ -25,7 +25,6 @@ #include "iscsi-test-cu.h" static int change_num; -static struct iscsi_transport iscsi_drv_orig; static int my_iscsi_queue_pdu(struct iscsi_context *iscsi, struct iscsi_pdu *pdu) { @@ -45,7 +44,7 @@ static int my_iscsi_queue_pdu(struct iscsi_context *iscsi, struct iscsi_pdu *pdu } change_num = 0; - return iscsi_drv_orig.queue_pdu(iscsi, pdu); + return orig_queue_pdu(iscsi, pdu); } void test_sanitize_block_erase_reserved(void) @@ -60,7 +59,6 @@ void test_sanitize_block_erase_reserved(void) CHECK_FOR_ISCSI(sd); /* override transport queue_pdu callback for PDU manipulation */ - iscsi_drv_orig = *sd->iscsi_ctx->drv; sd->iscsi_ctx->drv->queue_pdu = my_iscsi_queue_pdu; logging(LOG_VERBOSE, "Send SANITIZE command with the reserved " @@ -77,7 +75,4 @@ void test_sanitize_block_erase_reserved(void) SANITIZE(sd, 0, 0, SCSI_SANITIZE_BLOCK_ERASE, 0, NULL, EXPECT_INVALID_FIELD_IN_CDB); } - - /* restore transport callbacks */ - *(sd->iscsi_ctx->drv) = iscsi_drv_orig; } diff --git a/test-tool/test_sanitize_crypto_erase_reserved.c b/test-tool/test_sanitize_crypto_erase_reserved.c index 2621b79..7b71019 100644 --- a/test-tool/test_sanitize_crypto_erase_reserved.c +++ b/test-tool/test_sanitize_crypto_erase_reserved.c @@ -25,7 +25,6 @@ #include "iscsi-test-cu.h" static int change_num; -static struct iscsi_transport iscsi_drv_orig; static int my_iscsi_queue_pdu(struct iscsi_context *iscsi, struct iscsi_pdu *pdu) { @@ -45,7 +44,7 @@ static int my_iscsi_queue_pdu(struct iscsi_context *iscsi, struct iscsi_pdu *pdu } change_num = 0; - return iscsi_drv_orig.queue_pdu(iscsi, pdu); + return orig_queue_pdu(iscsi, pdu); } void test_sanitize_crypto_erase_reserved(void) @@ -60,7 +59,6 @@ void test_sanitize_crypto_erase_reserved(void) CHECK_FOR_ISCSI(sd); /* override transport queue_pdu callback for PDU manipulation */ - iscsi_drv_orig = *sd->iscsi_ctx->drv; sd->iscsi_ctx->drv->queue_pdu = my_iscsi_queue_pdu; logging(LOG_VERBOSE, "Send SANITIZE command with the reserved " @@ -77,7 +75,4 @@ void test_sanitize_crypto_erase_reserved(void) SANITIZE(sd, 0, 0, SCSI_SANITIZE_CRYPTO_ERASE, 0, NULL, EXPECT_INVALID_FIELD_IN_CDB); } - - /* restore transport callbacks */ - *(sd->iscsi_ctx->drv) = iscsi_drv_orig; } diff --git a/test-tool/test_sanitize_overwrite_reserved.c b/test-tool/test_sanitize_overwrite_reserved.c index 3df740a..c45a6f5 100644 --- a/test-tool/test_sanitize_overwrite_reserved.c +++ b/test-tool/test_sanitize_overwrite_reserved.c @@ -26,7 +26,6 @@ #include "iscsi-test-cu.h" static int change_num; -static struct iscsi_transport iscsi_drv_orig; static int my_iscsi_queue_pdu(struct iscsi_context *iscsi, struct iscsi_pdu *pdu) { @@ -46,7 +45,7 @@ static int my_iscsi_queue_pdu(struct iscsi_context *iscsi, struct iscsi_pdu *pdu } change_num = 0; - return iscsi_drv_orig.queue_pdu(iscsi, pdu); + return orig_queue_pdu(iscsi, pdu); } void test_sanitize_overwrite_reserved(void) @@ -71,7 +70,6 @@ void test_sanitize_overwrite_reserved(void) CHECK_FOR_ISCSI(sd); /* override transport queue_pdu callback for PDU manipulation */ - iscsi_drv_orig = *sd->iscsi_ctx->drv; sd->iscsi_ctx->drv->queue_pdu = my_iscsi_queue_pdu; logging(LOG_VERBOSE, "Send SANITIZE command with the reserved " @@ -88,7 +86,4 @@ void test_sanitize_overwrite_reserved(void) SANITIZE(sd, 0, 0, SCSI_SANITIZE_OVERWRITE, data.size, &data, EXPECT_INVALID_FIELD_IN_CDB); } - - /* restore transport callbacks */ - *(sd->iscsi_ctx->drv) = iscsi_drv_orig; }