From d9d798f78617026349e3c087c714e474e9eb2b7f Mon Sep 17 00:00:00 2001 From: Jeffrey Altman Date: Sun, 12 Feb 2006 06:25:37 +0000 Subject: [PATCH] windows-off-to-the-races-20060211 several race conditions were introduced over the last couple of weeks. let's fix them. --- src/WINNT/afsd/cm_cell.c | 21 ++++++------- src/WINNT/afsd/smb.c | 64 +++++++++++++++++++++------------------- src/WINNT/afsd/smb.h | 14 +++++---- 3 files changed, 50 insertions(+), 49 deletions(-) diff --git a/src/WINNT/afsd/cm_cell.c b/src/WINNT/afsd/cm_cell.c index a23e3b140..b74c1a8d8 100644 --- a/src/WINNT/afsd/cm_cell.c +++ b/src/WINNT/afsd/cm_cell.c @@ -61,7 +61,7 @@ long cm_AddCellProc(void *rockp, struct sockaddr_in *addrp, char *namep) */ cm_cell_t *cm_UpdateCell(cm_cell_t * cp) { - long code; + long code = 0; if (cp == NULL) return NULL; @@ -84,8 +84,8 @@ cm_cell_t *cm_UpdateCell(cm_cell_t * cp) } code = cm_SearchCellFile(cp->name, NULL, cm_AddCellProc, cp); - if (code) { #ifdef AFS_AFSDB_ENV + if (code) { if (cm_dnsEnabled) { int ttl; @@ -97,24 +97,21 @@ cm_cell_t *cm_UpdateCell(cm_cell_t * cp) #ifdef DEBUG fprintf(stderr, "cell %s: ttl=%d\n", cp->name, ttl); #endif - } else { + } else { /* if we fail to find it this time, we'll just do nothing and leave the - * current entry alone - */ + * current entry alone + */ cp->flags |= CM_CELLFLAG_VLSERVER_INVALID; - cp = NULL; /* return NULL to indicate failure */ } - } else + } + } else #endif /* AFS_AFSDB_ENV */ - { - cp = NULL; /* return NULL to indicate failure */ - } - } else { + { cp->timeout = time(0) + 7200; } } lock_ReleaseMutex(&cp->mx); - return cp; + return code ? NULL : cp; } /* load up a cell structure from the cell database, afsdcell.ini */ diff --git a/src/WINNT/afsd/smb.c b/src/WINNT/afsd/smb.c index 1d0d502a4..9d04c4afb 100644 --- a/src/WINNT/afsd/smb.c +++ b/src/WINNT/afsd/smb.c @@ -1003,7 +1003,7 @@ void smb_CleanupDeadVC(smb_vc_t *vcp) for (fidpIter = vcp->fidsp; fidpIter; fidpIter = fidpNext) { fidpNext = (smb_fid_t *) osi_QNext(&fidpIter->q); - if (fidpIter->flags & SMB_FID_DELETE) + if (fidpIter->delete) continue; fid = fidpIter->fid; @@ -1012,7 +1012,6 @@ void smb_CleanupDeadVC(smb_vc_t *vcp) smb_HoldFIDNoLock(fidpIter); lock_ReleaseWrite(&smb_rctLock); - /* smb_CloseFID sets SMB_FID_DELETE on Success */ smb_CloseFID(vcp, fidpIter, NULL, 0); smb_ReleaseFID(fidpIter); @@ -1022,9 +1021,9 @@ void smb_CleanupDeadVC(smb_vc_t *vcp) for (tidpIter = vcp->tidsp; tidpIter; tidpIter = tidpNext) { tidpNext = tidpIter->nextp; - - if (tidpIter->flags & SMB_TIDFLAG_DELETE) + if (tidpIter->delete) continue; + tidpIter->delete = 1; tid = tidpIter->tid; osi_Log2(smb_logp, " Cleanup TID %d (tidp=0x%x)", tid, tidpIter); @@ -1032,10 +1031,6 @@ void smb_CleanupDeadVC(smb_vc_t *vcp) smb_HoldTIDNoLock(tidpIter); lock_ReleaseWrite(&smb_rctLock); - lock_ObtainMutex(&tidpIter->mx); - tidpIter->flags |= SMB_TIDFLAG_DELETE; - lock_ReleaseMutex(&tidpIter->mx); - smb_ReleaseTID(tidpIter); lock_ObtainWrite(&smb_rctLock); @@ -1044,18 +1039,14 @@ void smb_CleanupDeadVC(smb_vc_t *vcp) for (uidpIter = vcp->usersp; uidpIter; uidpIter = uidpNext) { uidpNext = uidpIter->nextp; - - if (uidpIter->flags & SMB_USERFLAG_DELETE) + if (uidpIter->delete) continue; + uidpIter->delete = 1; /* do not add an additional reference count for the smb_user_t * as the smb_vc_t already is holding a reference */ lock_ReleaseWrite(&smb_rctLock); - lock_ObtainMutex(&uidpIter->mx); - uidpIter->flags |= SMB_USERFLAG_DELETE; - lock_ReleaseMutex(&uidpIter->mx); - smb_ReleaseUID(uidpIter); lock_ObtainWrite(&smb_rctLock); @@ -1120,7 +1111,7 @@ void smb_ReleaseTID(smb_tid_t *tidp) userp = NULL; lock_ObtainWrite(&smb_rctLock); osi_assert(tidp->refCount-- > 0); - if (tidp->refCount == 0 && (tidp->flags & SMB_TIDFLAG_DELETE)) { + if (tidp->refCount == 0 && (tidp->delete)) { ltpp = &tidp->vcp->tidsp; for(tp = *ltpp; tp; ltpp = &tp->nextp, tp = *ltpp) { if (tp == tidp) @@ -1447,7 +1438,7 @@ void smb_ReleaseFID(smb_fid_t *fidp) lock_ObtainWrite(&smb_rctLock); osi_assert(fidp->refCount-- > 0); lock_ObtainMutex(&fidp->mx); - if (fidp->refCount == 0 && (fidp->flags & SMB_FID_DELETE)) { + if (fidp->refCount == 0 && (fidp->delete)) { vcp = fidp->vcp; fidp->vcp = NULL; scp = fidp->scp; /* release after lock is released */ @@ -1455,8 +1446,8 @@ void smb_ReleaseFID(smb_fid_t *fidp) userp = fidp->userp; fidp->userp = NULL; - if (vcp->fidsp) - osi_QRemove((osi_queue_t **) &vcp->fidsp, &fidp->q); + if (vcp->fidsp) + osi_QRemove((osi_queue_t **) &vcp->fidsp, &fidp->q); thrd_CloseHandle(fidp->raw_write_event); /* and see if there is ioctl stuff to free */ @@ -1470,7 +1461,6 @@ void smb_ReleaseFID(smb_fid_t *fidp) free(ioctlp->outAllocp); free(ioctlp); } - lock_ReleaseMutex(&fidp->mx); lock_FinalizeMutex(&fidp->mx); free(fidp); @@ -3245,15 +3235,21 @@ long smb_ReceiveNegotiate(smb_vc_t *vcp, smb_packet_t *inp, smb_packet_t *outp) void smb_CheckVCs(void) { - smb_vc_t * vcp; + smb_vc_t * vcp, *nextp; smb_packet_t * outp = GetPacket(); smb_t *smbp; - for ( vcp=smb_allVCsp; vcp; vcp = vcp->nextp ) + lock_ObtainWrite(&smb_rctLock); + for ( vcp=smb_allVCsp, nextp=NULL; vcp; vcp = nextp ) { + nextp = vcp->nextp; + if (vcp->flags & SMB_VCFLAG_ALREADYDEAD) continue; + smb_HoldVCNoLock(vcp); + if (nextp) + smb_HoldVCNoLock(nextp); smb_FormatResponsePacket(vcp, NULL, outp); smbp = (smb_t *)outp; outp->inCom = smbp->com = 0x2b /* Echo */; @@ -3266,10 +3262,16 @@ void smb_CheckVCs(void) smb_SetSMBParm(outp, 0, 0); smb_SetSMBDataLength(outp, 0); + lock_ReleaseWrite(&smb_rctLock); smb_SendPacket(vcp, outp); - } + lock_ObtainWrite(&smb_rctLock); + smb_ReleaseVCNoLock(vcp); + if (nextp) + smb_ReleaseVCNoLock(nextp); + } + lock_ReleaseWrite(&smb_rctLock); smb_FreePacket(outp); } @@ -4723,9 +4725,9 @@ long smb_ReceiveCoreTreeDisconnect(smb_vc_t *vcp, smb_packet_t *inp, smb_packet_ /* find the tree and free it */ tidp = smb_FindTID(vcp, ((smb_t *)inp)->tid, 0); if (tidp) { - lock_ObtainMutex(&tidp->mx); - tidp->flags |= SMB_TIDFLAG_DELETE; - lock_ReleaseMutex(&tidp->mx); + lock_ObtainWrite(&smb_rctLock); + tidp->delete = 1; + lock_ReleaseWrite(&smb_rctLock); smb_ReleaseTID(tidp); } @@ -5687,16 +5689,16 @@ long smb_CloseFID(smb_vc_t *vcp, smb_fid_t *fidp, cm_user_t *userp, cm_InitReq(&req); - lock_ObtainMutex(&fidp->mx); - - if (fidp->flags & SMB_FID_DELETE) { + lock_ObtainWrite(&smb_rctLock); + if (fidp->delete) { osi_Log0(smb_logp, " Fid already closed."); - lock_ReleaseMutex(&fidp->mx); + lock_ReleaseWrite(&smb_rctLock); return CM_ERROR_BADFD; } + fidp->delete = 1; + lock_ReleaseWrite(&smb_rctLock); - fidp->flags |= SMB_FID_DELETE; - + lock_ObtainMutex(&fidp->mx); /* Don't jump the gun on an async raw write */ while (fidp->raw_writers) { lock_ReleaseMutex(&fidp->mx); diff --git a/src/WINNT/afsd/smb.h b/src/WINNT/afsd/smb.h index 5afe761d6..d2ebb5e3c 100644 --- a/src/WINNT/afsd/smb.h +++ b/src/WINNT/afsd/smb.h @@ -227,11 +227,12 @@ typedef struct smb_vc { typedef struct smb_user { struct smb_user *nextp; /* next sibling */ unsigned long refCount; /* ref count */ - long flags; /* flags; locked by mx */ + afs_uint32 flags; /* flags; locked by mx */ osi_mutex_t mx; unsigned short userID; /* the session identifier */ struct smb_vc *vcp; /* back ptr to virtual circuit */ struct smb_username *unp; /* user name struct */ + afs_uint32 delete; /* ok to del: locked by smb_rctLock */ } smb_user_t; #define SMB_USERFLAG_DELETE 1 /* delete struct when ref count zero */ @@ -270,17 +271,17 @@ typedef struct smb_username { typedef struct smb_tid { struct smb_tid *nextp; /* next sibling */ unsigned long refCount; - long flags; + afs_uint32 flags; /* protected by mx */ osi_mutex_t mx; /* for non-tree-related stuff */ unsigned short tid; /* the tid */ struct smb_vc *vcp; /* back ptr */ struct cm_user *userp; /* user logged in at the * tree connect level (base) */ char *pathname; /* pathname derived from sharename */ + afs_uint32 delete; /* ok to del: locked by smb_rctLock */ } smb_tid_t; -#define SMB_TIDFLAG_DELETE 1 /* delete struct when ref count zero */ -#define SMB_TIDFLAG_IPC 2 /* IPC$ */ +#define SMB_TIDFLAG_IPC 1 /* IPC$ */ /* one per process ID */ typedef struct smb_pid { @@ -327,7 +328,7 @@ typedef struct smb_ioctl { typedef struct smb_fid { osi_queue_t q; unsigned long refCount; - unsigned long flags; + afs_uint32 flags; /* protected by mx */ osi_mutex_t mx; /* for non-tree-related stuff */ unsigned short fid; /* the file ID */ struct smb_vc *vcp; /* back ptr */ @@ -350,11 +351,12 @@ typedef struct smb_fid { int prev_chunk; /* previous chunk read */ int raw_writers; /* pending async raw writes */ EVENT_HANDLE raw_write_event; /* signal this when raw_writers zero */ + afs_uint32 delete; /* ok to del: locked by smb_rctLock */ } smb_fid_t; #define SMB_FID_OPENREAD 1 /* open for reading */ #define SMB_FID_OPENWRITE 2 /* open for writing */ -#define SMB_FID_DELETE 4 /* delete struct on ref count 0 */ +#define SMB_FID_UNUSED 4 /* free for use */ #define SMB_FID_IOCTL 8 /* a file descriptor for the * magic ioctl file */ #define SMB_FID_OPENDELETE 0x10 /* open for deletion (NT) */ -- 2.39.5