From aec5f0a3efa41b0102f361c0f66be55daddbf294 Mon Sep 17 00:00:00 2001
From: Robert Lubos <robert.lubos@nordicsemi.no>
Date: Mon, 20 Jul 2020 12:14:30 +0200
Subject: [PATCH] net: mqtt: Prevent double CONNACK event notification on
 server reject

Currently, the application could receive a duplicate CONNACK event, in
case the server rejected the connection at MQTT level (with an error
code provided with CONNACK message). A subsequent connection close (with
`mqtt_abort` for instance) would produce the duplicate event.

Fix this by reporting back to the MQTT engine, that the connection was
refused, so it can close the connection rightaway. Rework the event
notification logic, so that DISCONNECT event instead of a duplicate
CONNACK event is notified in that case.

Also, prevent the MQTT engine from notyfing DISCONNECT event in case of
socket errors during initial connection phase (i. e. before
`mqtt_connect` function finished).

Signed-off-by: Robert Lubos <robert.lubos@nordicsemi.no>
---
 subsys/net/lib/mqtt/mqtt.c    | 55 +++++++++++++----------------------
 subsys/net/lib/mqtt/mqtt_rx.c |  2 ++
 2 files changed, 23 insertions(+), 34 deletions(-)

diff --git a/subsys/net/lib/mqtt/mqtt.c b/subsys/net/lib/mqtt/mqtt.c
index 4cc7cd12e8e4..b12069b5c12a 100644
--- a/subsys/net/lib/mqtt/mqtt.c
+++ b/subsys/net/lib/mqtt/mqtt.c
@@ -35,31 +35,6 @@ static void tx_buf_init(struct mqtt_client *client, struct buf_ctx *buf)
 	buf->end = client->tx_buf + client->tx_buf_size;
 }
 
-/**@brief Notifies disconnection event to the application.
- *
- * @param[in] client Identifies the client for which the procedure is requested.
- * @param[in] result Reason for disconnection.
- */
-static void disconnect_event_notify(struct mqtt_client *client, int result)
-{
-	struct mqtt_evt evt;
-
-	/* Determine appropriate event to generate. */
-	if (MQTT_HAS_STATE(client, MQTT_STATE_CONNECTED)) {
-		evt.type = MQTT_EVT_DISCONNECT;
-		evt.result = result;
-	} else {
-		evt.type = MQTT_EVT_CONNACK;
-		evt.result = -ECONNREFUSED;
-	}
-
-	/* Notify application. */
-	event_notify(client, &evt);
-
-	/* Reset internal state. */
-	client_reset(client);
-}
-
 void event_notify(struct mqtt_client *client, const struct mqtt_evt *evt)
 {
 	if (client->evt_cb != NULL) {
@@ -71,7 +46,8 @@ void event_notify(struct mqtt_client *client, const struct mqtt_evt *evt)
 	}
 }
 
-static void client_disconnect(struct mqtt_client *client, int result)
+static void client_disconnect(struct mqtt_client *client, int result,
+			      bool notify)
 {
 	int err_code;
 
@@ -80,7 +56,18 @@ static void client_disconnect(struct mqtt_client *client, int result)
 		MQTT_ERR("Failed to disconnect transport!");
 	}
 
-	disconnect_event_notify(client, result);
+	if (notify) {
+		struct mqtt_evt evt = {
+			.type = MQTT_EVT_DISCONNECT,
+			.result = result,
+		};
+
+		/* Notify application. */
+		event_notify(client, &evt);
+	}
+
+	/* Reset internal state. */
+	client_reset(client);
 }
 
 static int client_connect(struct mqtt_client *client)
@@ -118,7 +105,7 @@ static int client_connect(struct mqtt_client *client)
 	return 0;
 
 error:
-	client_disconnect(client, err_code);
+	client_disconnect(client, err_code, false);
 	return err_code;
 }
 
@@ -132,7 +119,7 @@ static int client_read(struct mqtt_client *client)
 
 	err_code = mqtt_handle_rx(client);
 	if (err_code < 0) {
-		client_disconnect(client, err_code);
+		client_disconnect(client, err_code, true);
 	}
 
 	return err_code;
@@ -149,7 +136,7 @@ static int client_write(struct mqtt_client *client, const uint8_t *data,
 	if (err_code < 0) {
 		MQTT_TRC("Transport write failed, err_code = %d, "
 			 "closing connection", err_code);
-		client_disconnect(client, err_code);
+		client_disconnect(client, err_code, true);
 		return err_code;
 	}
 
@@ -170,7 +157,7 @@ static int client_write_msg(struct mqtt_client *client,
 	if (err_code < 0) {
 		MQTT_TRC("Transport write failed, err_code = %d, "
 			 "closing connection", err_code);
-		client_disconnect(client, err_code);
+		client_disconnect(client, err_code, true);
 		return err_code;
 	}
 
@@ -477,7 +464,7 @@ int mqtt_disconnect(struct mqtt_client *client)
 		goto error;
 	}
 
-	client_disconnect(client, 0);
+	client_disconnect(client, 0, true);
 
 error:
 	mqtt_mutex_unlock(client);
@@ -596,7 +583,7 @@ int mqtt_abort(struct mqtt_client *client)
 	NULL_PARAM_CHECK(client);
 
 	if (client->internal.state != MQTT_STATE_IDLE) {
-		client_disconnect(client, -ECONNABORTED);
+		client_disconnect(client, -ECONNABORTED, true);
 	}
 
 	mqtt_mutex_unlock(client);
@@ -698,7 +685,7 @@ static int read_publish_payload(struct mqtt_client *client, void *buffer,
 			ret = -ENOTCONN;
 		}
 
-		client_disconnect(client, ret);
+		client_disconnect(client, ret, true);
 		goto exit;
 	}
 
diff --git a/subsys/net/lib/mqtt/mqtt_rx.c b/subsys/net/lib/mqtt/mqtt_rx.c
index 5b582be756a5..687c4abb6a8b 100644
--- a/subsys/net/lib/mqtt/mqtt_rx.c
+++ b/subsys/net/lib/mqtt/mqtt_rx.c
@@ -42,6 +42,8 @@ static int mqtt_handle_packet(struct mqtt_client *client,
 						MQTT_CONNECTION_ACCEPTED) {
 				/* Set state. */
 				MQTT_SET_STATE(client, MQTT_STATE_CONNECTED);
+			} else {
+				err_code = -ECONNREFUSED;
 			}
 
 			evt.result = evt.param.connack.return_code;