Commit 2fb672af authored by Krzysztof Karas's avatar Krzysztof Karas Committed by Tomasz Zawadzki
Browse files

lib/jsonrpc: Store pointers to outstanding requests. 



Currently a request is only added to a list in jsonrpc
connection structure (stored inside jsonrpc server
data structure), when its response is ready to send.
This means that until that point, we do not have its
pointer available inside jsonrpc server data, so if
a server is shut down, it cannot properly handle that
request. Furthermore, when server's memory is freed
and connection is closed before such a request is sent,
`jsonrpc_server_send_response()` will still try to insert
the request into connection queue, resulting in
heap-use-after-free errors. To remedy that issue, this
patch introduces a new list for outstanding requests and
skips sending responses in case a connection is not
available.

Fixes #3052

Change-Id: I5ea6510d7cae5560654dbe2c18782e38eaa9fe97
Signed-off-by: default avatarKrzysztof Karas <krzysztof.karas@intel.com>
Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/19001


Tested-by: default avatarSPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: default avatarJim Harris <jim.harris@gmail.com>
Community-CI: Mellanox Build Bot
Reviewed-by: default avatarKonrad Sztyber <konrad.sztyber@intel.com>
parent a22656f5

lib/jsonrpc/jsonrpc_internal.h

+4 −0
Original line number Diff line number Diff line
@@ -55,6 +55,10 @@ struct spdk_jsonrpc_server_conn { 


	pthread_spinlock_t queue_lock;

	STAILQ_HEAD(, spdk_jsonrpc_request) send_queue;

	/* List of incomplete requests that are not yet ready to be sent.

	 * This is a safety net for cases, where server shutdown is called

	 * before all requests are placed into send_queue. */

	STAILQ_HEAD(, spdk_jsonrpc_request) outstanding_queue;



	struct spdk_jsonrpc_request *send_request;

lib/jsonrpc/jsonrpc_server.c

+19 −1
Original line number Diff line number Diff line
@@ -186,7 +186,10 @@ jsonrpc_parse_request(struct spdk_jsonrpc_server_conn *conn, const void *json, s 
		return -1;

	}



	pthread_spin_lock(&conn->queue_lock);

	conn->outstanding_requests++;

	STAILQ_INSERT_TAIL(&conn->outstanding_queue, request, link);

	pthread_spin_unlock(&conn->queue_lock);



	request->conn = conn;
@@ -315,6 +318,9 @@ end_response(struct spdk_jsonrpc_request *request) 
void

jsonrpc_free_request(struct spdk_jsonrpc_request *request)

{

	struct spdk_jsonrpc_request *req;

	struct spdk_jsonrpc_server_conn *conn;



	if (!request) {

		return;

	}
@@ -322,7 +328,19 @@ jsonrpc_free_request(struct spdk_jsonrpc_request *request) 
	/* We must send or skip response explicitly */

	assert(request->response == NULL);



	request->conn->outstanding_requests--;

	conn = request->conn;

	if (conn != NULL) {

		pthread_spin_lock(&conn->queue_lock);

		conn->outstanding_requests--;

		STAILQ_FOREACH(req, &conn->outstanding_queue, link) {

			if (req == request) {

				STAILQ_REMOVE(&conn->outstanding_queue,

					      req, spdk_jsonrpc_request, link);

				break;

			}

		}

		pthread_spin_unlock(&conn->queue_lock);

	}

	free(request->recv_buffer);

	free(request->values);

	free(request->send_buf);

lib/jsonrpc/jsonrpc_server_tcp.c

+18 −0
Original line number Diff line number Diff line
@@ -85,6 +85,15 @@ jsonrpc_server_free_conn_request(struct spdk_jsonrpc_server_conn *conn) 


	jsonrpc_free_request(conn->send_request);

	conn->send_request = NULL ;



	pthread_spin_lock(&conn->queue_lock);

	/* There might still be some requests being processed.

	 * We need to tell them that this connection is closed. */

	STAILQ_FOREACH(request, &conn->outstanding_queue, link) {

		request->conn = NULL;

	}

	pthread_spin_unlock(&conn->queue_lock);



	while ((request = jsonrpc_server_dequeue_request(conn)) != NULL) {

		jsonrpc_free_request(request);

	}
@@ -186,6 +195,7 @@ jsonrpc_server_accept(struct spdk_jsonrpc_server *server) 
		conn->recv_len = 0;

		conn->outstanding_requests = 0;

		STAILQ_INIT(&conn->send_queue);

		STAILQ_INIT(&conn->outstanding_queue);

		conn->send_request = NULL;



		if (pthread_spin_init(&conn->queue_lock, PTHREAD_PROCESS_PRIVATE)) {
@@ -308,8 +318,16 @@ jsonrpc_server_send_response(struct spdk_jsonrpc_request *request) 
{

	struct spdk_jsonrpc_server_conn *conn = request->conn;



	if (conn == NULL) {

		/* We cannot respond to the request, because the connection is closed. */

		SPDK_WARNLOG("Unable to send response: connection closed.\n");

		jsonrpc_free_request(request);

		return;

	}



	/* Queue the response to be sent */

	pthread_spin_lock(&conn->queue_lock);

	STAILQ_REMOVE(&conn->outstanding_queue, request, spdk_jsonrpc_request, link);

	STAILQ_INSERT_TAIL(&conn->send_queue, request, link);

	pthread_spin_unlock(&conn->queue_lock);

}

test/unit/lib/jsonrpc/jsonrpc_server.c/jsonrpc_server_ut.c

+4 −0
Original line number Diff line number Diff line
@@ -173,6 +173,8 @@ test_parse_request(void) 


	conn = calloc(1, sizeof(*conn));

	SPDK_CU_ASSERT_FATAL(conn != NULL);

	pthread_spin_init(&conn->queue_lock, PTHREAD_PROCESS_PRIVATE);

	STAILQ_INIT(&conn->outstanding_queue);



	conn->server = server;
@@ -310,6 +312,8 @@ test_parse_request_streaming(void) 


	conn = calloc(1, sizeof(*conn));

	SPDK_CU_ASSERT_FATAL(conn != NULL);

	pthread_spin_init(&conn->queue_lock, PTHREAD_PROCESS_PRIVATE);

	STAILQ_INIT(&conn->outstanding_queue);



	conn->server = server;