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?

Kovar

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.)

Seibert

Even if the ordering is left-to-right, there could be issues due to unrolling/reordering and larger-than-byte units of copying. I would consider this usage of strcpy highly unsafe and likely to break even between different versions of the same library, or even different builds with different compilers. – Agonizing

@R.. I agree, especially with the point about larger-than-byte units. Just to be clear, I agree that the code is incorrect and should be changed. – Seibert

I think what you originally wrote, strlen(newBuff+1)+1, is better. It should be in your code, because it's readable. It should be on Stackoverflow, to avoid confusion. – Blackfish

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.

Agonizing

Answer 3

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.

Churchyard

Answer 4

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.

Phanerogam

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.

Gabrielagabriele

Regardless of how it's implemented, the behavior is undefined. This is stated explicitly in the standard. – Agonizing

strcpy gives undefined behavior if the buffers overlap. memcpy does the same, but is otherwise irrelevant. – Phanerogam

OK, my local man page for strcpy doesn't contain that warning, but the memcpy one does. – Gabrielagabriele

The only functions in the C standard that accept pointers to overlapping memory regions for reading and writing are memmove and wmemmove. For everything else (including snprintf!) the behavior is undefined. – Agonizing

Answer 6

The answer is yes: with certain compiler/library implementations, newest ones I guess, you'll end up with a bogus result. See How is strcpy implemented? for an example.

Shipowner