Memory error while using memcpy?

Question

I'm using dcmtk library to modify the pixel data of a multi frame compressed dicom image. So, to do that, at one stage in an for loop I take the pixel data of each decompressed frame and modify them according my wish and try to concatenate each modify pixel data in a big memory buffer frame by frame. This core process of for loop is as below.

The problem is after the first iteration it gives memory at the line of the code where I call the function getUncompressedFrame. I think it's happening because of the line memcpy(fullBuffer+(i*sizeF),newBuffer,sizeF);, as when I remove that line there's no error at that time and the whole for loop works absolutely fine.

Could you please say me if I'm making a mistake in working with memcpy? Thanks.

Uint32 sizeF=828072;// I just wrote it to show what is the data type. 
Uint8 * fullBuffer = new Uint8(int(sizeF*numOfFrames));//The big memory buffer
for(int i=0;i<numOfFrames;i++)
{
    Uint8 * buffer = new Uint8[int(sizeF)];//Buffer for each frame
    Uint8 * newBuffer = new Uint8[int(sizeF)];//Buffer in which the modified frame data is stored 
    DcmFileCache * cache=NULL;
    OFCondition cond=element->getUncompressedFrame(dataset,i,startFragment,buffer,sizeF,decompressedColorModel,cache);
    //I get the uncompressed individual frame pixel data 
    if(buffer != NULL)
    {
        for(unsigned long y = 0; y < rows; y++)
        {
            for(unsigned long x = 0; x < cols; x++)
            {
                if(planarConfiguration==0)
                {
                    if(x>xmin && x<xmax && y>ymin && y<ymax)
                    {
                        index=(x + y +  y*(cols-1))*samplePerPixel;
                        if(index<sizeF-2)
                        {
                            newBuffer[index]  = 0;
                            newBuffer[index + 1]  = 0;
                            newBuffer[index +2]  = 0;
                        }
                    }
                    else
                    {
                        index=(x + y +  y*(cols-1))*samplePerPixel;
                        if(index<sizeF-2)
                        {
                            newBuffer[index]  = buffer[index];
                            newBuffer[index + 1]  = buffer[index + 1];
                            newBuffer[index + 2]  = buffer[index + 2];
                        }
                    }
                }
            }
        }
        memcpy(fullBuffer+(i*sizeF),newBuffer,sizeF);
        //concatenate the modified frame by frame pixel data
    }                   

Vinnie

Run in a debugger, if it stops there then examine the values of i and sizeF to see if they look okay. If it doesn't stop, then set a breakpoint. – Translator

I have run the debugger, i and sizeF gives the appropriate number. The program works fine I don't use the line of code with memcpy, but if I use it, the program breaks. – Vinnie

what's the purpose of that whole nested x y loop? and BTW, instead of 'index=(x + y + y*(cols-1))*samplePerPixel;' you could do why do you use 'index=(x + y*(cols))*samplePerPixel;', right? – Drews

Uint32 sizeF=0;???????? you are allocating 0 size? – Raving

Sorry, I was just trying to show the data type of the sizeF, I do get the value of the sizeF from another method, it returns something like 828072. :) – Vinnie

Please do not "fix" the error in the question. That makes it impossible to compare your original code to the suggestion(s) in the answer(s). – Tsarevitch

Okay, thanks for reminding it :). – Vinnie

Answer 1

Change the declaration of fullBuffer to this:

Uint8 * fullBuffer = new Uint8[int(sizeF*numOfFrames)];

Your code didn't allocate an array, it allocated a single Uint8 with the value int(sizeF*numOfFrames).

Papiamento

Yes, that was big big mistake, Ah! my bad. Thanks! I will try now. I guess my I eyes deceived me. – Vinnie

Answer 2

Uint8 * fullBuffer = new Uint8(int(sizeF*numOfFrames));

This allocates a single byte, giving it an initial value of sizeF*numOfFrames (after truncating it first to int and then to Uint8). You want an array, and you don't want to truncate the size to int:

Uint8 * fullBuffer = new Uint8[sizeF*numOfFrames];
                              ^                 ^

or, to fix the likely memory leaks in your code:

std::vector<Uint8> fullBuffer(sizeF*numOfFrames);

Valve

Answer 3

If the method getUncompressedFrame is doing an inner memcpy to cache, then it makes sense why, as you are passing a null pointer as argument for the cache, with no memory allocated.

Wheelbarrow