Problems with implementing memmove in c

Question

I am trying to implement the memmove() function for a certain quiz, so I dynamically allocated a block of memory with the ptr buffer as temp holder in the swap operation and the swap is done in one loop. When I submit this code it gives me one wrong answer out of 3 test cases. When I separate the swap in two for loops, all cases succeed. So what is the difference ?

• swap in the same loop (One Case Wrong):

  uint8_t *my_memmove(uint8_t *src, uint8_t *dst, size_t length) {
      uint8_t *buffer;
      buffer = (uint8_t*)reserve_words(length);
      for (uint8_t i = 0; i < length; i++) {
          *(buffer+i) = *(src+i);
          *(dst+i) = *(buffer+i);   
      }
      return dst;
      free_words((uint32_t*)buffer);
  }
  

• swap in two loops (All Cases Succeed):

  uint8_t *my_memmove(uint8_t *src, uint8_t *dst, size_t length) {
      uint8_t *buffer;
      buffer = (uint8_t*)reserve_words(length);
      for (uint8_t i = 0; i < length; i++) {
          *(buffer+i) = *(src+i);
      }
  
      for (uint8_t i = 0; i < length; i++) {
          *(dst+i) = *(buffer+i);   
      }
  
      return dst;
      free_words((uint32_t*)buffer);
  }
  

• reserve_words function used (user_defined) :

  int32_t *reserve_words(size_t length) {
      int32_t *ptr;
      ptr = (int32_t *)malloc(length * sizeof(int32_t));
      return ptr;
  }
  

Answer 1

The problem here is that memmove is guaranteed to work with overlapping memory blocks. If you want to see a case where your first version would fail, try this:

char str[20]="Hello, World!";
my_memmove(&str[0], &str[1], 13);
puts(str);

The output is:

HHHHHHHHHHHHHH

It's easy to realize why. Every iteration you will write to the byte you're going to read from next iteration.

So your first version of my_memmove should really be called my_memcpy instead, because memcpy is not guaranteed to work with overlapping memory areas, which memmove is.

Answer 2

• The first version copies from the source to the destination as it copies to the temporary array: if the source and destination arrays overlap and src < dst you will overwrite parts of the source array before copying them. This is exactly where the difficulty lies when implementing memmove()
• The second version allocated a temporary array, copies from the source to it, then copies from the temporary array to destination: no problem, except you free the temporary array after the return statement, so there is a memory leak, and you are probably not expected to allocate memory for this assignment.
• There is a bug in all copying loops: i should be defined as a size_t, not a uint8_t. As coded, the function will fail for blocks longer than 255 bytes.
• there is no need to allocate length words to save length bytes.

The trick for memmove() is to test whether dst points inside the source array and copy from the end of the source array to the beginning if such is the case. There is no fully portable way to do this, but for most current systems with a flat address space, the test if (dst > src && dst < src + length) works fine.

Here is a simplistic implementation (note that your prototype has the src and dst pointers transposed from the standard memmove() function):

uint8_t *my_memmove(const uint8_t *src, uint8_t *dst, size_t length) {
    if (dst > src && dst < src + length) {
        for (size_t i = length; i-- > 0;)
            dst[i] = src[i];
    } else {
        for (size_t i = 0; i < length; i++)
            dst[i] = src[i];
    }
    return dst;
}

Answer 3

1. Memory leak

    return dst;
    free_words((uint32_t*)buffer);

You will never call free_words as it will return earlier.

2. When you copy in one loop and memory locations overlap your algorithm will overwrite the data and the function will not work.

3. This buffer completelly useless and it is an example of bad programming

4. function should get and return void * pointers.

5. You do not have to cast the pointers if one side of the assignment is (void *)

void *mymemmove(void *dest, void *src, size_t size)
{
    void *saveddest = dest;
    unsigned char *ucdest = dest, *ucsrc = src;
    if(dest && src && size)
    {
        if(dest < src)
        {
            while(size--)
            {
                *ucdest++ = *ucsrc++;
            }
        }
        else if(src < dest) //this if to avoid copy if dest == src (not needed)
        {
            ucdest += size - 1; ucsrc += size - 1;
            while(size--)
            {
                *ucdest-- = *ucsrc--;
            }
        }
    }
    return saveddest;
}