From c0f753dfae6d69344ff84eb052f91971f304e652 Mon Sep 17 00:00:00 2001 From: apnadkarni Date: Fri, 29 Sep 2023 11:19:46 +0000 Subject: [PATCH 1/4] Start on zipfs finalization panic --- generic/tclZipfs.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/generic/tclZipfs.c b/generic/tclZipfs.c index 7b3911caf5f..befcc7d383c 100644 --- a/generic/tclZipfs.c +++ b/generic/tclZipfs.c @@ -1678,9 +1678,15 @@ ZipFSOpenArchive( ZIPFS_POSIX_ERROR(interp, "file read error"); goto error; } - Tcl_Close(interp, zf->chan); - zf->chan = NULL; } + /* + * Close the Tcl channel. If the file was mapped, the mapping is + * unaffected. It is important to close the channel otherwise there is a + * potential chicken and egg issue at finalization time as the channels + * are closed before the file systems are dismounted. + */ + Tcl_Close(interp, zf->chan); + zf->chan = NULL; return ZipFSFindTOC(interp, needZip, zf); /* From 5d2cc9aa8096c724098eaa68d33bdbdbf11fda3c Mon Sep 17 00:00:00 2001 From: apnadkarni Date: Fri, 29 Sep 2023 16:39:33 +0000 Subject: [PATCH 2/4] Refactor zipfs finalization --- generic/tclIO.c | 3 +- generic/tclInt.h | 5 +- generic/tclZipfs.c | 114 ++++++++++++++++++++++++++++++++++----------- 3 files changed, 91 insertions(+), 31 deletions(-) diff --git a/generic/tclIO.c b/generic/tclIO.c index 1527dc0f09d..2e25d2dab1c 100644 --- a/generic/tclIO.c +++ b/generic/tclIO.c @@ -706,8 +706,9 @@ TclFinalizeIOSubsystem(void) TclpFinalizeSockets(); TclpFinalizePipes(); + TclZipfsFinalize(); } - + /* *---------------------------------------------------------------------- * diff --git a/generic/tclInt.h b/generic/tclInt.h index a336a53e92f..399d83c450d 100644 --- a/generic/tclInt.h +++ b/generic/tclInt.h @@ -3476,8 +3476,9 @@ MODULE_SCOPE void * TclpThreadGetGlobalTSD(void *tsdKeyPtr); MODULE_SCOPE void TclErrorStackResetIf(Tcl_Interp *interp, const char *msg, Tcl_Size length); /* Tip 430 */ -MODULE_SCOPE int TclZipfs_Init(Tcl_Interp *interp); -MODULE_SCOPE int TclIsZipfsPath(const char *path); +MODULE_SCOPE int TclZipfs_Init(Tcl_Interp *interp); +MODULE_SCOPE int TclIsZipfsPath(const char *path); +MODULE_SCOPE void TclZipfsFinalize(void); MODULE_SCOPE int *TclGetUnicodeFromObj(Tcl_Obj *, int *); MODULE_SCOPE Tcl_Obj *TclNewUnicodeObj(const int *, int); diff --git a/generic/tclZipfs.c b/generic/tclZipfs.c index befcc7d383c..14da08319c8 100644 --- a/generic/tclZipfs.c +++ b/generic/tclZipfs.c @@ -319,10 +319,12 @@ static int InitWritableChannel(Tcl_Interp *interp, ZipChannel *info, ZipEntry *z, int trunc); static int ListMountPoints(Tcl_Interp *interp); static int ContainsMountPoint(const char *path, int pathLen); -static void SerializeCentralDirectoryEntry( - const unsigned char *start, - const unsigned char *end, unsigned char *buf, - ZipEntry *z, size_t nameLength); +static void CleanupMount(ZipFile *zf); +static void SerializeCentralDirectoryEntry(const unsigned char *start, + const unsigned char *end, + unsigned char *buf, + ZipEntry *z, + size_t nameLength); static void SerializeCentralDirectorySuffix( const unsigned char *start, const unsigned char *end, unsigned char *buf, @@ -364,10 +366,7 @@ static int ZipFSLoadFile(Tcl_Interp *interp, Tcl_Obj *path, Tcl_FSUnloadFileProc **unloadProcPtr, int flags); static int ZipMapArchive(Tcl_Interp *interp, ZipFile *zf, void *handle); -static void ZipfsExitHandler(void *clientData); -static void ZipfsMountExitHandler(void *clientData); static void ZipfsSetup(void); -static void ZipfsFinalize(void); static int ZipChannelClose(void *instanceData, Tcl_Interp *interp, int flags); static Tcl_DriverGetHandleProc ZipChannelGetFile; @@ -1906,7 +1905,6 @@ ZipFSCatalogFilesystem( */ zf->mountPoint = (char *) Tcl_GetHashKey(&ZipFS.zipHash, hPtr); - Tcl_CreateExitHandler(ZipfsMountExitHandler, zf); zf->mountPointLen = strlen(zf->mountPoint); zf->nameLength = strlen(zipname); @@ -2154,7 +2152,6 @@ ZipfsSetup(void) ckalloc(strlen(ZIPFS_FALLBACK_ENCODING) + 1); strcpy(ZipFS.fallbackEntryEncoding, ZIPFS_FALLBACK_ENCODING); ZipFS.initialized = 1; - Tcl_CreateExitHandler(ZipfsExitHandler, NULL); } /* @@ -2207,6 +2204,41 @@ ListMountPoints( return TCL_OK; } +/* + *------------------------------------------------------------------------ + * + * CleanupMount -- + * + * Releases all resources associated with a mounted archive. There + * must not be any open files in the archive. + * + * Caller MUST be holding WriteLock() before calling this function. + * + * Results: + * None. + * + * Side effects: + * Memory associated with the mounted archive is deallocated. + *------------------------------------------------------------------------ + */ +static void +CleanupMount(ZipFile *zf) /* Mount point */ +{ + ZipEntry *z, *znext; + Tcl_HashEntry *hPtr; + for (z = zf->entries; z; z = znext) { + znext = z->next; + hPtr = Tcl_FindHashEntry(&ZipFS.fileHash, z->name); + if (hPtr) { + Tcl_DeleteHashEntry(hPtr); + } + if (z->data) { + ckfree(z->data); + } + ckfree(z); + } +} + /* *------------------------------------------------------------------------- * @@ -2349,6 +2381,7 @@ TclZipfs_MountBuffer( { ZipFile *zf; + /* TODO - how come a *read* lock suffices for initialzing ? */ ReadLock(); if (!ZipFS.initialized) { ZipfsSetup(); @@ -2439,7 +2472,6 @@ TclZipfs_Unmount( const char *mountPoint) /* Mount point path. */ { ZipFile *zf; - ZipEntry *z, *znext; Tcl_HashEntry *hPtr; Tcl_DString dsm; int ret = TCL_OK, unmounted = 0; @@ -2477,19 +2509,9 @@ TclZipfs_Unmount( * still cleaning things up. */ - for (z = zf->entries; z; z = znext) { - znext = z->next; - hPtr = Tcl_FindHashEntry(&ZipFS.fileHash, z->name); - if (hPtr) { - Tcl_DeleteHashEntry(hPtr); - } - if (z->data) { - ckfree(z->data); - } - ckfree(z); - } + CleanupMount(zf); ZipFSCloseArchive(interp, zf); - Tcl_DeleteExitHandler(ZipfsMountExitHandler, zf); + ckfree(zf); unmounted = 1; @@ -6203,6 +6225,7 @@ ZipfsAppHookFindTclInit( return TCL_ERROR; } #endif +#ifdef OBSOLETE static void ZipfsExitHandler( @@ -6222,15 +6245,49 @@ ZipfsExitHandler( } } } +#endif -static void -ZipfsFinalize(void) { - Tcl_FSUnregister(&zipfsFilesystem); - Tcl_DeleteHashTable(&ZipFS.fileHash); - ckfree(ZipFS.fallbackEntryEncoding); - ZipFS.initialized = -1; +void TclZipfsFinalize(void) +{ + /* + * Finalization steps: + * For every mounted archive, if it no longer has any open handles + * clean up the mount and associated zip file entries. + * If there are no more mounted archives, clean up and free the + * ZipFS.fileHash and ZipFS.zipHash tables. + */ + WriteLock(); + + Tcl_HashEntry *hPtr; + Tcl_HashSearch zipSearch; + for (hPtr = Tcl_FirstHashEntry(&ZipFS.zipHash, &zipSearch); hPtr; + hPtr = Tcl_NextHashEntry(&zipSearch)) { + ZipFile *zf = (ZipFile *) Tcl_GetHashValue(hPtr); + if (zf->numOpen == 0) { + Tcl_DeleteHashEntry(hPtr); + CleanupMount(zf); + ZipFSCloseArchive(NULL, zf); + ckfree(zf); + } + } + + hPtr = Tcl_FirstHashEntry(&ZipFS.fileHash, &zipSearch); + if (hPtr == NULL) { + hPtr = Tcl_FirstHashEntry(&ZipFS.zipHash, &zipSearch); + if (hPtr == NULL) { + /* Both hash tables empty. Free them */ + Tcl_DeleteHashTable(&ZipFS.fileHash); + Tcl_DeleteHashTable(&ZipFS.zipHash); + Tcl_FSUnregister(&zipfsFilesystem); + ckfree(ZipFS.fallbackEntryEncoding); + ZipFS.initialized = -1; + } + } + + Unlock(); } +#ifdef OBSOLETE static void ZipfsMountExitHandler( void *clientData) @@ -6250,6 +6307,7 @@ ZipfsMountExitHandler( } } +#endif /* *------------------------------------------------------------------------- From 99e1b20c806fb795368afb3512e472bd93931313 Mon Sep 17 00:00:00 2001 From: apnadkarni Date: Fri, 29 Sep 2023 17:17:11 +0000 Subject: [PATCH 3/4] Add test for crash --- tests/zipfs.test | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/tests/zipfs.test b/tests/zipfs.test index 64ce1ebb24c..583a4d83977 100644 --- a/tests/zipfs.test +++ b/tests/zipfs.test @@ -1509,6 +1509,19 @@ namespace eval test_ns_zipfs { list [catch {read $fd} message] [close $fd] $message close $fd } -result {file size error (may be zip64)} -returnCodes error + + test bug-8259d74a64 "Crash exiting with open files" -setup { + set path [zippath test.zip] + set script "zipfs mount $path /\n" + append script {open [zipfs root]test} \n + append script "exit\n" + } -body { + set fd [open |[info nameofexecutable] r+] + puts $fd $script + flush $fd + read $fd + close $fd + } -result "" } From d7cff9067558c62d7c8259e7fe885c5fa8d5f438 Mon Sep 17 00:00:00 2001 From: apnadkarni Date: Sat, 30 Sep 2023 02:58:33 +0000 Subject: [PATCH 4/4] Remove obsolete code --- generic/tclZipfs.c | 61 +++++++--------------------------------------- 1 file changed, 9 insertions(+), 52 deletions(-) diff --git a/generic/tclZipfs.c b/generic/tclZipfs.c index 14da08319c8..0cc46a05757 100644 --- a/generic/tclZipfs.c +++ b/generic/tclZipfs.c @@ -16,7 +16,7 @@ * projects. * * Helpful docs: - * https://pkware.cachefly.net/webdocs/APPNOTE/APPNOTE-6.3.9.TXT + * https://pkware.cachefly.net/webdocs/APPNOTE/APPNOTE-6.3.9.TXT * https://libzip.org/specifications/appnote_iz.txt */ @@ -320,11 +320,10 @@ static int InitWritableChannel(Tcl_Interp *interp, static int ListMountPoints(Tcl_Interp *interp); static int ContainsMountPoint(const char *path, int pathLen); static void CleanupMount(ZipFile *zf); -static void SerializeCentralDirectoryEntry(const unsigned char *start, - const unsigned char *end, - unsigned char *buf, - ZipEntry *z, - size_t nameLength); +static void SerializeCentralDirectoryEntry( + const unsigned char *start, + const unsigned char *end, unsigned char *buf, + ZipEntry *z, size_t nameLength); static void SerializeCentralDirectorySuffix( const unsigned char *start, const unsigned char *end, unsigned char *buf, @@ -2237,6 +2236,7 @@ CleanupMount(ZipFile *zf) /* Mount point */ } ckfree(z); } + zf->entries = NULL; } /* @@ -6225,27 +6225,6 @@ ZipfsAppHookFindTclInit( return TCL_ERROR; } #endif -#ifdef OBSOLETE - -static void -ZipfsExitHandler( - TCL_UNUSED(void *) -) -{ - Tcl_HashEntry *hPtr; - Tcl_HashSearch search; - if (ZipFS.initialized != -1) { - hPtr = Tcl_FirstHashEntry(&ZipFS.fileHash, &search); - if (hPtr == NULL) { - ZipfsFinalize(); - } else { - /* ZipFS.fallbackEntryEncoding was already freed by - * ZipfsMountExitHandler - */ - } - } -} -#endif void TclZipfsFinalize(void) { @@ -6275,40 +6254,18 @@ void TclZipfsFinalize(void) if (hPtr == NULL) { hPtr = Tcl_FirstHashEntry(&ZipFS.zipHash, &zipSearch); if (hPtr == NULL) { - /* Both hash tables empty. Free them */ + /* Both hash tables empty. Free all resources */ + Tcl_FSUnregister(&zipfsFilesystem); Tcl_DeleteHashTable(&ZipFS.fileHash); Tcl_DeleteHashTable(&ZipFS.zipHash); - Tcl_FSUnregister(&zipfsFilesystem); ckfree(ZipFS.fallbackEntryEncoding); - ZipFS.initialized = -1; + ZipFS.initialized = 0; } } Unlock(); } -#ifdef OBSOLETE -static void -ZipfsMountExitHandler( - void *clientData) -{ - Tcl_HashEntry *hPtr; - Tcl_HashSearch search; - - ZipFile *zf = (ZipFile *) clientData; - - if (TCL_OK != TclZipfs_Unmount(NULL, zf->mountPoint)) { - Tcl_Panic("tried to unmount busy filesystem"); - } - - hPtr = Tcl_FirstHashEntry(&ZipFS.fileHash, &search); - if (hPtr == NULL) { - ZipfsFinalize(); - } - -} -#endif - /* *------------------------------------------------------------------------- *