Memory leak in BlobToStringInfo()

[shenjoku]

Inside of BlobToStringInfo() it is calling AcquireStringInfo(0), and because it is passing zero there is a block of code that causes the datum member of the StringInfo struct to be allocated:

Code: Select all

if (~string_info->length >= (MaxTextExtent-1))
    string_info->datum=(unsigned char *) AcquireQuantumMemory(
      string_info->length+MaxTextExtent,sizeof(*string_info->datum));

Immediately after AcquireStringInfo() is called this exact same code is being executed. Since the length of the string blob is 5144, which is greater than the MaxTextExtent-1, it causes it to allocate the datum member again and overwrite with this new value without deallocating the previous value, causing a memory leak.

Shouldn't BlobToStringInfo() be passing the length parameter directly to AcquireStringInfo() so it can allocate the datum at the correct size instead of trying to allocate it itself? This would not only fix the memory leak but also get rid of some ugly copy-pasta. Additionally, the code that is checking whether or not the datum was allocated can be removed since AcquireStringInfo() already checks and throws an exception if it fails.

The current implementation of BlobToStringInfo() looks like this:

Code: Select all

MagickExport StringInfo *BlobToStringInfo(const void *blob,const size_t length)
{
  StringInfo
    *string_info;

  string_info=AcquireStringInfo(0);
  string_info->length=length;
  if (~string_info->length >= (MaxTextExtent-1))
    string_info->datum=(unsigned char *) AcquireQuantumMemory(
      string_info->length+MaxTextExtent,sizeof(*string_info->datum));
  if (string_info->datum == (unsigned char *) NULL)
    {
      string_info=DestroyStringInfo(string_info);
      return((StringInfo *) NULL);
    }
  if (blob != (const void *) NULL)
    (void) memcpy(string_info->datum,blob,length);
  return(string_info);
}

And with the proposed changes it would look like this:

Code: Select all

MagickExport StringInfo *BlobToStringInfo(const void *blob,const size_t length)
{
  StringInfo
    *string_info;

  string_info=AcquireStringInfo(length);
  if (blob != (const void *) NULL)
    (void) memcpy(string_info->datum,blob,length);
  return(string_info);
}

[magick]

We can reproduce the problem you posted and have a patch in ImageMagick 6.8.9-2 Beta, available within a few days. Thanks.

We do not call AcquireStringInfo() with the length parameter because if we exceed the memory-limits, AcquireStringInfo() exits with a fatal exception, whereas BlobToStringInfo() instead returns a NULL and that exception is recoverable.

[shenjoku]

Ah, I see. I'll pick up the patch later, thanks.