3

It's a home work. I want to implement memcpy(). I was told memory area must not overlap. Actually I don't understand what that means, because this code works fine, but there is a possibility of memory overlap. How to prevent it?

void *mem_copy(void *dest, const void *src, unsigned int n) {
    assert((src != NULL) && (n > 0));  
    int i = 0;
    char *newsrc = (char*)src;
    char *newdest = (char*)dest;
    while (i < n) {
        newdest[i] = newsrc[i];
        i++;
    }
    newdest[i]='\0';
    return newdest;
}
chqrlie
  • 131,814
  • 10
  • 121
  • 189
surjit
  • 338
  • 4
  • 15
  • 2
    Delete `newdest[i]='\0';` – BLUEPIXY Jun 03 '17 at 20:49
  • 2
    'No overlap' means that no address in the range `[dest..dest+n-1]` is the same as any address in the range `[src..src+n-1]`. Determining that portably is actually rather hard — formally, you can only compare addresses in a single array, but usually when the address ranges do not overlap, the areas are *not* part of the same array. – Jonathan Leffler Jun 03 '17 at 20:50
  • You can prevent the overlapping by checking via pointer arithmetics – Sebastian Walla Jun 03 '17 at 20:50
  • if I remove `newdest[i]='\0'`, how to know the string is ended, it it's string to be copied? – surjit Jun 03 '17 at 20:51
  • 2
    `assert(n > 0)` is wrong. Copying 0 bytes is allowed. – melpomene Jun 03 '17 at 20:51
  • 2
    `memcpy` is not a string function. It doesn't care about `'\0'`. – melpomene Jun 03 '17 at 20:52
  • "Actually I don't understand what doest that mean," Look at it on paper, writing 2 lines, 1 2 3 4 5 and just below it 3 4 5 6 7 . Start copying -- copy 1 over the 3, then 2 over the 4, then 3 -- NO, 3 is now 1! 4 is now 2! Do you see why overlapping made things go horribly wrong? – Dave S Jun 03 '17 at 20:53
  • Note that you can get overlapping memory using: `char data[] = "ABCDEFGHIJKLMOPQRSTUVCWXYZ"; mem_copy(data+2, data+6, 6); mem_copy(data + 12, data + 4, 10);`. – Jonathan Leffler Jun 03 '17 at 20:56
  • Surjit, I appreciate you're a beginner. Take a copy of @JonathanLeffler 's comment and store it away; it will make sense to you once you've read your first comprehensive C book. – Bathsheba Jun 03 '17 at 20:56
  • I've taken two different variables for `src` and `dest`. so I guess memory should not overlap – surjit Jun 03 '17 at 20:57
  • No, they might. E.g. dest might start 3 bytes before src but src have 4 or more bytes to copy. – Bathsheba Jun 03 '17 at 20:58
  • so how do I prevent it – surjit Jun 03 '17 at 20:59
  • Do what dasblinkenlight suggests, but it is tricky to do portably. – Bathsheba Jun 03 '17 at 21:01
  • yeah.. I'm not getting it – surjit Jun 03 '17 at 21:04
  • @surjit You're not supposed to prevent it. It's the caller's job to make sure the regions to be copied don't overlap. – melpomene Jun 03 '17 at 21:06
  • No, I was told to implement the above function such that memory area should not overlap while copying. – surjit Jun 03 '17 at 21:10
  • You could copy to a temporary block of memory first, then copy from there to the destination. – Barmar Jun 03 '17 at 21:25
  • for that I'll have to use two loops, it won't be efficient – surjit Jun 03 '17 at 21:27
  • Possible duplicate of [Copying bytes without memcpy](https://stackoverflow.com/questions/42823726/copying-bytes-without-memcpy) – S.S. Anne Sep 28 '19 at 17:53

3 Answers3

3

When source and destination memory blocks overlap, and if your loop copies one element after the other starting from index 0, it works for dest < source, but not for dest > source (because you overwrite elements before having copied them) and vice versa.

Your code starts copying from index 0, so you can simply test which situations work and which not. See the following test code; It shows how moving a test string forward fails, whereas moving the string backwards works fine. Further, it shows how moving the test string forward works fine when copying from backward:

#include <stdio.h>
#include <string.h>

void *mem_copy(void *dest, const void *src, size_t n) {
    size_t i = 0;
    char* newsrc = (char*)src;
    char* newdest = (char*)dest;
    while(i < n) {
        newdest[i] = newsrc[i];
        i++;
    }
    return newdest;
}

void *mem_copy_from_backward(void *dest, const void *src, size_t n) {
    size_t i;
    char* newsrc = (char*)src;
    char* newdest = (char*)dest;
    for (i = n; i-- > 0;) {
        newdest[i] = newsrc[i];
    }
    return newdest;
}

int main() {

    const char* testcontent = "Hello world!";
    char teststr[100] = "";

    printf("move teststring two places forward:\n");
    strcpy(teststr, testcontent);
    size_t length = strlen(teststr);
    printf("teststr before mem_copy: %s\n", teststr);
    mem_copy(teststr+2, teststr, length+1);
    printf("teststr after mem_copy: %s\n", teststr);

    printf("\nmove teststring two places backward:\n");
    strcpy(teststr, testcontent);
    length = strlen(teststr);
    printf("teststr before mem_copy: %s\n", teststr);
    mem_copy(teststr, teststr+2, length+1);
    printf("teststr after mem_copy: %s\n", teststr);

    printf("move teststring two places forward using copy_from_backward:\n");
    strcpy(teststr, testcontent);
    length = strlen(teststr);
    printf("teststr before mem_copy: %s\n", teststr);
    mem_copy_from_backward(teststr+2, teststr, length+1);
    printf("teststr after mem_copy: %s\n", teststr);
}

Output:

move teststring two places forward:
teststr before mem_copy: Hello world!
teststr after mem_copy: HeHeHeHeHeHeHeH

move teststring two places backward:
teststr before mem_copy: Hello world!
teststr after mem_copy: llo world!

move teststring two places forward using copy_from_backward:
teststr before mem_copy: Hello world!
teststr after mem_copy: HeHello world!

So one could write one function, which decides whether to start copying from index 0 or from index n depending on whether the caller wants to copy forward or backward. The tricky thing is to find out whether the caller will copy forward or backward, since a pointer arithmetic on src and dest like if (src < dest) copy_from_backward(...) is actually not permitted in every case (cf. the standard, e.g. this draft):

6.5.9 Equality operators

When two pointers are compared, the result depends on the relative locations in the address space of the objects pointed to. If two pointers to object or incomplete types both point to the same object, or both point one past the last element of the same array object, they compare equal. If the objects pointed to are members of the same aggregate object, pointers to structure members declared later compare greater than pointers to members declared earlier in the structure, and pointers to array elements with larger subscript values compare greater than pointers to elements of the same array with lower subscript values. All pointers to members of the same union object compare equal. If the expression P points to an element of an array object and the expression Q points to the last element of the same array object, the pointer expression Q+1 compares greater than P. In all other cases, the behavior is undefined.

Though I've never been in a situation where src < dest did not give me the desired results, comparing two pointers this way is actually undefined behaviour if they do not belong to the same array.

Hence, if you ask "how to prevent it?", I think that the only correct answer must be: "It's subject to the caller, because function mem_copy cannot decide whether it may compare src and dest correctly."

Stephan Lechner
  • 34,891
  • 4
  • 35
  • 58
  • am getting this error `Conditional jump or move depends on uninitialised value(s)` when I used `--track-origins=yes` flag I got `Uninitialised value was created by a stack allocation` – surjit Jun 03 '17 at 22:29
  • 2
    @surjit: That's correct: after the first `mem_copy(teststr+2, teststr, length+1);`, the null terminator set by `strcpy()` in `teststr` at offset `length` gets overwritten and the array is no longer null terminated before its uninitialized portion. Fix this by initializing the array: `char teststr[100] = "";` – chqrlie Jun 03 '17 at 22:36
2

Actually I don't understand what doest that mean [for memory to overlap]

Consider this example:

char data[100];
memcpy(&data[5], &data[0], 95);

From the program's point of view, the range of addresses from src to src+n must not overlap the range from dest to dest+n.

if there is possibility of memory overlap, how to prevent it?

You can make your algorithm work with or without an overlap by deciding to copy overlapping regions from the back if src has numerically lower address than dest.

Note: Since you are doing memcpy, not strcpy, forcing null termination with newdest[i]='\0' is incorrect, and needs to be removed.

Sergey Kalinichenko
  • 714,442
  • 84
  • 1,110
  • 1,523
  • I understood what is memory overlap, but I still don't understand how to prevent it – surjit Jun 03 '17 at 20:56
  • @surjit Take a look at [this Q&A](https://stackoverflow.com/q/13339582/335858) which is discussing `memmove`, a version of `memcpy` that allows regions to overlap. – Sergey Kalinichenko Jun 03 '17 at 21:02
  • how to copy backwards? – surjit Jun 03 '17 at 21:11
  • 1
    @surjit Start at `src+n-1`, end at `src`, inclusive, and do `--` instead of `++`. The Q&A that I linked in the comment uses this approach by setting `operation`, `current`, and `end` before entering the loop. – Sergey Kalinichenko Jun 03 '17 at 21:14
  • I used this `for(; (n-1) != (-1); n--) newdest[n-1] = newsrc[n-1]` It's not working – surjit Jun 03 '17 at 21:50
  • @surjit That's because `n` is `unsigned`, you shouldn't compare it to `-1`. You can use `do {n--; newdest[n] = newsrc[n];} while (n);`, or you can use pointer arithmetics instead. – Sergey Kalinichenko Jun 03 '17 at 21:54
  • that's not working either.. my input was `coming` `n = 2`. I got output as `co���` – surjit Jun 03 '17 at 21:59
  • @surjit It is working, you've instructed the program to copy two characters, and that's precisely what it did. You are copying strings, though, so you need to make sure they are null-terminated for printing. This shouldn't be done in `memcpy`, but in the code that calls it. Alternatively, fill the buffer with zeros before copying into it, either by `memset` or by allocating it with `calloc`. – Sergey Kalinichenko Jun 03 '17 at 22:01
  • I am getting this error `Conditional jump or move depends on uninitialised value(s)` when I used `--track-origins=yes` flag I got `Uninitialised value was created by a stack allocation` – surjit Jun 03 '17 at 22:25
  • 2
    @surjit: you get this warning because `data[]` is uninitialized, therefore `memcpy()` reads funinitialized bytes and some code elsewhere in your program likely depends on these byte values... – chqrlie Jun 03 '17 at 22:40
1

There are some issues in your re-implementation of memcpy():

  • The size argument n should have type size_t. The index variable i should have the same type as the size argument.

  • It is OK to pass a count of 0. Indeed your code would behave correctly in this case, remove the test from the assert().

  • Avoid casting away the const qualifier unless absolutely necessary.

  • Do not tack a '\0' at the end of the destination, it is incorrect and will cause buffer overruns.

Here is a corrected version:

void *mem_copy(void *dest, const void *src, size_t n) {
    assert(n == 0 || (src != NULL && dest != NULL));  
    size_t i = 0;
    const char *newsrc = (const char *)src;
    char *newdest = (char *)dest;
    while (i < n) {
        newdest[i] = newsrc[i];
        i++;
    }
    return dest;
}

Regarding the potential overlap between the source area and the destination area, your code's behavior will be surprising if the destination pointer is greater than the source, but within the source area:

char buffer[10] = "12345";
printf("before: %s\n", buffer);
mem_copy(buffer + 1, buffer, 5);
printf("after: %s\n", buffer);

Will output:

before: 12345
after: 111111

There is no completely portable way to test for such overlap, but it is quite easy on non exotic architectures at some small cost in execution time and code size. The semantics of memcpy() is that no such test is assumed to be performed by the library, so the programmer should only call this function if there is no possibility for source and destination areas to overlap. When in doubt, use memmove() that handles overlapping areas correctly.

If you wish to add an assert for this, here is a mostly portable one:

assert(n == 0 || newdest + n <= newsrc || newdest >= newsrc + n);

Here is a simple rewrite of memmove(), albeit not completely portable:

void *mem_move(void *dest, const void *src, size_t n) {
    assert(n == 0 || (src != NULL && dest != NULL));  
    const char *newsrc = (const char *)src;
    char *newdest = (char *)dest;
    if (newdest <= newsrc || newdest >= newsrc + n) {
        /* Copying forward */
        for (size_t i = 0; i < n; i++) {
            newdest[i] = newsrc[i];
        }
    } else {
        /* Copying backwards */
        for (size_t i = n; i-- > 0;) {
            newdest[i] = newsrc[i];
        }
    }
    return dest;
}
chqrlie
  • 131,814
  • 10
  • 121
  • 189