From 01fbfb424865291e00242681662ed9b20c33a524 Mon Sep 17 00:00:00 2001 From: John Hodge Date: Sat, 2 Oct 2010 21:43:51 +0800 Subject: [PATCH] Fixed threading bug - Mutex->LastWaiting was never reset to NULL, hence could point to a thread on the active queue, causing corruption. HEADDESK, HEADDESK, HEADDESK - Also, fixed lack of LEAVE macros in some parts of binary.c - Fixed a typo in EXT2 directory handling (write) --- Kernel/arch/x86/include/arch.h | 2 +- Kernel/binary.c | 10 +++- Kernel/threads.c | 83 +++++++++++++++++----------------- Modules/Filesystems/Ext2/dir.c | 4 +- 4 files changed, 54 insertions(+), 45 deletions(-) diff --git a/Kernel/arch/x86/include/arch.h b/Kernel/arch/x86/include/arch.h index 045986fe..c4b3a2e6 100644 --- a/Kernel/arch/x86/include/arch.h +++ b/Kernel/arch/x86/include/arch.h @@ -12,7 +12,7 @@ // Allow nested spinlocks? #define STACKED_LOCKS 2 // 0: No, 1: Per-CPU, 2: Per-Thread -#define LOCK_DISABLE_INTS 0 +#define LOCK_DISABLE_INTS 1 // - Processor/Machine Specific Features #if ARCH != i386 && ARCH != i486 && ARCH != i586 diff --git a/Kernel/binary.c b/Kernel/binary.c index b3040a5d..aa5d9547 100644 --- a/Kernel/binary.c +++ b/Kernel/binary.c @@ -121,6 +121,7 @@ int Proc_Execve(char *File, char **ArgV, char **EnvP) argenvBuf = malloc(argenvBytes); if(argenvBuf == NULL) { Log_Error("BIN", "Proc_Execve - What the hell? The kernel is out of heap space"); + LEAVE('i', 0); return 0; } strBuf = argenvBuf + (argc+1)*sizeof(void*) + (envc+1)*sizeof(void*); @@ -158,6 +159,7 @@ int Proc_Execve(char *File, char **ArgV, char **EnvP) if(bases[0] == 0) { Log_Warning("BIN", "Proc_Execve - Unable to load '%s'", Threads_GetName(-1)); + LEAVE('-'); Threads_Exit(0, 0); for(;;); } @@ -171,6 +173,9 @@ int Proc_Execve(char *File, char **ArgV, char **EnvP) /** * \fn Uint Binary_Load(char *file, Uint *entryPoint) + * \brief Load a binary into the current address space + * \param file Path to binary to load + * \param entryPoint Pointer for exectuable entry point */ Uint Binary_Load(char *file, Uint *entryPoint) { @@ -178,7 +183,7 @@ Uint Binary_Load(char *file, Uint *entryPoint) tBinary *pBinary; Uint base = -1; - ENTER("sfile", file); + ENTER("sfile pentryPoint", file, entryPoint); // Sanity Check Argument if(file == NULL) { @@ -188,6 +193,7 @@ Uint Binary_Load(char *file, Uint *entryPoint) // Get True File Path sTruePath = VFS_GetTruePath(file); + LOG("sTruePath = %p", sTruePath); if(sTruePath == NULL) { Log_Warning("BIN", "'%s' does not exist.", file); @@ -196,6 +202,8 @@ Uint Binary_Load(char *file, Uint *entryPoint) } LOG("sTruePath = '%s'", sTruePath); + + // TODO: Also get modifcation time // Check if the binary has already been loaded if( !(pBinary = Binary_GetInfo(sTruePath)) ) diff --git a/Kernel/threads.c b/Kernel/threads.c index 51cfbdfe..3b731118 100644 --- a/Kernel/threads.c +++ b/Kernel/threads.c @@ -36,7 +36,7 @@ tThread *Threads_CloneTCB(Uint *Err, Uint Flags); int Threads_WaitTID(int TID, int *status); tThread *Threads_GetThread(Uint TID); void Threads_AddToDelete(tThread *Thread); -tThread *Threads_int_GetPrev(tThread **List, tThread *Thread); +tThread *Threads_int_DelFromQueue(tThread **List, tThread *Thread); void Threads_Exit(int TID, int Status); void Threads_Kill(tThread *Thread, int Status); void Threads_Yield(void); @@ -51,6 +51,7 @@ tUID Threads_GetUID(void); tGID Threads_GetGID(void); int Threads_SetGID(Uint *Errno, tUID ID); void Threads_Dump(void); +void Threads_DumpActive(void); void Mutex_Acquire(tMutex *Mutex); void Mutex_Release(tMutex *Mutex); int Mutex_IsLocked(tMutex *Mutex); @@ -362,33 +363,36 @@ void Threads_AddToDelete(tThread *Thread) } /** - * \brief Gets the previous entry in a thead linked list + * \brief Deletes an entry from a list * \param List Pointer to the list head * \param Thread Thread to find - * \return Thread before \a Thread on \a List - * \note This uses a massive hack of assuming that the first field in the - * structure is the .Next pointer. By doing this, we can return \a List - * as a (tThread*) and simplify other code. + * \return \a Thread */ -tThread *Threads_int_GetPrev(tThread **List, tThread *Thread) +tThread *Threads_int_DelFromQueue(tThread **List, tThread *Thread) { - tThread *ret; - // First Entry - if(*List == Thread) { - return (tThread*)List; + tThread *ret, *prev = NULL; + + for(ret = *List; + ret && ret != Thread; + prev = ret, ret = ret->Next + ); + + // Is the thread on the list + if(!ret) { + //LogF("%p(%s) is not on list %p\n", Thread, Thread->ThreadName, List); + return NULL; + } + + if( !prev ) { + *List = Thread->Next; + //LogF("%p(%s) removed from head of %p\n", Thread, Thread->ThreadName, List); } - // Or not else { - for(ret = *List; - ret->Next && ret->Next != Thread; - ret = ret->Next - ); - // Error if the thread is not on the list - if(!ret->Next || ret->Next != Thread) { - return NULL; - } + prev->Next = Thread->Next; + //LogF("%p(%s) removed from %p (prev=%p)\n", Thread, Thread->ThreadName, List, prev); } - return ret; + + return Thread; } /** @@ -415,7 +419,6 @@ void Threads_Exit(int TID, int Status) */ void Threads_Kill(tThread *Thread, int Status) { - tThread *prev; tMsg *msg; // TODO: Kill all children @@ -449,9 +452,9 @@ void Threads_Kill(tThread *Thread, int Status) // Lock thread list SHORTLOCK( &glThreadListLock ); - // Get previous thread on list - prev = Threads_int_GetPrev( &gActiveThreads, Thread ); - if(!prev) { + // Delete from active list + if( !Threads_int_DelFromQueue( &gActiveThreads, Thread ) ) + { Warning("Proc_Exit - Current thread is not on the active queue"); SHORTREL( &glThreadListLock ); SHORTREL( &Thread->IsLocked ); @@ -461,7 +464,6 @@ void Threads_Kill(tThread *Thread, int Status) // Ensure that we are not rescheduled Thread->Remaining = 0; // Clear Remaining Quantum Thread->Quantum = 0; // Clear Quantum to indicate dead thread - prev->Next = Thread->Next; // Remove from active // Update bookkeeping giNumActiveThreads --; @@ -553,8 +555,6 @@ void Threads_Sleep(void) */ int Threads_Wake(tThread *Thread) { - tThread *prev; - if(!Thread) return -EINVAL; @@ -567,8 +567,7 @@ int Threads_Wake(tThread *Thread) case THREAD_STAT_SLEEPING: SHORTLOCK( &glThreadListLock ); // Remove from sleeping queue - prev = Threads_int_GetPrev(&gSleepingThreads, Thread); - prev->Next = Thread->Next; + Threads_int_DelFromQueue(&gSleepingThreads, Thread); Threads_AddActive( Thread ); @@ -633,12 +632,12 @@ void Threads_AddActive(tThread *Thread) } #endif - // Add to active list - Thread->Next = gActiveThreads; - gActiveThreads = Thread; // Set state Thread->Status = THREAD_STAT_ACTIVE; Thread->CurCPU = -1; + // Add to active list + Thread->Next = gActiveThreads; + gActiveThreads = Thread; // Update bookkeeping giNumActiveThreads ++; @@ -659,12 +658,11 @@ void Threads_AddActive(tThread *Thread) tThread *Threads_RemActive(void) { tThread *ret = Proc_GetCurThread(); - tThread *prev; SHORTLOCK( &glThreadListLock ); - prev = Threads_int_GetPrev(&gActiveThreads, ret); - if(!prev) { + // Delete from active queue + if( !Threads_int_DelFromQueue(&gActiveThreads, ret) ) { SHORTREL( &glThreadListLock ); return NULL; } @@ -672,7 +670,6 @@ tThread *Threads_RemActive(void) ret->Remaining = 0; ret->CurCPU = -1; - prev->Next = ret->Next; giNumActiveThreads --; // no need to decrement tickets, scheduler did it for us @@ -839,7 +836,7 @@ tThread *Threads_GetNextToRun(int CPU, tThread *Last) if( CPU_HAS_LOCK( &glThreadListLock ) ) return Last; - // Same if the current CPU has any lock + // Don't change threads if the current CPU has switches disabled if( gaThreads_NoTaskSwitch[CPU] ) return Last; @@ -901,8 +898,8 @@ tThread *Threads_GetNextToRun(int CPU, tThread *Last) } #if DEBUG_TRACE_TICKETS else - LogF(" CPU %i released %p (%s)->Status = %i (Released)\n", - CPU, Last, Last->ThreadName, Last->Status); + LogF(" CPU %i released %p (%i %s)->Status = %i (Released)\n", + CPU, Last, Last->TID, Last->ThreadName, Last->Status); #endif Last->CurCPU = -1; } @@ -1010,7 +1007,7 @@ void Mutex_Acquire(tMutex *Mutex) if( Mutex->Owner ) { SHORTLOCK( &glThreadListLock ); // - Remove from active list - Threads_RemActive(); + us = Threads_RemActive(); // - Mark as sleeping us->Status = THREAD_STAT_OFFSLEEP; @@ -1046,6 +1043,9 @@ void Mutex_Release(tMutex *Mutex) if( Mutex->Waiting ) { Mutex->Owner = Mutex->Waiting; // Set owner Mutex->Waiting = Mutex->Waiting->Next; // Next! + // Reset ->LastWaiting to NULL if we have just removed the last waiting thread + if( Mutex->LastWaiting == Mutex->Owner ) + Mutex->LastWaiting = NULL; // Wake new owner SHORTLOCK( &glThreadListLock ); @@ -1070,6 +1070,7 @@ int Mutex_IsLocked(tMutex *Mutex) // === EXPORTS === EXPORT(Threads_GetUID); +EXPORT(Threads_GetGID); EXPORT(Mutex_Acquire); EXPORT(Mutex_Release); EXPORT(Mutex_IsLocked); diff --git a/Modules/Filesystems/Ext2/dir.c b/Modules/Filesystems/Ext2/dir.c index 99e787e8..c7d8dd89 100644 --- a/Modules/Filesystems/Ext2/dir.c +++ b/Modules/Filesystems/Ext2/dir.c @@ -283,7 +283,7 @@ int Ext2_Link(tVFS_Node *Node, tVFS_Node *Child, const char *Name) BLOCK_DIR_OFS(Node->Data, block) = nEntries; block ++; ofs = 0; - base = Ext2_int_GetBlockAddr(disk, inode.i_blocks, block); + base = Ext2_int_GetBlockAddr(disk, inode.i_block, block); VFS_ReadAt( disk->FD, base, disk->BlockSize, blockData ); } } @@ -291,7 +291,7 @@ int Ext2_Link(tVFS_Node *Node, tVFS_Node *Child, const char *Name) // Check if a free slot was found if( bestMatch >= 0 ) { // Read-Modify-Write - bestBlock = Ext2_int_GetBlockAddr(disk, inode.i_blocks, bestBlock); + bestBlock = Ext2_int_GetBlockAddr(disk, inode.i_block, bestBlock); if( block > 0 ) bestMatch = BLOCK_DIR_OFS(Node->Data, bestBlock); VFS_ReadAt( disk->FD, base, disk->BlockSize, blockData ); -- 2.20.1