Fix multiple memory leaks

.github/workflows/cppcheck.yml

@@ -8,6 +8,7 @@ defaults:
 jobs:
   build:
     runs-on: ubuntu-24.04
+    timeout-minutes: 5
     steps:
       - name: Checkout
         uses: actions/checkout@v4

.github/workflows/linux-build.yml

@@ -8,6 +8,7 @@ defaults:
 jobs:
   build:
     runs-on: ubuntu-24.04
+    timeout-minutes: 5
     strategy:
       matrix:
         compiler: ["gcc", "clang"]
@@ -58,7 +59,7 @@ jobs:
           }
       - name: Build
         run: |
-          make || {
+          make -j || {
             echo "::error::Failure during Build"
             exit 1
           }

.github/workflows/memleaks.yml

@@ -0,0 +1,65 @@
+name: Memory leak check
+on: [push]
+permissions:
+  contents: read
+defaults:
+  run:
+    shell: bash
+jobs:
+  build:
+    runs-on: ubuntu-24.04
+    timeout-minutes: 10
+    steps:
+
+      - name: Checkout Tcl
+        uses: actions/checkout@v4
+        with:
+          repository: tcltk/tcl
+          ref: core-8-6-14
+          path: tcl
+      - name: Configure Tcl
+        working-directory: tcl/unix
+        run: |
+          ./configure --enable-symbols=all --prefix $HOME/tcl_install || {
+            cat config.log
+            echo "::error::Failure during Configure Tcl"
+            exit 1
+          }
+      - name: Build Tcl
+        working-directory: tcl/unix
+        run: |
+          make -j || {
+            echo "::error::Failure during Build Tcl"
+            exit 1
+          }
+      - name: Install Tcl
+        working-directory: tcl/unix
+        run: |
+          make install || {
+            echo "::error::Failure during Install Tcl"
+            exit 1
+          }
+
+      - name: Checkout
+        uses: actions/checkout@v4
+        with:
+          submodules: recursive
+      - name: Configure
+        run: |
+          ./configure --with-tcl=$HOME/tcl_install --enable-symbols=all || {
+            cat config.log
+            echo "::error::Failure during Configure"
+            exit 1
+          }
+      - name: Build
+        run: |
+          make -j || {
+            echo "::error::Failure during Build"
+            exit 1
+          }
+      - name: Run Tests
+        run: |
+          MEMDEBUG=1 make test || {
+            echo "::error::Failure during Test"
+            exit 1
+          }

ChangeLog

@@ -1,3 +1,6 @@
+2024-05-30 Konstantin Kushnir <[email protected]>
+	* Fix multiple memory leaks
+
 2024-05-29 Konstantin Kushnir <[email protected]>
 	* Fix a regression when register volume and unregister with vfs::unmount
 	* Add memory leak hunter to tests

generic/fsindex.c

@@ -519,7 +519,7 @@ Cookfs_FsindexEntry *Cookfs_FsindexSet(Cookfs_Fsindex *i, Tcl_Obj *pathList, int
 
     /* if finding failed (i.e. parent did not exist), return NULL */
     if ((foundFileNode == NULL) || (dirNode == NULL)) {
-        CookfsLog(printf("Cookfs_FsindexSet - NULL"))
+        CookfsLog(printf("Cookfs_FsindexSet - NULL"));
         /* the current node is already released by CookfsFsindexFind() */
         return NULL;
     }
@@ -1088,14 +1088,14 @@ static Cookfs_FsindexEntry *CookfsFsindexFind(Cookfs_Fsindex *i, Cookfs_FsindexE
     char *pathTailStr;
 
     if (Tcl_ListObjLength(NULL, pathList, &listSize) != TCL_OK) {
-        return NULL;
+        goto error;
     }
     if (listSize == 0) {
 	if (command == COOKFSFSINDEX_FIND_FIND) {
             return i->rootItem;
 	}  else  {
 	    /* create or delete will not work with empty file list */
-	    return NULL;
+	    goto error;
 	}
     }
 
@@ -1112,19 +1112,19 @@ static Cookfs_FsindexEntry *CookfsFsindexFind(Cookfs_Fsindex *i, Cookfs_FsindexE
     /* if parent was not found or is not a directory, return NULL */
     if (currentNode == NULL) {
         CookfsLog(printf("CookfsFsindexCreateHashElement - node not found"))
-        return NULL;
+        goto error;
     }
 
     if (currentNode->fileBlocks != COOKFS_NUMBLOCKS_DIRECTORY) {
         CookfsLog(printf("CookfsFsindexCreateHashElement - not a directory"))
-        return NULL;
+        goto error;
     }
 
     /* get information about fail of the file name
      * and invoke CookfsFsindexFindInDirectory() */
     if (Tcl_ListObjIndex(NULL, pathList, listSize - 1, &pathTail) != TCL_OK) {
         CookfsLog(printf("CookfsFsindexCreateHashElement - Unable to get element"))
-        return NULL;
+        goto error;
     }
 
     pathTailStr = Tcl_GetStringFromObj(pathTail, NULL);
@@ -1135,6 +1135,14 @@ static Cookfs_FsindexEntry *CookfsFsindexFind(Cookfs_Fsindex *i, Cookfs_FsindexE
         Cookfs_FsindexIncrChangeCount(i, 1);
     }
     return rc;
+
+error:
+
+    if (newFileNode != NULL) {
+        Cookfs_FsindexEntryFree(newFileNode);
+    }
+    return NULL;
+
 }
 
 

generic/fsindexCmd.c

@@ -26,6 +26,8 @@ static int CookfsFsindexCmdGetBlockUsage(Cookfs_Fsindex *fsIndex, Tcl_Interp *in
 static int CookfsFsindexCmdChangeCount(Cookfs_Fsindex *fsIndex, Tcl_Interp *interp, int objc, Tcl_Obj *const objv[]);
 static int CookfsFsindexCmdImport(Cookfs_Fsindex *fsIndex, Tcl_Interp *interp, int objc, Tcl_Obj *const objv[]);
 
+static void CookfsRegisterExistingFsindexObjectCmd(Tcl_Interp *interp, Cookfs_Fsindex *i);
+
 // These functions are in header
 //static int CookfsFsindexCmdSetMetadata(Cookfs_Fsindex *fsIndex, Tcl_Interp *interp, int objc, Tcl_Obj *const objv[]);
 //static int CookfsFsindexCmdGetMetadata(Cookfs_Fsindex *fsIndex, Tcl_Interp *interp, int objc, Tcl_Obj *const objv[]);
@@ -99,7 +101,7 @@ Tcl_Obj *CookfsGetFsindexObjectCmd(Tcl_Interp *interp, Cookfs_Fsindex *i) {
  *----------------------------------------------------------------------
  */
 
-void CookfsRegisterExistingFsindexObjectCmd(Tcl_Interp *interp, Cookfs_Fsindex *i) {
+static void CookfsRegisterExistingFsindexObjectCmd(Tcl_Interp *interp, Cookfs_Fsindex *i) {
     if (i->commandToken != NULL) {
         return;
     }
@@ -109,7 +111,6 @@ void CookfsRegisterExistingFsindexObjectCmd(Tcl_Interp *interp, Cookfs_Fsindex *
     i->commandToken = Tcl_CreateObjCommand(interp, buf, CookfsFsindexCmd,
         (ClientData) i, CookfsFsindexDeleteProc);
     i->interp = interp;
-    Tcl_SetObjResult(interp, Tcl_NewStringObj(buf, -1));
 }
 
 
@@ -165,7 +166,7 @@ static int CookfsRegisterFsindexObjectCmd(ClientData clientData, Tcl_Interp *int
 
     /* create Tcl command and return its name and set interp result to the command name */
     CookfsLog(printf("Create Tcl command for the fsindex object..."))
-    CookfsRegisterExistingFsindexObjectCmd(interp, i);
+    Tcl_SetObjResult(interp, CookfsGetFsindexObjectCmd(interp, i));
     return TCL_OK;
 
 ERROR:

generic/fsindexCmd.h

@@ -7,10 +7,8 @@
 #ifdef COOKFS_USECFSINDEX
 
 int Cookfs_InitFsindexCmd(Tcl_Interp *interp);
-void CookfsRegisterExistingFsindexObjectCmd(Tcl_Interp *interp, Cookfs_Fsindex *i);
 Tcl_Obj *CookfsGetFsindexObjectCmd(Tcl_Interp *interp, Cookfs_Fsindex *i);
 
-
 int CookfsFsindexCmdGetMetadata(Cookfs_Fsindex *fsIndex, Tcl_Interp *interp, int objc, Tcl_Obj *const objv[]);
 int CookfsFsindexCmdSetMetadata(Cookfs_Fsindex *fsIndex, Tcl_Interp *interp, int objc, Tcl_Obj *const objv[]);
 

generic/pages.c

@@ -880,13 +880,15 @@ int Cookfs_PageAddRaw(Cookfs_Pages *p, unsigned char *bytes, int objLength) {
 
 	    /* use -1000 weight as it is temporary page and we don't really need it in cache */
 	    otherPageData = Cookfs_PageGet(p, idx, -1000);
+	    // No need to incr refcounter on otherPageData because Cookfs_PageGet()
+	    // always returns pages with refcount=1.
+
 	    /* fail in case when decompression is not available */
 	    if (otherPageData == NULL) {
 		CookfsLog(printf("Cookfs_PageAdd: Unable to verify page with same MD5 checksum"))
 		return -1;
 	    }
 
-	    Tcl_IncrRefCount(otherPageData);
 	    otherBytes = Tcl_GetByteArrayFromObj(otherPageData, &otherObjLength);
 
 	    /* if page with same checksum was found, verify its contents as we
@@ -993,7 +995,7 @@ Tcl_Obj *Cookfs_PageGet(Cookfs_Pages *p, int index, int weight) {
     /* if cache is disabled, immediately get page */
     if (p->cacheSize <= 0) {
 	rc = CookfsPagesPageGetInt(p, index);
-	CookfsLog(printf("Returning directly [%s]", rc == NULL ? "NULL" : "SET"))
+	CookfsLog(printf("Cookfs_PageGet: Returning directly [%p]", (void *)rc))
 	return rc;
     }
 
@@ -1007,17 +1009,21 @@ Tcl_Obj *Cookfs_PageGet(Cookfs_Pages *p, int index, int weight) {
 
     rc = Cookfs_PageCacheGet(p, index, 1, weight);
     if (rc != NULL) {
+	CookfsLog(printf("Cookfs_PageGet: Returning from cache [%p]", (void *)rc));
+        // Increase refcount by 1 for pages from cache because
+        // CookfsPagesPageGetInt()->Cookfs_ReadPage() returns pages with
+        // refcount=1
+	Tcl_IncrRefCount(rc);
 	return rc;
     }
 
     /* get page and store it in cache */
     rc = CookfsPagesPageGetInt(p, index);
-    CookfsLog(printf("Returning and caching [%s]", rc == NULL ? "NULL" : "SET"))
-    if (rc == NULL) {
-	return NULL;
-    }
+    CookfsLog(printf("Cookfs_PageGet: Returning and caching [%p]", (void *)rc))
 
-    Cookfs_PageCacheSet(p, index, rc, weight);
+    if (rc != NULL) {
+        Cookfs_PageCacheSet(p, index, rc, weight);
+    }
 
     return rc;
 }
@@ -1156,6 +1162,7 @@ void Cookfs_PageCacheSet(Cookfs_Pages *p, int idx, Tcl_Obj *obj, int weight) {
     p->cache[newIdx].pageIdx = idx;
     p->cache[newIdx].pageObj = obj;
     p->cache[newIdx].weight = weight;
+    Tcl_IncrRefCount(obj);
     CookfsLog(printf("Cookfs_PageCacheSet: replace entry [%d]", newIdx));
     /* age will be set by CookfsPagesPageCacheMoveToTop */
     CookfsPagesPageCacheMoveToTop(p, newIdx);
@@ -1898,8 +1905,9 @@ static int CookfsReadIndex(Tcl_Interp *interp, Cookfs_Pages *p) {
 indexReadOk:
 
     /* read page MD5 checksums and pages */
+    Tcl_DecrRefCount(p->dataIndex);
     p->dataIndex = buffer;
-    Tcl_IncrRefCount(p->dataIndex);
+    // Do not increase refcount for p->dataIndex because Cookfs_ReadPage returns Tcl_Obj with refcount=1
 
     /* seek to beginning of data, depending on if foffset was specified */
     Tcl_Seek(p->fileChannel, p->foffset, SEEK_SET);

generic/pagesCmd.c

@@ -14,7 +14,7 @@ static int CookfsPagesCmd(ClientData clientData, Tcl_Interp *interp, int objc, T
 static int CookfsPagesCmdHash(Cookfs_Pages *pages, Tcl_Interp *interp, int objc, Tcl_Obj *const objv[]);
 static void CookfsPagesDeleteProc(ClientData clientData);
 static int CookfsRegisterPagesObjectCmd(ClientData clientData, Tcl_Interp *interp, int objc, Tcl_Obj *const objv[]);
-
+static void CookfsRegisterExistingPagesObjectCmd(Tcl_Interp *interp, Cookfs_Pages *p);
 
 /*
  *----------------------------------------------------------------------
@@ -65,6 +65,7 @@ Tcl_Obj *CookfsGetPagesObjectCmd(Tcl_Interp *interp, Cookfs_Pages *p) {
     CookfsRegisterExistingPagesObjectCmd(interp, p);
     Tcl_Obj *rc = Tcl_NewObj();
     Tcl_GetCommandFullName(interp, p->commandToken, rc);
+    CookfsLog(printf("CookfsGetPagesObjectCmd: return [%p]", (void *)rc));
     return rc;
 }
 
@@ -85,7 +86,7 @@ Tcl_Obj *CookfsGetPagesObjectCmd(Tcl_Interp *interp, Cookfs_Pages *p) {
  *----------------------------------------------------------------------
  */
 
-void CookfsRegisterExistingPagesObjectCmd(Tcl_Interp *interp, Cookfs_Pages *p) {
+static void CookfsRegisterExistingPagesObjectCmd(Tcl_Interp *interp, Cookfs_Pages *p) {
     if (p->commandToken != NULL) {
         return;
     }
@@ -95,7 +96,6 @@ void CookfsRegisterExistingPagesObjectCmd(Tcl_Interp *interp, Cookfs_Pages *p) {
     p->commandToken = Tcl_CreateObjCommand(interp, buf, CookfsPagesCmd,
         (ClientData)p, CookfsPagesDeleteProc);
     p->interp = interp;
-    Tcl_SetObjResult(interp, Tcl_NewStringObj(buf, -1));
 }
 
 /* command for creating new objects that deal with pages */
@@ -299,7 +299,7 @@ static int CookfsRegisterPagesObjectCmd(ClientData clientData, Tcl_Interp *inter
 
     /* create Tcl command and return its name and set interp result to the command name */
     CookfsLog(printf("Create Tcl command for the pages object..."))
-    CookfsRegisterExistingPagesObjectCmd(interp, pages);
+    Tcl_SetObjResult(interp, CookfsGetPagesObjectCmd(interp, pages));
     return TCL_OK;
 
 ERROR:
@@ -402,6 +402,9 @@ static int CookfsPagesCmd(ClientData clientData, Tcl_Interp *interp, int objc, T
                 return TCL_ERROR;
             }  else  {
                 Tcl_SetObjResult(interp, rc);
+                // Cookfs_PageGet always returns a page with refcount=1. We need
+                // to decrease refcount now.
+                Tcl_DecrRefCount(rc);
             }
             break;
         }

generic/pagesCmd.h

@@ -9,7 +9,6 @@
 #ifdef COOKFS_USECPAGES
 
 int Cookfs_InitPagesCmd(Tcl_Interp *interp);
-void CookfsRegisterExistingPagesObjectCmd(Tcl_Interp *interp, Cookfs_Pages *p);
 Tcl_Obj *CookfsGetPagesObjectCmd(Tcl_Interp *interp, Cookfs_Pages *p);
 
 int CookfsPagesCmdCompression(Cookfs_Pages *pages, Tcl_Interp *interp, int objc, Tcl_Obj *const objv[]);

generic/pagesCompr.c

@@ -283,6 +283,13 @@ void Cookfs_PagesFiniCompr(Cookfs_Pages *rc) {
 	    }
 	    ckfree((void *) rc->asyncCompressCommandPtr);
 	}
+	if (rc->asyncDecompressCommandPtr != NULL) {
+	    Tcl_Obj **ptr;
+	    for (ptr = rc->asyncDecompressCommandPtr; *ptr; ptr++) {
+		Tcl_DecrRefCount(*ptr);
+	    }
+	    ckfree((void *) rc->asyncDecompressCommandPtr);
+	}
     }
 #ifdef COOKFS_USEXZ
     CookfsLog(printf("Cookfs_PagesFiniCompr: free xz resources"));

generic/readerchannelIO.c

@@ -95,9 +95,6 @@ int Cookfs_Readerchannel_Input(ClientData instanceData, char *buf, int bufSize,
 	    goto error;
 	}
 
-	CookfsLog(printf("Cookfs_Readerchannel_Input: before incr"))
-	Tcl_IncrRefCount(pageObj);
-	CookfsLog(printf("Cookfs_Readerchannel_Input: after incr"))
 	pageBuf = (char *) Tcl_GetByteArrayFromObj(pageObj, &pageBufSize);
 
 	CookfsLog(printf("Cookfs_Readerchannel_Input: copying %d+%d", instData->buf[instData->currentBlock + 1], instData->currentBlockOffset))
@@ -106,9 +103,7 @@ int Cookfs_Readerchannel_Input(ClientData instanceData, char *buf, int bufSize,
 	    goto error;
 	}
 	memcpy(buf + bytesRead, pageBuf + instData->buf[instData->currentBlock + 1] + instData->currentBlockOffset, blockRead);
-	CookfsLog(printf("Cookfs_Readerchannel_Input: before decr"))
 	Tcl_DecrRefCount(pageObj);
-	CookfsLog(printf("Cookfs_Readerchannel_Input: after decr"))
 
 	instData->currentBlockOffset += blockRead;
 	bytesRead += blockRead;

generic/vfsCmd.c

@@ -1090,6 +1090,8 @@ static int CookfsMountHandleCommandOptimizelist(Cookfs_Vfs *vfs,
         }
     }
 
+    //ckfree(pageFiles);
+
     CookfsLog(printf("CookfsMountHandleCommandOptimizelist: add the large"
         " files to the small files"));
     Tcl_ListObjAppendList(interp, smallFiles, largeFiles);