strcpy() revisited

[syzygy]

Some of these arguments are somewhat circular. For example, we were previously discussing a compiler change to detect and report overflow. ONLY if a compile-time switch were used. So why does a user have to turn it on there, but not for the overlap test? You can't have it both ways. That one won't be used (compiler switch) so they make it default, while the other defaults on (overlap or really PARTIAL overlap detection).

Doesn't that again seem both inconsistent AND strange???

It would certainly be inconsistent if this was done by one and the same entity. But -fsanitize=undefined was implemented by clang and soon by gcc, and the Apple thing is solely due to Apple, as you already said.

I don't think there is any disagreement here that what Apple did is at least somewhat dubious. Opinions do differ on whether it is still acceptable (given the fact that anything less would not force people to fix their code) or completely unacceptable.

It would be interesting to know Apple's official position on why they did this and why they think it is acceptable.

[wgarvin]

Okay, so a few weeks ago a completely legal change made by Apple to their strcpy implementation, caused bob's program (which had a bug in it) to not work correctly on his Mac. It didn't just silently misbehave though; it aborted before any harm was done. And bob has been complaining about it ever since.

Let me just point out the considerable irony of this, because once I stopped and thought it through, it was rather funny to me:

Apple helped him find a subtle bug in Crafty, in fact it was likely THE oldest bug still remaining in Crafty, since by his own admission this code is extremely old and has never been changed. And bob is unhappy about it. He thinks they should have done something else. ANYTHING else. He thinks it was literally the meanest, nastiest thing Apple could have done to his poor little program.

He has run this code on many other platforms over the years, and not one of those C implementations helped him find his bug. All of them silently did something--bob asserts they were doing "the right thing", and many of them probably WERE doing the thing bob thought they ought to do, but since the standard doesn't say they have to do that, there's no way we can be completely sure.

In fact there's probably Crafty binaries out there somewhere, compiled for some platform where overlapping strcpy can fail in a silent and subtle way, on which ReadPGN() will mangle that input and/or not do exactly what bob expected. The poor user stuck with THAT version of Crafty, might never have tried that feature on it; but if he did, he must surely have been disappointed! Too bad he couldn't be bothered to file a bug report about it, after all he's just a user who wants programs to "work", and while he might be willing to help diagnose and fix broken programs, also he might not and that's totally fair.

So to recap: Apple helped bob find an extremely old and subtle bug, which none of his other target systems lifted a finger to help him find, but bob feels little gratitude for this. In fact, one of his many complaints seems to be that Apple didn't help him enough, since the standard library function just printed 'Abort' and didn't write something more informative to stderr, or something, and bob didn't know or care what the right system log to look in was, and for at least for the first few days, it didn't occur to him to fire up the program under a debugger and see why it was aborting. He was (and seemingly still is) indignant about his time being was "wasted" diagnosing the program, because Apple didn't help him enough. But it also would have been just fine with him if they had not helped him at all and just let his program possibly trash memory or spawn nasal demons or whatever. Then he would have been blissfully unaware of the bug, and his users would (hopefully) also have been blissfully unaware of the bug, and as we all know, ignorance is bliss!

(He has also argued repeatedly that the bug shouldn't really be a bug, that the compiler or library should just do what he thought they would do--DWIM!!--and that there would never be any performance cost to this--not true of course).

I mean, I've certainly had my share of bugs in my code before, but if my code has a bug in it I recognize that its my fault and I am grateful when other parties help me find those bugs, even if it means my code was crashing until I get the fix figured out and deployed. As the programmer who wrote that code, I'm the one responsible for making sure it is a correct, working program. Even if I wanted to fob that responsibility off on the library vendor or whoever else, no other party is capable of making my program do what I want. Its correctness is something only I can achieve, and to do that I have to follow the rules of the language, and this is not some difficult, elusive thing--its what programming is all about.

[syzygy]

Seems perfectly reasonable to me. Somewhere. Some alternate universe. Where insanity reigns supreme... ... when they COULD have fixed both with a simple call to memmove().

Yes, let's cover up bugs instead of expose them.... that is insanity.

So, to recap the position you must believe, "In order to compile more efficiently and be able to use tricky optimizations, it is perfectly OK to slow down strcpy() by a factor of 2."??? I wonder if those "wonderful optimizations" will offset that factor of 2? That is, I wonder if this is one of those "useful optimizations" that slows the code down rather than speeding it up?

No he said it is insane to cover up bugs instead of exposing them.

There are two things here:
1) Apple pushing people to fix their bugs;
2) the good sides of strcpy() UB.

Ad 1)
The "ethics" of what Apple did may be debatable, but what they did does work. The extra check certainly does not slow down strcpy() by a factor of 2, that's just nonsense.

Ad 2)
Leaving strcpy() behaviour for overlapping regions undefined indeed leaves room for "wonderful optimisations". This is independent of 1). Just look at gcc and clang on Linux.

I suppose that is OK. But I can't even disable it to gain the speed back. I guess we call that a "de-optimizer"???

You were going to install Linux.

[syzygy]

Those existing programs are not affected, as the change is introduced during translation time, not in the link-time instance of strpcy(). One already must have rebuilt the project for the checks to enter, and then XCode would show the stack trace and the offending line right in your face. There is a reason there is no outcry about it other than here.

Are you talking about the same strcpy() Abort? That's not done by the compiler, it is done inside the lib. You can find Apple developers commenting on this and they even posted the lib source code showing the overlap check. This fails for gcc or clang on Mavericks, not on mountain lion, not on other platforms using the same version of gcc.

The magic is in the header file on my system: <string.h> under certain conditions includes <secure/_string.h>, which then aliases strcpy to __builtin_strcpy_chk. Whenever I link to the lib-supplied instance of strcpy (for example by using -U_FORTIFY_SOURCE, or running an old compile of crafty should also work), then it doesn't perform the check.

This should not be new information, and for me the point where I start to repeat myself means that I will prune my side of the discussion tree.

Ok, so this check is only included when you compile with -D_FORTIFY_SOURCE, which on Apple for whatever reason happens to be the default? Then we only have to tell Bob for the fifth time to add -U_FORTIFY_SOURCE and maybe he'll pick it up.

The whole purpose of -D_FORTIFY_SOURCE is to catch this sort of things. That it didn't do this before for strcpy() is what one could call a bug.

Btw, I am starting to be more interested in -D_FORTIFY_SOURCE. I still wouldn't want it for "production code", but it certainly makes sense to do long tests with this option enabled.

[Rein Halbersma]

Btw, I am starting to be more interested in -D_FORTIFY_SOURCE. I still wouldn't want it for "production code", but it certainly makes sense to do long tests with this option enabled.

Knowledge on compiler warnings (as well as optimizations) will become more and more important as time goes on, since these incredibly powerful optimizations will hit any non-conforming program quite hard.

I have been compiling routinely with -Wall -Wextra -pedantic -Werror and on both gcc / clang that is already quite stringent, but I have been warning free for about a year (this is on 15K lines and a few hundred commits). Today I tested clang with -Weverything on my own codebase. I found 5 bugs and 1 efficiency pessimization in 30 minutes, with 2 known bugs in Boost code and 4 false positives that I could verify were not actually happening in my code.

Then I tried clang -fsanitize=undefined, and found nothing (phew). All the other -fsanitize options also came up clean, apart from =address that interfered with the Boost.Test library that imposes a conflicting memory instrumentation.

[bob]

It takes one more add and one more compare. How hard is that? I think it is doing something wrong by detecting the overlap in the first place.

You do realize that to implement the solution you've been advocating, covering up the bug by invoking memmove(), that it is necessary to actually detect the overlap?

You do realize that I said EXACTLY THAT? Since they chose to detect the overlap, why just abort rather than actually fix it? Simple enough now? I have only written that a dozen times. Seems silly to waste the CPU cycles to catch it, when it is not done in most programs, and then get NOTHING from those wasted cycles... If you'd join a thread by reading from the top, this wasted post would not be necessary.

[bob]

Seems perfectly reasonable to me. Somewhere. Some alternate universe. Where insanity reigns supreme... ... when they COULD have fixed both with a simple call to memmove().

Yes, let's cover up bugs instead of expose them.... that is insanity.

So, to recap the position you must believe, "In order to compile more efficiently and be able to use tricky optimizations, it is perfectly OK to slow down strcpy() by a factor of 2."??? I wonder if those "wonderful optimizations" will offset that factor of 2? That is, I wonder if this is one of those "useful optimizations" that slows the code down rather than speeding it up?

No he said it is insane to cover up bugs instead of exposing them.

There are two things here:
1) Apple pushing people to fix their bugs;
2) the good sides of strcpy() UB.

Ad 1)
The "ethics" of what Apple did may be debatable, but what they did does work. The extra check certainly does not slow down strcpy() by a factor of 2, that's just nonsense.

What does it cost to walk down the string once? Twice? They have to walk it once to get the length, then again to copy. Close enough to 2x to use it as an approximation of the cost.

And what they did does not exactly "work". They only catch overlap in one direction, and NOT the worst one at that...

Ad 2)
Leaving strcpy() behaviour for overlapping regions undefined indeed leaves room for "wonderful optimisations". This is independent of 1). Just look at gcc and clang on Linux.

I suppose that is OK. But I can't even disable it to gain the speed back. I guess we call that a "de-optimizer"???

You were going to install Linux.

Once I finish grading, yes. Does that help anyone but me, however?

[wgarvin]

It takes one more add and one more compare. How hard is that? I think it is doing something wrong by detecting the overlap in the first place.

You do realize that to implement the solution you've been advocating, covering up the bug by invoking memmove(), that it is necessary to actually detect the overlap?

You do realize that I said EXACTLY THAT? Since they chose to detect the overlap, why just abort rather than actually fix it? Simple enough now? I have only written that a dozen times. Seems silly to waste the CPU cycles to catch it, when it is not done in most programs, and then get NOTHING from those wasted cycles... If you'd join a thread by reading from the top, this wasted post would not be necessary.

In fairness, reading this thread from the top would take hours.

We didn't get NOTHING from the wasted cycles. The bug in your program got found, and fixed. Many other bugs got found, and some of them will get fixed. Lots of users got annoyed, and maybe they will get their software from more reliable sources in the future (though I doubt it).

Anyway, don't you see the inconsistency between your two positions? On the one hand you rail against them wasting 1-2 cycles per strcpy for a comparison and highly-predictable branch (i.e. almost always there is no overlap). On the other hand, you seem to think C compilers should assign semantics to your source code, which would have a much higher performance cost than just 1-2 cycles here and there. C and C++ have both always chosen performance over safety.

[bob]

Those existing programs are not affected, as the change is introduced during translation time, not in the link-time instance of strpcy(). One already must have rebuilt the project for the checks to enter, and then XCode would show the stack trace and the offending line right in your face. There is a reason there is no outcry about it other than here.

Are you talking about the same strcpy() Abort? That's not done by the compiler, it is done inside the lib. You can find Apple developers commenting on this and they even posted the lib source code showing the overlap check. This fails for gcc or clang on Mavericks, not on mountain lion, not on other platforms using the same version of gcc.

The magic is in the header file on my system: <string.h> under certain conditions includes <secure/_string.h>, which then aliases strcpy to __builtin_strcpy_chk. Whenever I link to the lib-supplied instance of strcpy (for example by using -U_FORTIFY_SOURCE, or running an old compile of crafty should also work), then it doesn't perform the check.

This should not be new information, and for me the point where I start to repeat myself means that I will prune my side of the discussion tree.

Here's the top (just #includes) from /usr/include/string.h on my macbook mavericks box:

#include <_types.h>
#include <sys/cdefs.h>
#include <Availability.h>
#include <sys/_types/_size_t.h>
#include <sys/_types/_null.h>

All that is after that is prototypes.

as far as your "new information" goes, Apple developers posted the strcpy() source, which had the partial overlap detection code included. That matches what I seem to be seeing here...

[mvk]

What does it cost to walk down the string once? Twice? They have to walk it once to get the length, then again to copy. Close enough to 2x to use it as an approximation of the cost.

I have reason to believe they execute the check after the copy. See my screenshot of XCode. If that's confirmed, the cost is near zero (branch prediction etc).

And what they did does not exactly "work". They only catch overlap in one direction, and NOT the worst one at that...

So far that was only the case in your toy example. And that was explained: a gcc optimisation touched strcpy before apple got there. On non-toy examples, I get buffer errors in both directions.