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 dbe35ef42ee159d55c6e78dc743f36ae692febd4..bcd700f5918eb2a4a83b98a44cc95a24702fc147 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 f62b171a8325eb5e086c1c6248f674770f03bec1..daf58027112a6379744f1c1fd2b4e37c489a24f6 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 6cf8e969a31067a19f517a854b6461b4c68002c4..54d3c25bd72177af7347c76949ddc246a2996d4f 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 48ea5ba8412873aa6ef0772090441deac8963a9a..f9f28b5b2533a6f50eade9be3cb2a11ce9b10b84 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 a5cfe6b5ee0e0947c72bfb03ebd7794ade8f884a..c13a6e5d33c0a91456b67485f24376a573e83db8 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 fbfdda86bc8a0c7b168131b0b3e6f3f84efa8ac5..e2002a0b57d5e6c2cf1d2e7c8ab28de231baaf61 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 003c3ef8ed2c4fbfb6254b100aa8211b0714d367..784928524f8ab2ec1eaedd1a0b4e438e784ae86a 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 c9b5ac443c13e03d36c2c8f1a38dd69d72db4b2d..62a9c92697f17a56b83a4dfd8ea58c0da64aa3f8 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);