MagickGetImageLength is 0 after cloning MagickWand

[mkoppanen]

Hello,

I've encountered issue where MagickGetImageLength seems to return 0 after cloning MagickWand. The problem seems to be here:

Code: Select all

Index: MagickCore/image.c
===================================================================
--- MagickCore/image.c	(revision 10647)
+++ MagickCore/image.c	(working copy)
@@ -860,11 +860,7 @@
   if (detach == MagickFalse)
     clone_image->blob=ReferenceBlob(image->blob);
   else
-    {
-      clone_image->next=NewImageList();
-      clone_image->previous=NewImageList();
-      clone_image->blob=CloneBlobInfo((BlobInfo *) NULL);
-    }
+    clone_image->blob=CloneBlobInfo(image->blob);
   clone_image->ping=image->ping;
   clone_image->debug=IsEventLogging();
   clone_image->semaphore=AllocateSemaphoreInfo();

But I am not sure what other implications this patch would have. I wasn't sure why the clone method doesn't clone the actual blob info but rather generates new.

[magick]

Cloning is at the very heart of ImageMagick. Bad, bad things can happen if changes are made in this method. Note that CloneImage() has a detach parameter. If that detach parameter is MagickFalse, the blob is referenced rather than detached which would ensure MagickGetImageLength() returns a non-zero value. The question is, is there a cloning process in MagickWand where this parameter is currently MagickTrue when it should be MagickFalse? Another option is to note the value of MagickGetImageLength() before its cloned. That is, read an image, set a variable such as length = MagickGetImageLength() before you continue with processing options that will likely clone the image causing MagickGetImageLength() to return 0.

[mkoppanen]

Hello,

it seems that CloneMagickWand calls

clone_wand->images=CloneImageList(wand->images,clone_wand->exception);

and CloneImageList calls

clone=CloneImage(images,0,0,MagickTrue,exception);

which causes MagickTrue to be the parameter for detach. I will work on reproduce code and will post soon.

[mkoppanen]

Hello,

as promised, here is the reproduce code:

Code: Select all

#include <stdio.h>
#include <stdlib.h>
#include <assert.h>
#include <wand/MagickWand.h>

#define ThrowWandException(wand) { \
	ExceptionType severity;\
    char *description=MagickGetException(wand,&severity);\
    (void) fprintf(stderr,"%s %s %lu %s\n",GetMagickModule(),description); \
    description=(char *) MagickRelinquishMemory(description);\
    exit(1);\
  }

int main (int argc, char **argv)
{
    MagickBooleanType status;
    MagickWand *wand, *clone_wand;

	MagickSizeType length, clone_length;

    MagickWandGenesis ();
    wand = NewMagickWand();

    if (MagickReadImage (wand, argv[1]) == MagickFalse)
        ThrowWandException (wand);

	if (MagickGetImageLength (wand, &length) == MagickFalse)
		ThrowWandException (wand);
		
	clone_wand = CloneMagickWand (wand);
		
	if (MagickGetImageLength (clone_wand, &clone_length) == MagickFalse)
		ThrowWandException (clone_wand);	

	printf ("%ld == %ld\n", length, clone_length);

    assert (length == clone_length);

    DestroyMagickWand (wand);
    DestroyMagickWand (clone_wand);

    MagickWandTerminus ();
    return 0;
}

[magick]

This is the expected behavior. A clone detaches the blob from the source image list and therefore reports a zero length since the blob is no longer associated with a cloned image list. The question, as mentioned, is detaching the source blob necessary. We could just reference it, but historically we found that not detaching the blob caused problems although we cannot no longer recall what those problems were. The fix is to save the image length from the original source image before the clone.

[mkoppanen]

What I see problematic here is the assumption that user might have "CloneMagickWand() makes an exact copy of the specified wand." Maybe the documentation should be updated to reflect that it's not 1:1 copy of the original Wand (?)