Valgrind Warning: Should I Take It Seriously

Question

Background: I have a small routine that mimics fgets(character, 2, fp) except it takes a character from a string instead of a stream. newBuff is dynamically allocated string passed as a parameter and character is declared as char character[2].

Routine:

character[0] = newBuff[0];

character[1] = '\0';

strcpy(newBuff, newBuff+1);

The strcpy replicates the loss of information as each character is read from it.

Problem: Valgrind does warns me about this activity, "Source and destination overlap in strcpy(0x419b818, 0x419b819)".

Should I worry about this warning?

Answer 1

Probably the standard does not specify what happens when these buffers overlap. So yes, valgrind is right to complain about this.

In practical terms you will most likely find that your strcpy copies in order from left-to-right (eg. while (*dst++ = *src++);) and that it's not an issue. But it it still incorrect and may have issues when running with other C libraries.

One standards-correct way to write this would be:

memmove(newBuff, newBuff+1, strlen(newBuff));

Because memmove is defined to handle overlap. (Although here you would end up traversing the string twice, once to check the length and once to copy. I also took a shortcut, since strlen(newBuff) should equal strlen(newBuff+1)+1, which is what I originally wrote.)

Answer 2

Yes, and you should also worry that your function has pathologically bad performance (O(n^2) for a task that should be O(n)). Moving the entire contents of the string back by a character every time you read a character is a huge waste of time. Instead you should just keep a pointer to the current position and increment that pointer.

Situations where you find yourself needing memmove or the equivalent (copying between buffers that overlap) almost always indicate a design flaw. Often it's not just a flaw in the implementation but in the interface.

Answer 3

Yes -- the behavior of strcpy is only defined if the source and dest don't overlap. You might consider a combination of strlen and memmove instead.

Answer 4

Yes, you should worry. The C standard states that the behavior of strcpy is undefined when the source and destination objects overlap. Undefined behavior means it may work sometimes, or it may fail, or it may appear to succeed but manifest failure elsewhere in the program.

Answer 5

The behavior of strcpy() is officially undefined if source and destination overlap.

From the manpage for memcpy comes a suggestion:

The memcpy() function copies n bytes from memory area s2 to memory area s1. If s1 and s2 overlap, behavior is undefined. Applications in which s1 and s2 might overlap should use memmove(3) instead.