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.

diff --git a/citadel/context.c b/citadel/context.c
index cc8885483be0f52a620425cc044921f81412f9c7..3122e5c06b102dd55cc63fc2ef996e25ab18e0f2 100644 (file)
--- 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 4547c9743365bfddf8b6d5a4dc1619860ee85924..5e7488794dccbedba8ba833555d7cea5977bced1 100644 (file)
--- 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 172a4ad0921c00da53e5e49bf86911a7880712bb..965cec625563650f19cd964248c2c8d5bc41eda3 100644 (file)
--- 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;
        }