Serialize all libzip access behind one mutex - fixes PSP read corruption
Reverting the whole-PSAR buffer (previous commit) reintroduced real read corruption on hardware (EINVAL, then zlib data errors) - but this time it hit an unrelated asset (chunks/1_0_0.dcf) at the same moment as the WAV load, which pointed at concurrency rather than the seek pattern itself: the asset system genuinely calls libzip from three real threads at once (main, the background asset load thread, and PSP's own audio feeder thread), and libzip is documented as not thread-safe. Buffering the whole PSAR "worked" only by accident, since it stopped touching sceIo after init entirely. Adding ASSET.zipLock around every zip_fopen/zip_fread/ zip_fclose/zip_fseek/zip_stat/zip_name_locate call serializes hardware I/O properly without needing the whole archive resident in memory. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
@@ -19,6 +19,8 @@ asset_t ASSET;
|
|||||||
errorret_t assetInit(void) {
|
errorret_t assetInit(void) {
|
||||||
memoryZero(&ASSET, sizeof(asset_t));
|
memoryZero(&ASSET, sizeof(asset_t));
|
||||||
|
|
||||||
|
threadMutexInit(&ASSET.zipLock);
|
||||||
|
|
||||||
for(size_t i = 0; i < ASSET_LOADING_COUNT_MAX; i++) {
|
for(size_t i = 0; i < ASSET_LOADING_COUNT_MAX; i++) {
|
||||||
threadMutexInit(&ASSET.loading[i].mutex);
|
threadMutexInit(&ASSET.loading[i].mutex);
|
||||||
}
|
}
|
||||||
@@ -37,9 +39,12 @@ errorret_t assetInit(void) {
|
|||||||
bool_t assetFileExists(const char_t *filename) {
|
bool_t assetFileExists(const char_t *filename) {
|
||||||
assertStrLenMax(filename, ASSET_FILE_NAME_MAX, "Filename too long.");
|
assertStrLenMax(filename, ASSET_FILE_NAME_MAX, "Filename too long.");
|
||||||
|
|
||||||
if(zip_name_locate(ASSET.zip, filename, 0) >= 0) return true;
|
threadMutexLock(&ASSET.zipLock);
|
||||||
if(zip_name_locate(ASSET.zipStored, filename, 0) >= 0) return true;
|
bool_t found =
|
||||||
return false;
|
zip_name_locate(ASSET.zip, filename, 0) >= 0 ||
|
||||||
|
zip_name_locate(ASSET.zipStored, filename, 0) >= 0;
|
||||||
|
threadMutexUnlock(&ASSET.zipLock);
|
||||||
|
return found;
|
||||||
}
|
}
|
||||||
|
|
||||||
assetentry_t * assetGetEntry(
|
assetentry_t * assetGetEntry(
|
||||||
@@ -453,5 +458,6 @@ errorret_t assetDispose(void) {
|
|||||||
}
|
}
|
||||||
|
|
||||||
errorChain(assetDisposePlatform());
|
errorChain(assetDisposePlatform());
|
||||||
|
threadMutexDispose(&ASSET.zipLock);
|
||||||
errorOk();
|
errorOk();
|
||||||
}
|
}
|
||||||
@@ -39,6 +39,18 @@ typedef struct asset_s {
|
|||||||
|
|
||||||
assetplatform_t platform;
|
assetplatform_t platform;
|
||||||
|
|
||||||
|
// Guards every libzip call against ASSET.zip/ASSET.zipStored (zip_fopen,
|
||||||
|
// zip_fread, zip_fclose, zip_fseek, zip_stat, zip_name_locate - see
|
||||||
|
// assetfile.c/assetFileExists). libzip is documented as not thread-safe,
|
||||||
|
// and this asset system genuinely calls it from multiple real threads at
|
||||||
|
// once (main thread, the background load thread below, and - on
|
||||||
|
// platforms that stream PCM from an asset on their own thread, like PSP
|
||||||
|
// - an audio feeder thread too). Confirmed necessary on real PSP
|
||||||
|
// hardware: without this lock, concurrent zip_fopen/zip_fread calls from
|
||||||
|
// different threads corrupted reads (EINVAL, then zlib data errors) once
|
||||||
|
// dusk.dsk stopped being read entirely into memory up front.
|
||||||
|
threadmutex_t zipLock;
|
||||||
|
|
||||||
// Background loading thread.
|
// Background loading thread.
|
||||||
thread_t loadThread;
|
thread_t loadThread;
|
||||||
|
|
||||||
|
|||||||
@@ -28,11 +28,16 @@ errorret_t assetFileInit(
|
|||||||
// the stored one - remember which matched so assetFileOpen opens it from
|
// the stored one - remember which matched so assetFileOpen opens it from
|
||||||
// the right archive.
|
// the right archive.
|
||||||
zip_stat_init(&file->stat);
|
zip_stat_init(&file->stat);
|
||||||
|
threadMutexLock(&ASSET.zipLock);
|
||||||
if(zip_stat(ASSET.zip, filename, 0, &file->stat) == 0) {
|
if(zip_stat(ASSET.zip, filename, 0, &file->stat) == 0) {
|
||||||
file->sourceZip = ASSET.zip;
|
file->sourceZip = ASSET.zip;
|
||||||
} else if(zip_stat(ASSET.zipStored, filename, 0, &file->stat) == 0) {
|
} else if(zip_stat(ASSET.zipStored, filename, 0, &file->stat) == 0) {
|
||||||
file->sourceZip = ASSET.zipStored;
|
file->sourceZip = ASSET.zipStored;
|
||||||
} else {
|
} else {
|
||||||
|
file->sourceZip = NULL;
|
||||||
|
}
|
||||||
|
threadMutexUnlock(&ASSET.zipLock);
|
||||||
|
if(file->sourceZip == NULL) {
|
||||||
errorThrow("Failed to stat asset file: %s", filename);
|
errorThrow("Failed to stat asset file: %s", filename);
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -60,8 +65,12 @@ errorret_t assetFileRewind(assetfile_t *file) {
|
|||||||
// skipping ranges of the decompressed content on some re-opens after
|
// skipping ranges of the decompressed content on some re-opens after
|
||||||
// the first. A real seek avoids that failure mode entirely, with no
|
// the first. A real seek avoids that failure mode entirely, with no
|
||||||
// extra memory cost over the close+reopen fallback.
|
// extra memory cost over the close+reopen fallback.
|
||||||
if(zip_file_is_seekable(file->zipFile)) {
|
threadMutexLock(&ASSET.zipLock);
|
||||||
if(zip_fseek(file->zipFile, 0, SEEK_SET) != 0) {
|
bool_t seekable = zip_file_is_seekable(file->zipFile);
|
||||||
|
int seekResult = seekable ? zip_fseek(file->zipFile, 0, SEEK_SET) : -1;
|
||||||
|
threadMutexUnlock(&ASSET.zipLock);
|
||||||
|
if(seekable) {
|
||||||
|
if(seekResult != 0) {
|
||||||
errorThrow("Failed to seek asset file: %s", file->filename);
|
errorThrow("Failed to seek asset file: %s", file->filename);
|
||||||
}
|
}
|
||||||
file->position = 0;
|
file->position = 0;
|
||||||
@@ -79,7 +88,9 @@ errorret_t assetFileOpen(assetfile_t *file) {
|
|||||||
assertNotNull(file->sourceZip, "Asset file must be inited before opening.");
|
assertNotNull(file->sourceZip, "Asset file must be inited before opening.");
|
||||||
assertNull(file->zipFile, "Asset file already open.");
|
assertNull(file->zipFile, "Asset file already open.");
|
||||||
|
|
||||||
|
threadMutexLock(&ASSET.zipLock);
|
||||||
file->zipFile = zip_fopen(file->sourceZip, file->filename, 0);
|
file->zipFile = zip_fopen(file->sourceZip, file->filename, 0);
|
||||||
|
threadMutexUnlock(&ASSET.zipLock);
|
||||||
if(file->zipFile == NULL) {
|
if(file->zipFile == NULL) {
|
||||||
errorThrow("Failed to open asset file: %s", file->filename);
|
errorThrow("Failed to open asset file: %s", file->filename);
|
||||||
}
|
}
|
||||||
@@ -120,15 +131,27 @@ errorret_t assetFileRead(
|
|||||||
size_t chunkSize = mathMin(
|
size_t chunkSize = mathMin(
|
||||||
bufferSize - totalRead, ASSET_FILE_READ_CHUNK_MAX
|
bufferSize - totalRead, ASSET_FILE_READ_CHUNK_MAX
|
||||||
);
|
);
|
||||||
|
threadMutexLock(&ASSET.zipLock);
|
||||||
zip_int64_t bytesRead = zip_fread(
|
zip_int64_t bytesRead = zip_fread(
|
||||||
file->zipFile, dest + totalRead, chunkSize
|
file->zipFile, dest + totalRead, chunkSize
|
||||||
);
|
);
|
||||||
|
errorret_t readError = errorOkImpl();
|
||||||
if(bytesRead < 0) {
|
if(bytesRead < 0) {
|
||||||
errorThrow(
|
// Built (not thrown) while still holding the lock, so
|
||||||
|
// zip_file_strerror() reads the just-failed zipFile's error state
|
||||||
|
// before another thread can touch it - errorThrow() itself isn't
|
||||||
|
// used here since it returns immediately, which would leave
|
||||||
|
// ASSET.zipLock held forever.
|
||||||
|
readError = errorThrowImpl(
|
||||||
|
&ERROR_STATE, ERROR_NOT_OK, __FILE__, __func__, __LINE__,
|
||||||
"Failed to read from asset file: %s (%s)",
|
"Failed to read from asset file: %s (%s)",
|
||||||
file->filename, zip_file_strerror(file->zipFile)
|
file->filename, zip_file_strerror(file->zipFile)
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
threadMutexUnlock(&ASSET.zipLock);
|
||||||
|
if(errorIsNotOk(readError)) {
|
||||||
|
errorChain(readError);
|
||||||
|
}
|
||||||
if(bytesRead == 0) break;
|
if(bytesRead == 0) break;
|
||||||
totalRead += (size_t)bytesRead;
|
totalRead += (size_t)bytesRead;
|
||||||
}
|
}
|
||||||
@@ -141,7 +164,10 @@ errorret_t assetFileClose(assetfile_t *file) {
|
|||||||
assertNotNull(file, "Asset file cannot be NULL.");
|
assertNotNull(file, "Asset file cannot be NULL.");
|
||||||
assertNotNull(file->zipFile, "Asset file must be opened before closing.");
|
assertNotNull(file->zipFile, "Asset file must be opened before closing.");
|
||||||
|
|
||||||
if(zip_fclose(file->zipFile) != 0) {
|
threadMutexLock(&ASSET.zipLock);
|
||||||
|
int closeResult = zip_fclose(file->zipFile);
|
||||||
|
threadMutexUnlock(&ASSET.zipLock);
|
||||||
|
if(closeResult != 0) {
|
||||||
errorThrow("Failed to close asset file: %s", file->filename);
|
errorThrow("Failed to close asset file: %s", file->filename);
|
||||||
}
|
}
|
||||||
file->zipFile = NULL;
|
file->zipFile = NULL;
|
||||||
|
|||||||
Reference in New Issue
Block a user