Glibc change exposing bugs [LWN.net]

Glibc change exposing bugs

Posted Nov 10, 2010 20:51 UTC (Wed) by MisterIO (guest, #36192) [Link]

(!hand-written assembly) != (cpu=generic)

But anyway, my argument was not just !assembly, it was also not so much assembly. Look at the one proposed by Linus:
void *memcpy(void *dst, const void *src, size_t size)
{
void *orig = dst;
asm volatile("rep ; movsq"
:"=D" (dst), "=S" (src)
:"0" (dst), "1" (src), "c" (size >> 3)
:"memory");
asm volatile("rep ; movsb"
:"=D" (dst), "=S" (src)
:"0" (dst), "1" (src), "c" (size & 7)
:"memory");
return orig;
}

It may not be all that well tested, but it's simple enough to be comprehensible.

Glibc change exposing bugs

Posted Nov 10, 2010 21:15 UTC (Wed) by jwb (guest, #15467) [Link]

I'm not sure what you're driving at. The problem of the current article is not that memcpy is written in asm, it's that the memcpy runs backwards. This has changed the semantics of the function and is unrelated to how it is implemented.

Glibc change exposing bugs

Posted Nov 10, 2010 21:20 UTC (Wed) by JoeBuck (subscriber, #2330) [Link]

No, the semantics of the function are that the behavior is not defined if the source and destination strings overlap, as the relevant standards and the man page clearly state. That's why there's an alternative function named memmove. If you write C, call memcpy, and the arguments overlap, you've written a non-portable program.

Glibc change exposing bugs

Posted Nov 10, 2010 21:55 UTC (Wed) by nix (subscriber, #2304) [Link]

Nah, it would be non-portable if it were implementation-defined whether memcpy() worked for overlapping regions. Since it is undefined, what you have written when you use a memcpy() on overlapping regions is technically (in the most pedantic mode imaginable) not C at all.

(I know you know this, this is really for others reading)

Glibc change exposing bugs

Posted Nov 11, 2010 1:19 UTC (Thu) by tialaramex (subscriber, #21167) [Link]

I think it /is/ C, but it isn't clear what the C means. This code doesn't break any of the rules of the C language which would cause it not to parse. Otherwise we wouldn't have got into this mess because it wouldn't compile.

[ Sadly I don't trust the Adobe developers enough to imagine that a diagnostic from static analysis would have stopped them doing this. I think "Warning: abuse of memcpy()" would have scrolled by with hundreds of other warnings they ignore... ]

In the same way "Don't frabidulate the wugs, she's my uncle" is an English sentence, but it isn't clear what it means. You can parse it, and you can answer some questions about it, e.g. "Are you being asked to frabidulate the wugs?" but there are big unknowns.

Glibc change exposing bugs

Posted Nov 11, 2010 7:21 UTC (Thu) by nix (subscriber, #2304) [Link]

This code doesn't break any of the rules of the C language which would cause it not to parse.

It is true that the standard does not require a diagnostic in this case, and that providing a diagnostic in all cases at compile time is impossible, but that doesn't make it any less 'not C'. C is not just 'what the compiler happens to accept'.

(I completely agree that in this particular case a warning would likely have been useless unless accompanied by a brickbat.)

btw, HJ's trying to fix the underlying problem here.

Glibc change exposing bugs

Posted Nov 10, 2010 21:21 UTC (Wed) by MisterIO (guest, #36192) [Link]

I'm not sure what _you_ are driving at! Read again the first message of mine that you commented on and you should get the general tone of my comment.

Glibc change exposing bugs

Posted Nov 10, 2010 22:04 UTC (Wed) by joib (guest, #8541) [Link]

And, this has a couple of other nice advantages

1) Less I$ pollution. You won't see this in a memcpy() benchmark, but what about a more realistic workload?

2) Give some incentive to CPU makers to optimize the simple rep mov instead of requiring ever more fancy unrolled loops written in the latest instruction set extension. :)

Glibc change exposing bugs - a bug in proposed memcpy

Posted Nov 16, 2010 16:45 UTC (Tue) by promotion-account (guest, #70778) [Link]

Look at the one proposed by Linus:
void *memcpy(void *dst, const void *src, size_t size)
{
void *orig = dst;
asm volatile("rep ; movsq"
:"=D" (dst), "=S" (src)
:"0" (dst), "1" (src), "c" (size >> 3)
:"memory");
asm volatile("rep ; movsb"
:"=D" (dst), "=S" (src)
:"0" (dst), "1" (src), "c" (size & 7)
:"memory");
return orig;
}

For completeness, this should have an "rcx" clobber, or GCC may believe that this important register will not change after each assembly snippet. Such a bug may get triggered if GCC aggressively inlined the code, which occurs in a good number of cases given its optimizer competency.

--Darwish