From be80a3f4d0ad691e9b210840c71848a36ee82a6d Mon Sep 17 00:00:00 2001
From: "Roger A. Light" <roger@atchoo.org>
Date: Wed, 6 Oct 2021 09:54:02 +0100
Subject: [PATCH] Fix client id not showing in log on failed connections, where
 possible.

---
 ChangeLog.txt                   |  1 +
 apps/db_dump/stubs.c            |  5 +++++
 lib/mosquitto_internal.h        |  2 +-
 src/bridge.c                    |  2 +-
 src/context.c                   | 13 +++++++++++--
 src/handle_connect.c            |  8 +-------
 src/mosquitto_broker_internal.h |  1 +
 src/persist_read.c              |  2 +-
 8 files changed, 22 insertions(+), 12 deletions(-)

diff --git a/ChangeLog.txt b/ChangeLog.txt
index 95bd285c..90e14207 100644
--- a/ChangeLog.txt
+++ b/ChangeLog.txt
@@ -9,6 +9,7 @@ Broker:
 - Fix problem parsing config files with Windows line endings. Closes #2297.
 - Don't send retained messages when a shared subscription is made.
 - Fix log being truncated in Windows.
+- Fix client id not showing in log on failed connections, where possible.
 
 Client library:
 - Initialise sockpairR/W to invalid in `mosquitto_reinitialise()` to avoid
diff --git a/apps/db_dump/stubs.c b/apps/db_dump/stubs.c
index e7b184c5..f3773a6d 100644
--- a/apps/db_dump/stubs.c
+++ b/apps/db_dump/stubs.c
@@ -17,6 +17,11 @@ struct mosquitto *context__init(mosq_sock_t sock)
 	return NULL;
 }
 
+void context__add_to_by_id(struct mosquitto *context)
+{
+	UNUSED(context);
+}
+
 int db__message_store(const struct mosquitto *source, struct mosquitto_msg_store *stored, uint32_t message_expiry_interval, dbid_t store_id, enum mosquitto_msg_origin origin)
 {
 	UNUSED(source);
diff --git a/lib/mosquitto_internal.h b/lib/mosquitto_internal.h
index b2e7a03e..e26ded7c 100644
--- a/lib/mosquitto_internal.h
+++ b/lib/mosquitto_internal.h
@@ -285,7 +285,7 @@ struct mosquitto {
 	time_t session_expiry_time;
 	uint32_t session_expiry_interval;
 #ifdef WITH_BROKER
-	bool removed_from_by_id; /* True if removed from by_id hash */
+	bool in_by_id;
 	bool is_dropping;
 	bool is_bridge;
 	struct mosquitto__bridge *bridge;
diff --git a/src/bridge.c b/src/bridge.c
index 7139dbf2..f6e77d32 100644
--- a/src/bridge.c
+++ b/src/bridge.c
@@ -92,7 +92,7 @@ int bridge__new(struct mosquitto__bridge *bridge)
 			return MOSQ_ERR_NOMEM;
 		}
 		new_context->id = local_id;
-		HASH_ADD_KEYPTR(hh_id, db.contexts_by_id, new_context->id, strlen(new_context->id), new_context);
+		context__add_to_by_id(new_context);
 	}
 	new_context->bridge = bridge;
 	new_context->is_bridge = true;
diff --git a/src/context.c b/src/context.c
index a0125eed..b2f851ea 100644
--- a/src/context.c
+++ b/src/context.c
@@ -281,16 +281,25 @@ void context__free_disused(void)
 }
 
 
+void context__add_to_by_id(struct mosquitto *context)
+{
+	if(context->in_by_id == false){
+		context->in_by_id = true;
+		HASH_ADD_KEYPTR(hh_id, db.contexts_by_id, context->id, strlen(context->id), context);
+	}
+}
+
+
 void context__remove_from_by_id(struct mosquitto *context)
 {
 	struct mosquitto *context_found;
 
-	if(context->removed_from_by_id == false && context->id){
+	if(context->in_by_id == true && context->id){
 		HASH_FIND(hh_id, db.contexts_by_id, context->id, strlen(context->id), context_found);
 		if(context_found){
 			HASH_DELETE(hh_id, db.contexts_by_id, context_found);
 		}
-		context->removed_from_by_id = true;
+		context->in_by_id = false;
 	}
 }
 
diff --git a/src/handle_connect.c b/src/handle_connect.c
index 0aa7fd33..944b9f1e 100644
--- a/src/handle_connect.c
+++ b/src/handle_connect.c
@@ -253,7 +253,7 @@ int connect__on_authorised(struct mosquitto *context, void *auth_data_out, uint1
 	connection_check_acl(context, &context->msgs_out.inflight);
 	connection_check_acl(context, &context->msgs_out.queued);
 
-	HASH_ADD_KEYPTR(hh_id, db.contexts_by_id, context->id, strlen(context->id), context);
+	context__add_to_by_id(context);
 
 #ifdef WITH_PERSISTENCE
 	if(!context->clean_start){
@@ -908,12 +908,6 @@ int handle__connect(struct mosquitto *context)
 #endif
 		{
 			rc = mosquitto_unpwd_check(context);
-			if(rc != MOSQ_ERR_SUCCESS){
-				/* We must have context->id == NULL here so we don't later try and
-				* remove the client from the by_id hash table */
-				mosquitto__free(context->id);
-				context->id = NULL;
-			}
 			switch(rc){
 				case MOSQ_ERR_SUCCESS:
 					break;
diff --git a/src/mosquitto_broker_internal.h b/src/mosquitto_broker_internal.h
index 7ca127dd..5d4f5de5 100644
--- a/src/mosquitto_broker_internal.h
+++ b/src/mosquitto_broker_internal.h
@@ -692,6 +692,7 @@ void context__disconnect(struct mosquitto *context);
 void context__add_to_disused(struct mosquitto *context);
 void context__free_disused(void);
 void context__send_will(struct mosquitto *context);
+void context__add_to_by_id(struct mosquitto *context);
 void context__remove_from_by_id(struct mosquitto *context);
 
 int connect__on_authorised(struct mosquitto *context, void *auth_data_out, uint16_t auth_data_out_len);
diff --git a/src/persist_read.c b/src/persist_read.c
index 3a3aeb37..5e7be454 100644
--- a/src/persist_read.c
+++ b/src/persist_read.c
@@ -64,7 +64,7 @@ static struct mosquitto *persist__find_or_add_context(const char *client_id, uin
 
 		context->clean_start = false;
 
-		HASH_ADD_KEYPTR(hh_id, db.contexts_by_id, context->id, strlen(context->id), context);
+		context__add_to_by_id(context);
 	}
 	if(last_mid){
 		context->last_mid = last_mid;