An improper locking bug on the lock NewThread->StartMutex

[ryancaicse]

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

os/apps/libc/dynamic/pthread/pthread.c

Lines 248 to 278 in 460ae51

	pthread_mutex_lock(&(NewThread->StartMutex));
	//
	// Block all possible signals in the new thread while it sets itself up,
	// including the internal signals.
	//
	FILL_SIGNAL_SET(InternalSignals);
	NewThread->SignalMask = OsSetSignalBehavior(SignalMaskBlocked,
	SignalMaskOperationOverwrite,
	&InternalSignals);
	KernelStatus = OsCreateThread(NULL,
	0,
	ClpThreadStart,
	NewThread,
	NewThread->Attribute.StackBase,
	NewThread->Attribute.StackSize,
	NewThread->OsData,
	&(NewThread->ThreadId));
	OsSetSignalBehavior(SignalMaskBlocked,
	SignalMaskOperationOverwrite,
	&(NewThread->SignalMask));
	if (!KSUCCESS(KernelStatus)) {
	Status = ClConvertKstatusToErrorNumber(KernelStatus);
	ClpDestroyThreadKeyData(NewThread);
	ClpDestroyThread(NewThread);
	return Status;
	}

[evangreen]

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]

@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?

os/apps/libc/dynamic/pthread/pthread.c

Lines 1219 to 1228 in 460ae51

	ClCurrentThread = Thread;
	pthread_mutex_lock(&(Thread->StartMutex));
	OsSetSignalBehavior(SignalMaskBlocked,
	SignalMaskOperationOverwrite,
	&(Thread->SignalMask));
	pthread_testcancel();
	Result = Thread->ThreadRoutine(Thread->ThreadParameter);
	pthread_exit(Result);
	return;

[evangreen]

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]

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