Fix [e4336bef5d58cc96a438ba0fb5553ea57e94f4d8|e4336bef5d]: Unexpected result when copying a photo image to itself. Thanks to Simon Bachmann for bug report and fix!

3 files changed, 195 insertions, 47 deletions

diff --git a/generic/tkImgPhoto.c b/generic/tkImgPhoto.c
index 1bd0142..1fec208 100644
--- a/generic/tkImgPhoto.c
+++ b/generic/tkImgPhoto.c
@@ -2698,7 +2698,7 @@ Tk_PhotoPutBlock(
 				 * messages, or NULL. */
     Tk_PhotoHandle handle,	/* Opaque handle for the photo image to be
 				 * updated. */
-    register Tk_PhotoImageBlock *blockPtr,
+    Tk_PhotoImageBlock *blockPtr,
 				/* Pointer to a structure describing the pixel
 				 * data to be copied into the image. */
     int x, int y,		/* Coordinates of the top-left pixel to be
@@ -2709,6 +2709,8 @@ Tk_PhotoPutBlock(
 				 * transparent pixels. */
 {
     register PhotoMaster *masterPtr = (PhotoMaster *) handle;
+    Tk_PhotoImageBlock sourceBlock;
+    unsigned char *memToFree;
     int xEnd, yEnd, greenOffset, blueOffset, alphaOffset;
     int wLeft, hLeft, wCopy, hCopy, pitch;
     unsigned char *srcPtr, *srcLinePtr, *destPtr, *destLinePtr;
@@ -2736,11 +2738,43 @@ Tk_PhotoPutBlock(
 	return TCL_OK;
     }
 
+    /*
+     * Fix for bug e4336bef5d:
+     *
+     * Make a local copy of *blockPtr, as we might have to change some
+     * of its fields and don't want to interfere with the caller's data.
+     *
+     * If source and destination are the same image, create a copy  of the
+     * source data in our local sourceBlock.
+     *
+     * To find out, just comparing the pointers is not enough - they might have
+     * different values and still point to the same block of memory. (e.g.
+     * if the -from option was passed to [imageName copy])
+     */
+    sourceBlock = *blockPtr;
+    memToFree = NULL;
+    if (sourceBlock.pixelPtr >= masterPtr->pix32
+	    && sourceBlock.pixelPtr <= masterPtr->pix32 + masterPtr->width
+	    * masterPtr->height * 4) {
+	sourceBlock.pixelPtr = attemptckalloc(sourceBlock.height
+		* sourceBlock.pitch);
+	if (sourceBlock.pixelPtr == NULL) {
+	    if (interp != NULL) {
+		Tcl_SetObjResult(interp, Tcl_NewStringObj(
+			TK_PHOTO_ALLOC_FAILURE_MESSAGE, -1));
+		Tcl_SetErrorCode(interp, "TK", "MALLOC", NULL);
+	    }
+	    return TCL_ERROR;
+	}
+	memToFree = sourceBlock.pixelPtr;
+	memcpy(sourceBlock.pixelPtr, blockPtr->pixelPtr, sourceBlock.height
+	    * sourceBlock.pitch);
+    }
+
+
     xEnd = x + width;
     yEnd = y + height;
     if ((xEnd > masterPtr->width) || (yEnd > masterPtr->height)) {
-	int sameSrc = (blockPtr->pixelPtr == masterPtr->pix32);
-
 	if (ImgPhotoSetSize(masterPtr, MAX(xEnd, masterPtr->width),
 		MAX(yEnd, masterPtr->height)) == TCL_ERROR) {
 	    if (interp != NULL) {
@@ -2748,11 +2782,7 @@ Tk_PhotoPutBlock(
 			TK_PHOTO_ALLOC_FAILURE_MESSAGE, -1));
 		Tcl_SetErrorCode(interp, "TK", "MALLOC", NULL);
 	    }
-	    return TCL_ERROR;
-	}
-	if (sameSrc) {
-	    blockPtr->pixelPtr = masterPtr->pix32;
-	    blockPtr->pitch = masterPtr->width * 4;
+	    goto errorExit;
 	}
     }
 
@@ -2771,14 +2801,14 @@ Tk_PhotoPutBlock(
      * components, mark it as a color image.
      */
 
-    greenOffset = blockPtr->offset[1] - blockPtr->offset[0];
-    blueOffset = blockPtr->offset[2] - blockPtr->offset[0];
-    alphaOffset = blockPtr->offset[3];
-    if ((alphaOffset >= blockPtr->pixelSize) || (alphaOffset < 0)) {
+    greenOffset = sourceBlock.offset[1] - sourceBlock.offset[0];
+    blueOffset = sourceBlock.offset[2] - sourceBlock.offset[0];
+    alphaOffset = sourceBlock.offset[3];
+    if ((alphaOffset >= sourceBlock.pixelSize) || (alphaOffset < 0)) {
 	alphaOffset = 0;
 	sourceIsSimplePhoto = 1;
     } else {
-	alphaOffset -= blockPtr->offset[0];
+	alphaOffset -= sourceBlock.offset[0];
     }
     if ((greenOffset != 0) || (blueOffset != 0)) {
 	masterPtr->flags |= COLOR_IMAGE;
@@ -2798,13 +2828,13 @@ Tk_PhotoPutBlock(
      * pixelSize == 3 and alphaOffset == 0. Maybe other cases too.
      */
 
-    if ((blockPtr->pixelSize == 4)
+    if ((sourceBlock.pixelSize == 4)
 	    && (greenOffset == 1) && (blueOffset == 2) && (alphaOffset == 3)
-	    && (width <= blockPtr->width) && (height <= blockPtr->height)
+	    && (width <= sourceBlock.width) && (height <= sourceBlock.height)
 	    && ((height == 1) || ((x == 0) && (width == masterPtr->width)
-		&& (blockPtr->pitch == pitch)))
+		&& (sourceBlock.pitch == pitch)))
 	    && (compRule == TK_PHOTO_COMPOSITE_SET)) {
-	memmove(destLinePtr, blockPtr->pixelPtr + blockPtr->offset[0],
+	memmove(destLinePtr, sourceBlock.pixelPtr + sourceBlock.offset[0],
 		((size_t)height * width * 4));
 
 	/*
@@ -2820,11 +2850,11 @@ Tk_PhotoPutBlock(
      */
 
     for (hLeft = height; hLeft > 0;) {
-	int pixelSize = blockPtr->pixelSize;
+	int pixelSize = sourceBlock.pixelSize;
 	int compRuleSet = (compRule == TK_PHOTO_COMPOSITE_SET);
 
-	srcLinePtr = blockPtr->pixelPtr + blockPtr->offset[0];
-	hCopy = MIN(hLeft, blockPtr->height);
+	srcLinePtr = sourceBlock.pixelPtr + sourceBlock.offset[0];
+	hCopy = MIN(hLeft, sourceBlock.height);
 	hLeft -= hCopy;
 	for (; hCopy > 0; --hCopy) {
 	    /*
@@ -2835,10 +2865,10 @@ Tk_PhotoPutBlock(
 
 	    if ((pixelSize == 4) && (greenOffset == 1)
 		    && (blueOffset == 2) && (alphaOffset == 3)
-		    && (width <= blockPtr->width)
+		    && (width <= sourceBlock.width)
 		    && compRuleSet) {
 		memcpy(destLinePtr, srcLinePtr, ((size_t)width * 4));
-		srcLinePtr += blockPtr->pitch;
+		srcLinePtr += sourceBlock.pitch;
 		destLinePtr += pitch;
 		continue;
 	    }
@@ -2849,7 +2879,7 @@ Tk_PhotoPutBlock(
 
 	    destPtr = destLinePtr;
 	    for (wLeft = width; wLeft > 0;) {
-		wCopy = MIN(wLeft, blockPtr->width);
+		wCopy = MIN(wLeft, sourceBlock.width);
 		wLeft -= wCopy;
 		srcPtr = srcLinePtr;
 
@@ -2939,7 +2969,7 @@ Tk_PhotoPutBlock(
 		    destPtr += 4;
 		}
 	    }
-	    srcLinePtr += blockPtr->pitch;
+	    srcLinePtr += sourceBlock.pitch;
 	    destLinePtr += pitch;
 	}
     }
@@ -3055,7 +3085,15 @@ Tk_PhotoPutBlock(
 
     Tk_ImageChanged(masterPtr->tkMaster, x, y, width, height,
 	    masterPtr->width, masterPtr->height);
+
+    if (memToFree) ckfree(memToFree);
+
     return TCL_OK;
+
+  errorExit:
+    if (memToFree) ckfree(memToFree);
+
+    return TCL_ERROR;
 }
 
 /*
@@ -3082,7 +3120,7 @@ Tk_PhotoPutZoomedBlock(
 				 * messages, or NULL. */
     Tk_PhotoHandle handle,	/* Opaque handle for the photo image to be
 				 * updated. */
-    register Tk_PhotoImageBlock *blockPtr,
+    Tk_PhotoImageBlock *blockPtr,
 				/* Pointer to a structure describing the pixel
 				 * data to be copied into the image. */
     int x, int y,		/* Coordinates of the top-left pixel to be
@@ -3097,6 +3135,8 @@ Tk_PhotoPutZoomedBlock(
 				 * transparent pixels. */
 {
     register PhotoMaster *masterPtr = (PhotoMaster *) handle;
+    register Tk_PhotoImageBlock sourceBlock;
+    unsigned char *memToFree;
     int xEnd, yEnd, greenOffset, blueOffset, alphaOffset;
     int wLeft, hLeft, wCopy, hCopy, blockWid, blockHt;
     unsigned char *srcPtr, *srcLinePtr, *srcOrigPtr, *destPtr, *destLinePtr;
@@ -3133,11 +3173,41 @@ Tk_PhotoPutZoomedBlock(
 	return TCL_OK;
     }
 
+    /*
+     * Fix for Bug e4336bef5d:
+     * Make a local copy of *blockPtr, as we might have to change some
+     * of its fields and don't want to interfere with the caller's data.
+     *
+     * If source and destination are the same image, create a copy  of the
+     * source data in our local sourceBlock.
+     *
+     * To find out, just comparing the pointers is not enough - they might have
+     * different values and still point to the same block of memory. (e.g.
+     * if the -from option was passed to [imageName copy])
+     */
+    sourceBlock = *blockPtr;
+    memToFree = NULL;
+    if (sourceBlock.pixelPtr >= masterPtr->pix32
+	    && sourceBlock.pixelPtr <= masterPtr->pix32 + masterPtr->width
+	    * masterPtr->height * 4) {
+	sourceBlock.pixelPtr = attemptckalloc(sourceBlock.height
+		* sourceBlock.pitch);
+	if (sourceBlock.pixelPtr == NULL) {
+	    if (interp != NULL) {
+		Tcl_SetObjResult(interp, Tcl_NewStringObj(
+			TK_PHOTO_ALLOC_FAILURE_MESSAGE, -1));
+		Tcl_SetErrorCode(interp, "TK", "MALLOC", NULL);
+	    }
+	    return TCL_ERROR;
+	}
+	memToFree = sourceBlock.pixelPtr;
+	memcpy(sourceBlock.pixelPtr, blockPtr->pixelPtr, sourceBlock.height
+	    * sourceBlock.pitch);
+    }
+
     xEnd = x + width;
     yEnd = y + height;
     if ((xEnd > masterPtr->width) || (yEnd > masterPtr->height)) {
-	int sameSrc = (blockPtr->pixelPtr == masterPtr->pix32);
-
 	if (ImgPhotoSetSize(masterPtr, MAX(xEnd, masterPtr->width),
 		MAX(yEnd, masterPtr->height)) == TCL_ERROR) {
 	    if (interp != NULL) {
@@ -3145,11 +3215,7 @@ Tk_PhotoPutZoomedBlock(
 			TK_PHOTO_ALLOC_FAILURE_MESSAGE, -1));
 		Tcl_SetErrorCode(interp, "TK", "MALLOC", NULL);
 	    }
-	    return TCL_ERROR;
-	}
-	if (sameSrc) {
-	    blockPtr->pixelPtr = masterPtr->pix32;
-	    blockPtr->pitch = masterPtr->width * 4;
+	    goto errorExit;
 	}
     }
 
@@ -3168,14 +3234,14 @@ Tk_PhotoPutZoomedBlock(
      * components, mark it as a color image.
      */
 
-    greenOffset = blockPtr->offset[1] - blockPtr->offset[0];
-    blueOffset = blockPtr->offset[2] - blockPtr->offset[0];
-    alphaOffset = blockPtr->offset[3];
-    if ((alphaOffset >= blockPtr->pixelSize) || (alphaOffset < 0)) {
+    greenOffset = sourceBlock.offset[1] - sourceBlock.offset[0];
+    blueOffset = sourceBlock.offset[2] - sourceBlock.offset[0];
+    alphaOffset = sourceBlock.offset[3];
+    if ((alphaOffset >= sourceBlock.pixelSize) || (alphaOffset < 0)) {
 	alphaOffset = 0;
 	sourceIsSimplePhoto = 1;
     } else {
-	alphaOffset -= blockPtr->offset[0];
+	alphaOffset -= sourceBlock.offset[0];
     }
     if ((greenOffset != 0) || (blueOffset != 0)) {
 	masterPtr->flags |= COLOR_IMAGE;
@@ -3186,21 +3252,21 @@ Tk_PhotoPutZoomedBlock(
      * subsampling and zooming.
      */
 
-    blockXSkip = subsampleX * blockPtr->pixelSize;
-    blockYSkip = subsampleY * blockPtr->pitch;
+    blockXSkip = subsampleX * sourceBlock.pixelSize;
+    blockYSkip = subsampleY * sourceBlock.pitch;
     if (subsampleX > 0) {
-	blockWid = ((blockPtr->width + subsampleX - 1) / subsampleX) * zoomX;
+	blockWid = ((sourceBlock.width + subsampleX - 1) / subsampleX) * zoomX;
     } else if (subsampleX == 0) {
 	blockWid = width;
     } else {
-	blockWid = ((blockPtr->width - subsampleX - 1) / -subsampleX) * zoomX;
+	blockWid = ((sourceBlock.width - subsampleX - 1) / -subsampleX) * zoomX;
     }
     if (subsampleY > 0) {
-	blockHt = ((blockPtr->height + subsampleY - 1) / subsampleY) * zoomY;
+	blockHt = ((sourceBlock.height + subsampleY - 1) / subsampleY) * zoomY;
     } else if (subsampleY == 0) {
 	blockHt = height;
     } else {
-	blockHt = ((blockPtr->height - subsampleY - 1) / -subsampleY) * zoomY;
+	blockHt = ((sourceBlock.height - subsampleY - 1) / -subsampleY) * zoomY;
     }
 
     /*
@@ -3208,12 +3274,12 @@ Tk_PhotoPutZoomedBlock(
      */
 
     destLinePtr = masterPtr->pix32 + (y * masterPtr->width + x) * 4;
-    srcOrigPtr = blockPtr->pixelPtr + blockPtr->offset[0];
+    srcOrigPtr = sourceBlock.pixelPtr + sourceBlock.offset[0];
     if (subsampleX < 0) {
-	srcOrigPtr += (blockPtr->width - 1) * blockPtr->pixelSize;
+	srcOrigPtr += (sourceBlock.width - 1) * sourceBlock.pixelSize;
     }
     if (subsampleY < 0) {
-	srcOrigPtr += (blockPtr->height - 1) * blockPtr->pitch;
+	srcOrigPtr += (sourceBlock.height - 1) * sourceBlock.pitch;
     }
 
     pitch = masterPtr->width * 4;
@@ -3363,7 +3429,15 @@ Tk_PhotoPutZoomedBlock(
 
     Tk_ImageChanged(masterPtr->tkMaster, x, y, width, height, masterPtr->width,
 	    masterPtr->height);
+
+    if (memToFree) ckfree(memToFree);
+
     return TCL_OK;
+
+  errorExit:
+    if (memToFree) ckfree(memToFree);
+
+    return TCL_ERROR;
 }
 
 /*
diff --git a/tests/imgBmap.test b/tests/imgBmap.test
index 5ffd7c4..e7f2c7e 100644
--- a/tests/imgBmap.test
+++ b/tests/imgBmap.test
@@ -380,7 +380,8 @@ test imageBmap-7.9 {ImgBmapCmd procedure} -body {
 test imageBmap-7.10 {ImgBmapCmd procedure} -body {
     i1 gorp
 } -returnCodes error -result {bad option "gorp": must be cget or configure}
-
+# Clean it up after use!!
+imageCleanup
 
 test imageBmap-8.1 {ImgBmapGet/Free procedures, shared instances} -setup {
     destroy .c
diff --git a/tests/imgPhoto.test b/tests/imgPhoto.test
index f9ffa94..86da23d 100644
--- a/tests/imgPhoto.test
+++ b/tests/imgPhoto.test
@@ -953,6 +953,44 @@ test imgPhoto-10.1 {Tk_ImgPhotoPutBlock procedure} -setup {
     photo1 put "{#00ff00 #00ff00}" -to 2 0
     list [photo1 get 2 0] [photo1 get 3 0] [photo1 get 4 0]
 } -result {{0 255 0} {0 255 0} {255 0 0}}
+test imgPhoto-10.2 {Tk_ImgPhotoPutBlock, same source and dest img} -constraints {
+    hasTeapotPhoto
+} -setup {
+    imageCleanup
+} -body {
+    # Test for bug e4336bef5d
+    image create photo photo1 -file $teapotPhotoFile
+    image create photo photo2 -file $teapotPhotoFile
+    photo2 copy photo1 -to 1 2
+    photo1 copy photo1 -to 1 2
+    string equal [photo1 data] [photo2 data]
+} -cleanup {
+    imageCleanup
+} -result {1}
+test imgPhoto-10.3 {Tk_ImgPhotoPutBlock, same source and dest img} -constraints {
+    hasTeapotPhoto
+} -setup {
+    imageCleanup
+} -body {
+    # Test for bug e4336bef5d
+    image create photo photo1 -file $teapotPhotoFile
+    image create photo photo2 -file $teapotPhotoFile
+    photo2 copy photo1 -from 2 1 -to 4 5 300 300
+    photo1 copy photo1 -from 2 1 -to 4 5 300 300
+    string equal [photo1 data] [photo2 data]
+} -cleanup {
+    imageCleanup
+} -result {1}
+test imgPhoto-10.4 {Tk_ImgPhotoPutBlock, empty image} -setup {
+    imageCleanup
+} -body {
+    image create photo photo1
+    photo1 copy photo1 -to 0 5 10 20
+    list [image width photo1] [image height photo1]
+} -cleanup {
+    imageCleanup
+} -result {0 0}
+
 
 test imgPhoto-11.1 {Tk_FindPhoto} -setup {
     imageCleanup
@@ -972,6 +1010,41 @@ test imgPhoto-12.1 {Tk_PhotoPutZoomedBlock} -constraints hasTeapotPhoto -body {
 } -cleanup {
     image delete p3
 } -result {{19 92 192} {169 117 90} 512 512 {19 92 192}}
+test imgPhoto-12.2 {Tk_ImgPhotoPutZoomedBlock, same source and dest img} -constraints {
+    hasTeapotPhoto
+} -setup {
+    imageCleanup
+} -body {
+    # Test for bug e4336bef5d
+    image create photo photo1 -file $teapotPhotoFile
+    image create photo photo2 -file $teapotPhotoFile
+    photo2 copy photo1 -to 0 1 200 200 -zoom 2 3
+    photo1 copy photo1 -to 0 1 200 200 -zoom 2 3
+    string equal [photo1 data] [photo2 data]
+} -cleanup {
+    imageCleanup
+} -result {1}
+test imgPhoto-12.3 {Tk_ImgPhotoPutZoomedBlock, same source and dest img} -setup {
+    imageCleanup
+} -body {
+    # Test for bug e4336bef5d
+    image create photo photo1 -file $teapotPhotoFile
+    image create photo photo2 -file $teapotPhotoFile
+    photo2 copy photo1 -from 1 0 -to 4 5 300 300 -zoom 1 2
+    photo1 copy photo1 -from 1 0 -to 4 5 300 300 -zoom 1 2
+    string equal [photo1 data] [photo2 data]
+} -cleanup {
+    imageCleanup
+} -result {1}
+test imgPhoto-12.4 {Tk_ImgPhotoPutZoomedBlock, empty image} -setup {
+    imageCleanup
+} -body {
+    image create photo photo1
+    photo1 copy photo1 -to 0 5 10 20
+    list [image width photo1] [image height photo1]
+} -cleanup {
+    imageCleanup
+} -result {0 0}
 
 test imgPhoto-13.1 {check separation of images in different interpreters} -setup {
     imageCleanup