search

minoca / os

Minoca operating system
Other
2.71k stars 231 forks source link

An improper locking bug on the lock NewThread->StartMutex #167

Open ryancaicse opened 2 years ago

ryancaicse commented 2 years ago

Hi, developers, thank you for your checking. It seems the lock NewThread->StartMutex is not released correctly when !KSUCCESS(KernelStatus)?

https://github.com/minoca/os/blob/460ae51663e02f607f0738521cc2488e0866e70c/apps/libc/dynamic/pthread/pthread.c#L248-L278

evangreen commented 2 years ago

Thanks Ryan! Technically you're right, things would be aesthetically balanced if we had a pthread_mutex_unlock(&(NewThread->StartMutex)) in the !KSUCCESS() case. In practice it doesn't end up mattering because NewThread is being destroyed in the !KSUCCESS() case, having failed to start the new thread.

I'm tempted to leave things as-is.

ryancaicse commented 2 years ago

@evangreen Hi, thank you for your reply and confirm this!

There is a similar case, the Thread->StartMutex is not released before the thread goes die. Would it better to release the lock before?

https://github.com/minoca/os/blob/460ae51663e02f607f0738521cc2488e0866e70c/apps/libc/dynamic/pthread/pthread.c#L1219-L1228

evangreen commented 2 years ago

Hm. It's a similar story there. The StartMutex acts like a barrier, ensuring that ClpThreadStart() doesn't let the thread run its main routine until pthread_create() has finished initializing the thread members. I suppose the StartMutex could be replaced with a pthread_cond_t, but the mutex is a little lighter.

ryancaicse commented 2 years ago

@evangreen OK, Thank you. Feel free to close this issue if they would be fixed.