From be8d2e8c65b70b7caa227f794c3c849ef2367954 Mon Sep 17 00:00:00 2001
From: Dave West <davew@uncensored.citadel.org>
Date: Tue, 10 Nov 2009 19:06:33 +0000
Subject: [PATCH] Grabbed that previously noted bad code from rename_user that
 walked the context list without locking. Used it to create a function int
 CtdlIsUserLoggedIn (char *username) and exposed it to the API Oh yes and I
 fixed it to walk the list safely.

---
 citadel/context.c             | 24 ++++++++++++++++++++++++
 citadel/include/ctdl_module.h |  3 +++
 citadel/user_ops.c            | 20 ++++++--------------
 3 files changed, 33 insertions(+), 14 deletions(-)

diff --git a/citadel/context.c b/citadel/context.c
index cc8885483..3122e5c06 100644
--- a/citadel/context.c
+++ b/citadel/context.c
@@ -127,6 +127,30 @@ int CtdlIsSingleUser(void)
 }
 
 
+
+
+/*
+ * Check to see if a user is currently logged in
+ * Take care with what you do as a result of this test.
+ * The user may not have been logged in when this function was called BUT
+ * because of threading the user might be logged in before you test the result.
+ */
+int CtdlIsUserLoggedIn (char *user_name)
+{
+	CitContext *cptr;
+	int ret = 0;
+
+	begin_critical_section (S_SESSION_TABLE);
+	for (cptr = ContextList; cptr != NULL; cptr = cptr->next) {
+		if (!strcasecmp(cptr->user.fullname, user_name)) {
+			ret = 1;
+			break;
+		}
+	}
+	end_critical_section(S_SESSION_TABLE);
+	return ret;
+}
+
 /*
  * Return a pointer to the CitContext structure bound to the thread which
  * called this function.  If there's no such binding (for example, if it's
diff --git a/citadel/include/ctdl_module.h b/citadel/include/ctdl_module.h
index 4547c9743..5e7488794 100644
--- a/citadel/include/ctdl_module.h
+++ b/citadel/include/ctdl_module.h
@@ -189,6 +189,9 @@ int CtdlWantSingleUser(void);
 int CtdlIsSingleUser(void);
 
 
+int CtdlIsUserLoggedIn (char *user_name);
+
+
 /*
  * CtdlGetCurrentMessageNumber()  -  Obtain the current highest message number in the system
  * This provides a quick way to initialise a variable that might be used to indicate
diff --git a/citadel/user_ops.c b/citadel/user_ops.c
index 172a4ad09..965cec625 100644
--- a/citadel/user_ops.c
+++ b/citadel/user_ops.c
@@ -197,26 +197,12 @@ void lputuser(struct ctdluser *usbuf)
  *
  */
 int rename_user(char *oldname, char *newname) {
-	CitContext *cptr;
 	int retcode = RENAMEUSER_OK;
 	struct ctdluser usbuf;
 
 	char oldnamekey[USERNAME_SIZE];
 	char newnamekey[USERNAME_SIZE];
 
-	/* We cannot rename a user who is currently logged in */
-/* FIXME: This is very broken!!!!
- * We check that the user is not already logged in because we can't rename them
- * if they are logged in.
- * BUT THEN WE LEAVE A HUGE WINDOW FOR THEM TO LOG IN BEFORE WE LOCK TO RENAME THEM!!!!!
- * We are also traversing an un-locked context list which is a very bad thing to do.
- */
-	for (cptr = ContextList; cptr != NULL; cptr = cptr->next) {
-		if (!strcasecmp(cptr->user.fullname, oldname)) {
-			return(RENAMEUSER_LOGGED_IN);
-		}
-	}
-
 	/* Create the database keys... */
 	makeuserkey(oldnamekey, oldname);
 	makeuserkey(newnamekey, newname);
@@ -224,6 +210,12 @@ int rename_user(char *oldname, char *newname) {
 	/* Lock up and get going */
 	begin_critical_section(S_USERS);
 
+	/* We cannot rename a user who is currently logged in */
+	if (CtdlIsUserLoggedIn(oldname)) {
+		end_critical_section(S_USERS);
+		return RENAMEUSER_LOGGED_IN;
+	}
+
 	if (CtdlGetUser(&usbuf, newname) == 0) {
 		retcode = RENAMEUSER_ALREADY_EXISTS;
 	}
-- 
2.30.2