Changing from assignment operator "=" to memcpy for copying bytes resulting in segmentation fault

Question

I've written code to place a smaller BMP file (A) in a bigger BMP file (B) in Android Native Code.

Edit: more details on what fileA and fileB are like

//initialised as global variables
char * fileA;
char * fileB;

//file A and B are passed in to native C as byte arrays from Java
fileA=(char *) env->GetByteArrayElements(bufferA,&isCopy);
fileB=(char *) env->GetByteArrayElements(bufferB,&isCopy);

fileA consists of the BMP data from all BMPs of the 4 groups placed one after the other.

fileB is the BMP of the bigger file which I want to write on.

My initial implementation uses "=" assignment operator to copy byte by byte from file A to file B. Both are initialised as char arrays.

I had 4 groups of BMPs used for file A. BMPs of each group are the same size. Group 1 is 376 x 54, group 2 is 297 x 48, group 3 is 885 x 83 and group 4 is 238 x 108.

The simplified code to write 1 BMP (BMP X) from fileA into fileB.

//calculate offset on where to write in fileB
offset = ycoord_of_BMP_X * width_of_file_B + xcoord_of_BMP_X*BYTES_PER_PIXEL;
for (int a = 0; a < height_of_BMP_X; a++){
  for (int b = 0; b< width_of_BMP_X; b++){
     fileB[offset+ a*width_of_BMP_X + b] = fileA[position_of_BMP_X_in_file_A + (height_of_BMP_X-a-1)*width_of_BMP_X*BYTES_PER_PIXEL + b];  
  }
  offset = offset + width_of_fileB*BYTES_PER_PIXEL-width_of_BMP_X;
}

This works fine with no segmentation fault. Which leads me to conclude that the algorithm and its implementation is fine.

I replaced the inner for loop with memcpy and resulted in the following code (again simplified):

for (int a = 0; a < height_of_BMP_X; a++){
  memcpy(fileB[offset+ a*width_of_BMP_X],fileA[position_of_BMP_X_in_file_A + (height_of_BMP_X-a-1)*width_of_BMP_X*BYTES_PER_PIXEL],width_of_BMP_X*BYTES_PER_PIXEL]);

offset = offset + width_of_fileB*BYTES_PER_PIXEL-width_of_BMP_X;
}

Have to stress that I reference to fileA and fileB in both cases were almost identical (extracted below for clarity). I only removed the "+b" as it is no longer required for memcpy.

//the assignment case
fileB[offset+ a*width_of_BMP_X+b]

//memcpy case
fileB[offset+ a*width_of_BMP_X]

Segmentation fault (Fatal signal 11 (SIGSEGV), code 1 (SEGV_MAPERR)) occurs for group 3 and group 4 BMPs. Segmentation fault is reported when group 3 reaches a = 52. ie. memcpy is successful for first 52 out of 83 loops. For group 4, it is 92 out of 108 loops.

I printed out the array positions referenced in fileA and fileB arrays, for example values of "initial position x" for each iteration in the outer loop and it matches for both implementations.

Is the segmentation fault a result of some kind of optimisation done by memcpy, resulting in a race condition happening?

Thank you.

You no longer need memcpy for this You should use [std::copy](https://en.cppreference.com/w/cpp/algorithm/copy). Don't worry about performance the compiler will generate optimized (vectorized) code for you. The big advantage : type safety and more clear memory boundaries — Pepijn Kramer, Aug 13 '22 at 14:28
I can see no obvious problem with the code posted (other than a few typos) so I'm guessing the real problem has been obscured by your simplification.. It's always better to post real code. You don't have to post all your code, but enough for us to fully understand the code you do post. — john, Aug 13 '22 at 14:36
Without knowing what `FileA` and `FileB` are it's impossible to comment. But, generally speaking, `memcpy()` doesn't work for most C++ classes (e.g. with virtual functions, or non-trivial hand-rolled constructors/destructors). Unless you've specifically designed your types to be copyable with `memcpy()` (e.g. made your types POD) assume that using `memcpy()` gives undefined behaviour. Most C++ standard containers (`std::vector`, `std::string`, `std::list`, etc), or data structures containing them, CANNOT be safely copied with `memcpy()`. — Peter, Aug 13 '22 at 14:38
Thanks for the replies. I have added more details and hope it clarifies. memcpy works when the number of iterations is low (group 1 and 2) but segfault occurs when number of iterations is higher (group 3 and 4). the rest of the variables used in the referencing had been printed and it is the same for both implementations. So I really cannot see where the bug in my code is. appreciate everyone's help. thanks! — Ying Tat Ng, Aug 13 '22 at 15:17
memcpy expects pointers to the destination and source. Try `memcpy(&fileB[..], &fileA[..], ..);` — user5329483, Aug 13 '22 at 16:10

score 0 · Answer 1 · answered Aug 15 '22 at 18:22

Consider to use struct's to group the data of an image. An image's size is tied to the image data. Here a suggestion:

typedef struct
{
    int x,y;
} Coord_s;

typedef struct //Descriptor of an image
{
    void *  PixelData;
    Coord_s Size;
    int     BytePerLine; // = Size.x * BYTES_PER_PIXEL + possible padding
} Image_s;

void CopyImageRect(Image_s* pDst, Coord_s DstTopLeft, Image_s const* pSrc, Coord_s SrcTopLeft, Coord_s Size)
{
    //Copies a rectangle of <Size> from Image <pSrc>'s <SrcTopLeft> to <pDst>'s <DstTopLeft>

    //Prepare pointers to pixel data of top-left corner
    char* pSrcPix = (char*)pSrc->PixelData + SrcTopLeft.y*pSrc->BytePerLine + SrcTopLeft.x*BYTES_PER_PIXEL;
    char* pDstPix = (char*)pDst->PixelData + DstTopLeft.y*pDst->BytePerLine + DstTopLeft.x*BYTES_PER_PIXEL;

    for (int y = Size.y; y--;) //for each line
    {
        memcpy (pDstPix, pSrcPix, Size.x*BYTES_PER_PIXEL);
        pSrcPix += pSrc->BytePerLine; //next line
        pDstPix += pDst->BytePerLine; //next line
    }
}

Changing from assignment operator "=" to memcpy for copying bytes resulting in segmentation fault

1 Answers1