Skip to content

Commit 04c482b

Browse files
fjtrujyclaude
andcommitted
fix: [libpthreadglue] tighten error handling in OS-abstraction layer
pte_osThreadCreate previously: - called CreateThread with a stack pointer that hadn't been validated, then NULL-checked it afterwards - wrote into __threadInfo[threadId] before checking whether threadId was negative — a CreateThread failure caused an OOB write - ignored the return value of CreateSema for cancelSem - on cleanup paths, leaked the cancelSem semaphore and the stack allocation Reorder so each resource is validated immediately after allocation and cleaned up in reverse order on failure. Also propagate CreateSema failures from pte_osMutexCreate and pte_osSemaphoreCreate as PTE_OS_NO_RESOURCES instead of returning a negative handle as if it were valid. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 8beead0 commit 04c482b

1 file changed

Lines changed: 50 additions & 35 deletions

File tree

ee/libpthreadglue/src/osal.c

Lines changed: 50 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -199,7 +199,6 @@ pte_osResult pte_osThreadCreate(pte_osThreadEntryPoint entryPoint,
199199
static int threadNum = 1;
200200
void *pTls;
201201
s32 threadId;
202-
pte_osResult result;
203202
ps2ThreadData *pThreadData;
204203

205204
if (threadNum++ > MAX_PS2_UID) {
@@ -215,8 +214,7 @@ pte_osResult pte_osThreadCreate(pte_osThreadEntryPoint entryPoint,
215214
pTls = pteTlsThreadInit();
216215
if (pTls == NULL) {
217216
PS2_DEBUG("pteTlsThreadInit: PTE_OS_NO_RESOURCES\n");
218-
result = PTE_OS_NO_RESOURCES;
219-
goto FAIL0;
217+
return PTE_OS_NO_RESOURCES;
220218
}
221219

222220
/* Allocate some memory for our per-thread control data. We use this for:
@@ -227,10 +225,8 @@ pte_osResult pte_osThreadCreate(pte_osThreadEntryPoint entryPoint,
227225

228226
if (pThreadData == NULL) {
229227
pteTlsThreadDestroy(pTls);
230-
231228
PS2_DEBUG("malloc(ps2ThreadData): PTE_OS_NO_RESOURCES\n");
232-
result = PTE_OS_NO_RESOURCES;
233-
goto FAIL0;
229+
return PTE_OS_NO_RESOURCES;
234230
}
235231

236232
/* Save a pointer to our per-thread control data as a TLS value */
@@ -243,19 +239,47 @@ pte_osResult pte_osThreadCreate(pte_osThreadEntryPoint entryPoint,
243239
sema.max_count = 255;
244240
sema.option = 0;
245241
pThreadData->cancelSem = CreateSema(&sema);
242+
if (pThreadData->cancelSem < 0) {
243+
free(pThreadData);
244+
pteTlsThreadDestroy(pTls);
245+
PS2_DEBUG("CreateSema(cancelSem): PTE_OS_NO_RESOURCES\n");
246+
return PTE_OS_NO_RESOURCES;
247+
}
246248

247-
/* Create EE Thread */
249+
/* Allocate the thread stack before calling CreateThread — passing a
250+
* NULL stack to the EE kernel is undefined behaviour and the original
251+
* check below ran *after* the syscall.
252+
*/
248253
stack = malloc(stackSize);
254+
if (!stack) {
255+
DeleteSema(pThreadData->cancelSem);
256+
free(pThreadData);
257+
pteTlsThreadDestroy(pTls);
258+
PS2_DEBUG("malloc(stack): PTE_OS_NO_RESOURCES\n");
259+
return PTE_OS_NO_RESOURCES;
260+
}
261+
262+
eethread.attr = 0;
263+
eethread.option = 0;
264+
eethread.func = &__ps2StubThreadEntry;
265+
eethread.stack = stack;
266+
eethread.stack_size = stackSize;
267+
eethread.gp_reg = &_gp;
268+
eethread.initial_priority = invert_priority(initialPriority);
269+
threadId = CreateThread(&eethread);
270+
271+
if (threadId < 0) {
272+
/* Must check threadId *before* indexing __threadInfo — a negative
273+
* id is an OOB write.
274+
*/
275+
free(stack);
276+
DeleteSema(pThreadData->cancelSem);
277+
free(pThreadData);
278+
pteTlsThreadDestroy(pTls);
279+
PS2_DEBUG("CreateThread: PTE_OS_GENERAL_FAILURE\n");
280+
return PTE_OS_GENERAL_FAILURE;
281+
}
249282

250-
eethread.attr = 0;
251-
eethread.option = 0;
252-
eethread.func = &__ps2StubThreadEntry;
253-
eethread.stack = stack;
254-
eethread.stack_size = stackSize;
255-
eethread.gp_reg = &_gp;
256-
eethread.initial_priority = invert_priority(initialPriority);
257-
threadId = CreateThread(&eethread);
258-
259283
/* In order to emulate TLS functionality, we append the address of the TLS structure that we
260284
* allocated above to an additional struct. To set or get TLS values for this thread, the user
261285
* needs to get the threadId from the OS and then extract
@@ -265,25 +289,8 @@ pte_osResult pte_osThreadCreate(pte_osThreadEntryPoint entryPoint,
265289
threadInfo->threadNumber = threadNum;
266290
threadInfo->tlsPtr = pTls;
267291

268-
if (!stack) {
269-
free(pThreadData);
270-
pteTlsThreadDestroy(pTls);
271-
272-
PS2_DEBUG("CreateThread: PTE_OS_NO_RESOURCES\n");
273-
result = PTE_OS_NO_RESOURCES;
274-
} else if (threadId < 0) {
275-
free(pThreadData);
276-
pteTlsThreadDestroy(pTls);
277-
278-
PS2_DEBUG("CreateThread: PTE_OS_GENERAL_FAILURE\n");
279-
result = PTE_OS_GENERAL_FAILURE;
280-
} else {
281-
*ppte_osThreadHandle = threadId;
282-
result = PTE_OS_OK;
283-
}
284-
285-
FAIL0:
286-
return result;
292+
*ppte_osThreadHandle = threadId;
293+
return PTE_OS_OK;
287294
}
288295
#endif
289296

@@ -528,6 +535,10 @@ pte_osResult pte_osMutexCreate(pte_osMutexHandle *pHandle)
528535
sema.option = 0;
529536
handle = CreateSema(&sema);
530537

538+
if (handle < 0) {
539+
return PTE_OS_NO_RESOURCES;
540+
}
541+
531542
*pHandle = handle;
532543
return PTE_OS_OK;
533544
}
@@ -598,6 +609,10 @@ pte_osResult pte_osSemaphoreCreate(int initialValue, pte_osSemaphoreHandle *pHan
598609
sema.option = 0;
599610
handle = CreateSema(&sema);
600611

612+
if (handle < 0) {
613+
return PTE_OS_NO_RESOURCES;
614+
}
615+
601616
*pHandle = handle;
602617
return PTE_OS_OK;
603618
}

0 commit comments

Comments
 (0)