Tcl Source Code

I think there are two potential Tcl_Obj leaks in the current version
of tclPathObj.c in the TclJoinPath function, as shown in the following
patch.

The first in a newly introduced code path which releases a Tcl_Obj
although it gets used later, the second regarding the path name
separator returned as an optional new Tcl_Obj from a VFS implementation
which never gets released.

Index: generic/tclPathObj.c
==================================================================
--- generic/tclPathObj.c
+++ generic/tclPathObj.c
@@ -916,22 +916,27 @@
 		     * cannot have backslashes either.
 		     */
 
 		    if ((tclPlatform != TCL_PLATFORM_WINDOWS)
 			    || (strchr(Tcl_GetString(elt), '\\') == NULL)) {
-			if (res != NULL) {
-			    TclDecrRefCount(res);
-			}
-
 			if (PATHFLAGS(elt)) {
+			    if (res != NULL) {
+				TclDecrRefCount(res);
+			    }
 			    return TclNewFSPathObj(elt, str, len);
 			}
 			if (TCL_PATH_ABSOLUTE != Tcl_FSGetPathType(elt)) {
+			    if (res != NULL) {
+				TclDecrRefCount(res);
+			    }
 			    return TclNewFSPathObj(elt, str, len);
 			}
 			(void) Tcl_FSGetNormalizedPath(NULL, elt);
 			if (elt == PATHOBJ(elt)->normPathPtr) {
+			    if (res != NULL) {
+				TclDecrRefCount(res);
+			    }
 			    return TclNewFSPathObj(elt, str, len);
 			}
 		    }
 		}
 
@@ -1085,10 +1090,11 @@
 	    if (fsPtr->filesystemSeparatorProc != NULL) {
 		Tcl_Obj *sep = fsPtr->filesystemSeparatorProc(res);
 
 		if (sep != NULL) {
 		    separator = TclGetString(sep)[0];
+		    Tcl_DecrRefCount(sep);
 		}
 		/* Safety check in case the VFS driver caused sharing */
 		if (Tcl_IsShared(res)) {
 		    TclDecrRefCount(res);
 		    res = Tcl_DuplicateObj(res);

==================================================================

Revised the fix on the 8.6 branch; will merge fwd.

Having said that, it's worth noting that the
codebases of the 8.5 and 8.6 filesystem implementations
have diverged, and I lack much confidence that
a patch developed for one can lightly be applied
to the other.

I'd be happier leaving 8.5 untouched, and if that
leaves unfixed bugs, then the fix for them is to
start using 8.6.

I think that would be a further improvement.
Will look into it.

The code is messy, fragile, and abandoned
by the originator, so it improves only in
tiny, tentative incremental changes, usually
motivated by actual bugs and crashes, not
just aesthetic concerns.

Someone who really got up a head of steam
to make it much much better would be better
off rewriting the thing from the ground up,
IMO.

That said, the memleak in the separator
handling from a VFS looks legit and 
properly fixed.

> That means we can only execute this block on the first iteration of the for loop

Hmm, strange... The question is "Why this belong then to the cycle, and not above it?"
I fixed it just by the way like "minimalistic" surgery, so don't observed the cycle conditions.

This large conditional block

https://core.tcl.tk/tcl/artifact/6c6428a1c565405c?ln=877-960

is conditioned on (i == 0) among others.

That means we can only execute this block on
the first iteration of the for loop, and at
that point, nothing has had the chance to make
res contain anything other than NULL.  Much
code directing what to do when (res != NULL)
is actually useless.  Some asserts would be better,
I think.

Still looking, but it appears this is tidying
up some code that cannot actually ever be reached.

I think we'd be better eliminating it.

Any test cases?

Fixed for 8.6 and 8.5.
Thanks.