windows-integrated-logon-hack-fix-for-proper-refcounts-20060119

The Integrated Logon hack of setting a token for a smb name different
than the one associated with the current smb session fails when smb
virtual circuits, sessions and username objects are properly reference
counted.  When refcounts are not leaked the constructed smb_username_t
is destroyed immediately after the token is set since there are not
references to it from a current session.

The fix is to mark the smb_username_t object with a flag indicating that
it was created by the Network Provider.  This flag prevents the destruction
when the refcount is zero so that it will be available at the time the
smb session is created (just a moment or two later.)  During the binding
of the smb_username_t to the smb_vc_t the flag is cleared allowing the
tokens to be destroyed when the smb session is closed.

diff --git a/src/WINNT/afsd/afslogon.c b/src/WINNT/afsd/afslogon.c
index dbe35ef..bcd700f 100644 (file)
--- a/src/WINNT/afsd/afslogon.c
+++ b/src/WINNT/afsd/afslogon.c
@@ -801,7 +801,8 @@ DWORD APIENTRY NPLogonNotify(
         {                      
             if ( KFW_is_available() ) {
                 code = KFW_AFS_get_cred(uname, cell, password, 0, opt.smbName, &reason);
-                DebugEvent("KFW_AFS_get_cred  uname=[%s] smbname=[%s] cell=[%s] code=[%d]",uname,opt.smbName,cell,code);
+                DebugEvent("KFW_AFS_get_cred  uname=[%s] smbname=[%s] cell=[%s] code=[%d]",
+                           uname,opt.smbName,cell,code);
                 if (code == 0 && opt.theseCells) { 
                     char * principal, *p;
 
@@ -830,8 +831,8 @@ DWORD APIENTRY NPLogonNotify(
                 code = ka_UserAuthenticateGeneral2(KA_USERAUTH_VERSION+KA_USERAUTH_AUTHENT_LOGON,
                                                     uname, "", cell, password, opt.smbName, 0, &pw_exp, 0,
                                                     &reason);
-                DebugEvent("AFS AfsLogon - (INTEGRATED only)ka_UserAuthenticateGeneral2","Code[%x] uname[%s] Cell[%s]",
-                            code,uname,cell);
+               DebugEvent("AFS AfsLogon - (INTEGRATED only)ka_UserAuthenticateGeneral2 Code[%x] uname[%s] smbname=[%s] Cell[%s] PwExp=[%d] Reason=[%s]",
+                           code,uname,opt.smbName,cell,pw_exp,reason?reason:"");
             }       
             if ( code && code != KTC_NOCM && code != KTC_NOCMRPC && !lowercased_name ) {
                 for ( ctemp = uname; *ctemp ; ctemp++) {

diff --git a/src/WINNT/afsd/cm_ioctl.c b/src/WINNT/afsd/cm_ioctl.c
index f62b171..daf5802 100644 (file)
--- a/src/WINNT/afsd/cm_ioctl.c
+++ b/src/WINNT/afsd/cm_ioctl.c
@@ -1985,7 +1985,8 @@ long cm_IoctlSetToken(struct smb_ioctl *ioctlp, struct cm_user *userp)
     }
 
     if (flags & PIOCTL_LOGON) {
-        userp = smb_FindCMUserByName(smbname, ioctlp->fidp->vcp->rname);
+        userp = smb_FindCMUserByName(smbname, ioctlp->fidp->vcp->rname,
+                                    SMB_FLAG_CREATE|SMB_FLAG_AFSLOGON);
        release_userp = 1;
     }
 

diff --git a/src/WINNT/afsd/cm_user.c b/src/WINNT/afsd/cm_user.c
index 6cf8e96..54d3c25 100644 (file)
--- a/src/WINNT/afsd/cm_user.c
+++ b/src/WINNT/afsd/cm_user.c
@@ -39,14 +39,13 @@ void cm_InitUser(void)
 
 cm_user_t *cm_NewUser(void)
 {
-    cm_user_t *up;
+    cm_user_t *userp;
         
-    up = malloc(sizeof(*up));
-    memset(up, 0, sizeof(*up));
-    up->refCount = 1;
-    up->vcRefs = 1;            /* from caller */
-    lock_InitializeMutex(&up->mx, "cm_user_t");
-    return up;
+    userp = malloc(sizeof(*userp));
+    memset(userp, 0, sizeof(*userp));
+    userp->refCount = 1;
+    lock_InitializeMutex(&userp->mx, "cm_user_t");
+    return userp;
 }
 
 /* must be called with locked userp */
@@ -98,31 +97,39 @@ void cm_HoldUser(cm_user_t *up)
     lock_ReleaseWrite(&cm_userLock);
 }
 
-void cm_ReleaseUser(cm_user_t *up)
+void cm_ReleaseUser(cm_user_t *userp)
 {
     cm_ucell_t *ucp;
     cm_ucell_t *ncp;
 
-    if (up == NULL) 
+    if (userp == NULL) 
         return;
 
     lock_ObtainWrite(&cm_userLock);
-    osi_assert(up->refCount-- > 0);
-    if (up->refCount == 0) {
-        lock_FinalizeMutex(&up->mx);
-        for (ucp = up->cellInfop; ucp; ucp = ncp) {
+    osi_assert(userp->refCount-- > 0);
+    if (userp->refCount == 0) {
+        lock_FinalizeMutex(&userp->mx);
+        for (ucp = userp->cellInfop; ucp; ucp = ncp) {
             ncp = ucp->nextp;
             if (ucp->ticketp) 
                 free(ucp->ticketp);
             free(ucp);
         }
-        free(up);
+        free(userp);
     }
     lock_ReleaseWrite(&cm_userLock);
 }
 
+
+void cm_HoldUserVCRef(cm_user_t *userp)
+{
+    lock_ObtainMutex(&userp->mx);
+    userp->vcRefs++;
+    lock_ReleaseMutex(&userp->mx);
+}       
+
 /* release the count of the # of connections that use this user structure.
- * When this hits zero, we know we won't be getting an new requests from
+ * When this hits zero, we know we won't be getting any new requests from
  * this user, and thus we can start GC'ing connections.  Ref count on user
  * won't hit zero until all cm_conn_t's have been GC'd, since they hold
  * refCount references to userp.

diff --git a/src/WINNT/afsd/cm_user.h b/src/WINNT/afsd/cm_user.h
index 48ea5ba..f9f28b5 100644 (file)
--- a/src/WINNT/afsd/cm_user.h
+++ b/src/WINNT/afsd/cm_user.h
@@ -48,7 +48,6 @@ typedef struct cm_user {
 } cm_user_t;
 
 #define CM_USERFLAG_DELETE     1       /* delete on last reference */
-#define CM_USERFLAG_WASLOGON   2       /* was logon DLL user */
 
 extern void cm_InitUser(void);
 
@@ -60,6 +59,8 @@ extern cm_ucell_t *cm_FindUCell(cm_user_t *userp, int iterator);
 
 extern void cm_HoldUser(cm_user_t *up);
 
+extern void cm_HoldUserVCRef(cm_user_t *up);
+
 extern void cm_ReleaseUser(cm_user_t *up);
 
 extern void cm_ReleaseUserVCRef(cm_user_t *up);

diff --git a/src/WINNT/afsd/smb.c b/src/WINNT/afsd/smb.c
index a5cfe6b..c13a6e5 100644 (file)
--- a/src/WINNT/afsd/smb.c
+++ b/src/WINNT/afsd/smb.c
@@ -935,6 +935,8 @@ int smb_IsStarMask(char *maskp)
 
 void smb_ReleaseVCInternal(smb_vc_t *vcp)
 {
+    smb_vc_t **vcpp;
+
 #ifdef DEBUG
     osi_assert(vcp->refCount-- != 0);
 #else
@@ -942,6 +944,14 @@ void smb_ReleaseVCInternal(smb_vc_t *vcp)
 #endif
 
     if (vcp->refCount == 0) {
+       /* remove VCP from smb_deadVCsp */
+       for (vcpp = &smb_deadVCsp; *vcpp; vcpp = &((*vcpp)->nextp)) {
+           if (*vcpp == vcp) {
+               *vcpp = vcp->nextp;
+               break;
+           }
+       } 
+
        memset(vcp,0,sizeof(smb_vc_t));
        free(vcp);
     }
@@ -985,9 +995,9 @@ void smb_CleanupDeadVC(smb_vc_t *vcp)
     smb_tid_t *tidpNext;
     smb_tid_t *tidp;
     unsigned short tid;
-    smb_user_t *userpIter;
-    smb_user_t *userpNext;
-    smb_user_t *userp;
+    smb_user_t *uidpIter;
+    smb_user_t *uidpNext;
+    smb_user_t *uidp;
     unsigned short uid;
     smb_vc_t **vcpp;
 
@@ -1034,24 +1044,24 @@ void smb_CleanupDeadVC(smb_vc_t *vcp)
        lock_ObtainRead(&smb_rctLock);
     }
 
-    for (userpIter = vcp->usersp; userpIter; userpIter = userpNext) {
-       userpNext = userpIter->nextp;
+    for (uidpIter = vcp->usersp; uidpIter; uidpIter = uidpNext) {
+       uidpNext = uidpIter->nextp;
 
-       if (userpIter->flags & SMB_USERFLAG_DELETE)
+       if (uidpIter->flags & SMB_USERFLAG_DELETE)
            continue;
 
-       uid = userpIter->userID;
-       osi_Log2(smb_logp, "  Cleanup UID %d (userp=0x%x)", uid, userpIter);
+       uid = uidpIter->userID;
+       osi_Log2(smb_logp, "  Cleanup UID %d (uidp=0x%x)", uid, uidpIter);
        lock_ReleaseRead(&smb_rctLock);
 
-       userp = smb_FindUID(vcp, uid, 0);
-       osi_assert(userp);
+       uidp = smb_FindUID(vcp, uid, 0);
+       osi_assert(uidp);
 
-       lock_ObtainMutex(&userp->mx);
-       userp->flags |= SMB_USERFLAG_DELETE;
-       lock_ReleaseMutex(&userp->mx);
+       lock_ObtainMutex(&uidp->mx);
+       uidp->flags |= SMB_USERFLAG_DELETE;
+       lock_ReleaseMutex(&uidp->mx);
 
-       smb_ReleaseUID(userp);
+       smb_ReleaseUID(uidp);
 
        lock_ObtainRead(&smb_rctLock);
     }
@@ -1060,6 +1070,12 @@ void smb_CleanupDeadVC(smb_vc_t *vcp)
     for (vcpp = &smb_allVCsp; *vcpp; vcpp = &((*vcpp)->nextp)) {
        if (*vcpp == vcp) {
            *vcpp = vcp->nextp;
+           vcp->nextp = smb_deadVCsp;
+           smb_deadVCsp = vcp;
+           /* We intentionally do not keep a reference to the 
+            * vcp once it is placed on the deadVCsp list.  This
+            * allows the refcount to reach 0 so we can delete
+            * it. */
            smb_ReleaseVCNoLock(vcp);
            break;
        }
@@ -1154,7 +1170,7 @@ smb_user_t *smb_FindUID(smb_vc_t *vcp, unsigned short uid, int flags)
     return uidp;
 }              
 
-smb_username_t *smb_FindUserByName(char *usern, char *machine, int flags)
+smb_username_t *smb_FindUserByName(char *usern, char *machine, afs_uint32 flags)
 {
     smb_username_t *unp= NULL;
 
@@ -1175,7 +1191,10 @@ smb_username_t *smb_FindUserByName(char *usern, char *machine, int flags)
         unp->machine = strdup(machine);
         usernamesp = unp;
         lock_InitializeMutex(&unp->mx, "username_t mutex");
+       if (flags & SMB_FLAG_AFSLOGON)
+           unp->flags = SMB_USERFLAG_AFSLOGON;
     }
+
     lock_ReleaseWrite(&smb_rctLock);
     return unp;
 }      
@@ -1208,7 +1227,7 @@ void smb_ReleaseUsername(smb_username_t *unp)
 
     lock_ObtainWrite(&smb_rctLock);
     osi_assert(unp->refCount-- > 0);
-    if (unp->refCount == 0) {
+    if (unp->refCount == 0 && !(unp->flags & SMB_USERFLAG_AFSLOGON)) {
         lupp = &usernamesp;
         for(up = *lupp; up; lupp = &up->nextp, up = *lupp) {
             if (up == unp) 
@@ -1225,7 +1244,6 @@ void smb_ReleaseUsername(smb_username_t *unp)
     lock_ReleaseWrite(&smb_rctLock);
 
     if (userp) {
-        cm_ReleaseUserVCRef(userp);
         cm_ReleaseUser(userp);
     }  
 }      
@@ -1253,8 +1271,11 @@ void smb_ReleaseUID(smb_user_t *uidp)
     }          
     lock_ReleaseWrite(&smb_rctLock);
 
-    if (unp)
+    if (unp) {
+       if (unp->userp)
+           cm_ReleaseUserVCRef(unp->userp);
        smb_ReleaseUsername(unp);
+    }
 }      
 
 /* retrieve a held reference to a user structure corresponding to an incoming
@@ -8754,6 +8775,36 @@ int smb_DumpVCP(FILE *outputFile, char *cookie, int lock)
     sprintf(output, "done dumping smb_vc_t\n");
     WriteFile(outputFile, output, (DWORD)strlen(output), &zilch, NULL);
   
+    sprintf(output, "begin dumping DEAD smb_vc_t\n");
+    WriteFile(outputFile, output, (DWORD)strlen(output), &zilch, NULL);
+
+    for (vcp = smb_deadVCsp; vcp; vcp=vcp->nextp) 
+    {
+        smb_fid_t *fidp;
+      
+        sprintf(output, "%s vcp=0x%p, refCount=%d, flags=%d, vcID=%d, lsn=%d, uidCounter=%d, tidCounter=%d, fidCounter=%d\n",
+                 cookie, vcp, vcp->refCount, vcp->flags, vcp->vcID, vcp->lsn, vcp->uidCounter, vcp->tidCounter, vcp->fidCounter);
+        WriteFile(outputFile, output, (DWORD)strlen(output), &zilch, NULL);
+      
+        sprintf(output, "begin dumping smb_fid_t\n");
+        WriteFile(outputFile, output, (DWORD)strlen(output), &zilch, NULL);
+
+        for (fidp = vcp->fidsp; fidp; fidp = (smb_fid_t *) osi_QNext(&fidp->q))
+        {
+            sprintf(output, "%s -- smb_fidp=0x%p, refCount=%d, fid=%d, vcp=0x%p, scp=0x%p, ioctlp=0x%p, NTopen_pathp=%s, NTopen_wholepathp=%s\n", 
+                     cookie, fidp, fidp->refCount, fidp->fid, fidp->vcp, fidp->scp, fidp->ioctlp, 
+                     fidp->NTopen_pathp ? fidp->NTopen_pathp : "NULL", 
+                     fidp->NTopen_wholepathp ? fidp->NTopen_wholepathp : "NULL");
+            WriteFile(outputFile, output, (DWORD)strlen(output), &zilch, NULL);
+        }
+      
+        sprintf(output, "done dumping smb_fid_t\n");
+        WriteFile(outputFile, output, (DWORD)strlen(output), &zilch, NULL);
+    }
+
+    sprintf(output, "done dumping DEAD smb_vc_t\n");
+    WriteFile(outputFile, output, (DWORD)strlen(output), &zilch, NULL);
+  
     if (lock)
         lock_ReleaseRead(&smb_rctLock);
     return 0;

diff --git a/src/WINNT/afsd/smb.h b/src/WINNT/afsd/smb.h
index fbfdda8..e2002a0 100644 (file)
--- a/src/WINNT/afsd/smb.h
+++ b/src/WINNT/afsd/smb.h
@@ -91,6 +91,7 @@ typedef struct smb {
 
 /* flags for functions */
 #define SMB_FLAG_CREATE                1       /* create the structure if necessary */
+#define SMB_FLAG_AFSLOGON       2       /* operating on behalf of afslogon.dll */
 
 /* max # of bytes we'll receive in an incoming SMB message */
 /* the maximum is 2^18-1 for NBT and 2^25-1 for Raw transport messages */
@@ -246,6 +247,7 @@ typedef struct smb_username {
 } smb_username_t;
 
 #define SMB_USERFLAG_DELETE    1       /* delete struct when ref count zero */
+#define SMB_USERFLAG_AFSLOGON   2       /* do not delete when the refCount reaches zero */
 
 #define SMB_MAX_USERNAME_LENGTH 256
 
@@ -499,14 +501,10 @@ extern void smb_ReleaseTID(smb_tid_t *tidp);
 
 extern smb_user_t *smb_FindUID(smb_vc_t *vcp, unsigned short uid, int flags);
 
-extern smb_username_t *smb_FindUserByName(char *usern, char *machine, int flags);
+extern smb_username_t *smb_FindUserByName(char *usern, char *machine, afs_uint32 flags);
 
 extern smb_user_t *smb_FindUserByNameThisSession(smb_vc_t *vcp, char *usern); 
 
-extern smb_username_t *smb_FindUserByName(char *usern, char *machine, int flags);
-
-extern smb_user_t *smb_FindUserByNameThisSession(smb_vc_t *vcp, char *usern);
-
 extern void smb_ReleaseUsername(smb_username_t *unp);
 
 extern void smb_ReleaseUID(smb_user_t *uidp);

diff --git a/src/WINNT/afsd/smb3.c b/src/WINNT/afsd/smb3.c
index 003c3ef..7849285 100644 (file)
--- a/src/WINNT/afsd/smb3.c
+++ b/src/WINNT/afsd/smb3.c
@@ -662,7 +662,6 @@ long smb_ReceiveV3SessionSetupX(smb_vc_t *vcp, smb_packet_t *inp, smb_packet_t *
     smb_user_t *uidp;
     unsigned short newUid;
     unsigned long caps = 0;
-    cm_user_t *userp;
     smb_username_t *unp;
     char *s1 = " ";
     long code = 0; 
@@ -842,20 +841,31 @@ long smb_ReceiveV3SessionSetupX(smb_vc_t *vcp, smb_packet_t *inp, smb_packet_t *
     uidp = smb_FindUserByNameThisSession(vcp, usern);
     if (uidp) {   /* already there, so don't create a new one */
         unp = uidp->unp;
-        userp = unp->userp;
         newUid = uidp->userID;
-        osi_Log3(smb_logp,"smb_ReceiveV3SessionSetupX FindUserByName:Lana[%d],lsn[%d],userid[%d]",vcp->lana,vcp->lsn,newUid);
+        osi_Log3(smb_logp,"smb_ReceiveV3SessionSetupX FindUserByName:Lana[%d],lsn[%d],userid[%d]",
+                vcp->lana,vcp->lsn,newUid);
         smb_ReleaseUID(uidp);
     }
     else {
-      /* do a global search for the username/machine name pair */
+       cm_user_t *userp;
+
+       /* do a global search for the username/machine name pair */
         unp = smb_FindUserByName(usern, vcp->rname, SMB_FLAG_CREATE);
+       lock_ObtainMutex(&unp->mx);
+       if (unp->flags & SMB_USERFLAG_AFSLOGON) {
+           /* clear the afslogon flag so that the tickets can now 
+            * be freed when the refCount returns to zero.
+            */
+           unp->flags &= ~SMB_USERFLAG_AFSLOGON;
+       }
+       lock_ReleaseMutex(&unp->mx);
 
         /* Create a new UID and cm_user_t structure */
         userp = unp->userp;
         if (!userp)
             userp = cm_NewUser();
-        lock_ObtainMutex(&vcp->mx);
+       cm_HoldUserVCRef(userp);
+       lock_ObtainMutex(&vcp->mx);
         if (!vcp->uidCounter)
             vcp->uidCounter++; /* handle unlikely wraparounds */
         newUid = (strlen(usern)==0)?0:vcp->uidCounter++;
@@ -7149,12 +7159,12 @@ void smb3_Init()
     lock_InitializeMutex(&smb_Dir_Watch_Lock, "Directory Watch List Lock");
 }
 
-cm_user_t *smb_FindCMUserByName(char *usern, char *machine)
+cm_user_t *smb_FindCMUserByName(char *usern, char *machine, afs_uint32 flags)
 {
     smb_username_t *unp;
     cm_user_t *     userp;
 
-    unp = smb_FindUserByName(usern, machine, SMB_FLAG_CREATE);
+    unp = smb_FindUserByName(usern, machine, flags);
     if (!unp->userp) {
         lock_ObtainMutex(&unp->mx);
         unp->userp = cm_NewUser();

diff --git a/src/WINNT/afsd/smb3.h b/src/WINNT/afsd/smb3.h
index c9b5ac4..62a9c92 100644 (file)
--- a/src/WINNT/afsd/smb3.h
+++ b/src/WINNT/afsd/smb3.h
@@ -192,7 +192,7 @@ extern long smb_ReceiveNTRename(smb_vc_t *vcp, smb_packet_t *inp, smb_packet_t *
 extern int smb_V3MatchMask(char *namep, char *maskp, int flags);
 
 extern void smb3_Init();
-extern cm_user_t *smb_FindCMUserByName(/*smb_vc_t *vcp,*/ char *usern, char *machine);
+extern cm_user_t *smb_FindCMUserByName(char *usern, char *machine, afs_uint32 flags);
 
 /* SMB auth related functions */
 extern void smb_NegotiateExtendedSecurity(void ** secBlob, int * secBlobLength);