7

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?

nh2
  • 45
  • 6
Chris Allen
  • 653
  • 4
  • 9
  • 17

6 Answers6

12

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

asveikau
  • 39,039
  • 2
  • 53
  • 68
  • 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. – R.. GitHub STOP HELPING ICE Jan 28 '11 at 01:03
  • @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. – asveikau Jan 28 '11 at 01:13
  • 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. – Daniel Chin Apr 12 '23 at 03:30
5

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.

R.. GitHub STOP HELPING ICE
  • 208,859
  • 35
  • 376
  • 711
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.

Jerry Coffin
  • 476,176
  • 80
  • 629
  • 1,111
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.

ephemient
  • 198,619
  • 38
  • 280
  • 391
3

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.

Matt K
  • 13,370
  • 2
  • 32
  • 51
2

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.

Community
  • 1
  • 1
nh2
  • 45
  • 6