Glibc change exposing bugs

I'm not disagreeing with the gist of what you mentioned, but this does point out that software is very complex in that the exact semantics of every interacting component has compounding effects on the overall result. This is why there is a tendency to code to achieve a pass on tests rather than by strict well defined "interface specs". However, having access to source code and working openly means problems can be identified quicker and shared quicker with other projects. If source code was not available (eg, if the library change had happened on a proprietary platform), finding the mistake would be more difficult and costly and there would be more bugs that would only come out under odd scenarios because thorough testing is impossible and certainly more difficult than analyzing even lots of source code.

An ideal wish: I want to see code assumptions be documented better on source code (assert calls and/or prose), even though we do have access to version control, peer review, and sometimes lots of testing with decent feedback. HTML can be generated from a heavily documented code to exclude all the little comments except when you want to see them (eg, before a final release or for inspection/audits). Trying to keep the source clean makes it easier to end up with problems over time. We would benefit from extensive standards for documentation, and those that like simple tools (like a simple text editor) can run simple filters on the project from the make file so that you can have all those notes not pollute a working copy.

[An extensive test suite designed to catch these problems is similar in effect but will leave holes whereas descriptive text can offer an important layer of defense.]

[Many times when you are (I am) learning a new code base, you have to make these notes anyway. Why not just formalize the effort and keep it together with the other code? We should even be able to have tests run from this documentation. Tagging precise spots can be done using any "unique" delimiter and can take an sgml approach. Then git and other tools can identify "conflicts" not just from 3-way merges with overlaps but from simple edits which overlap with a comment's scope.. triggering a requirement to update the comments whose scope were touched.]

[Another approach would be to try and keep something like a git branch of such comments/tests in sync and require that it be run before accepting commits on the main clean branch.]

[It might just be too much effort to do this in terms of bang for buck. Comments can very easily grow stale, though, that is why I suggested automatic conflict identification efforts within the workflow.]

In a communal view of software production and use it seems a bit unthoughtful to push this through and let (less technical) users suffer. It makes the software and the makers look bad. It makes it worse if that is shrugged off in a disdainful manner.

And it has been doing it for nearly a decade. And of course many other free memory debugging facilities like Duma (improved version of Electric Fence), mpatrol etc. produce these warnings too. As I would assume proprietary ones (on other platforms) to do also...

One could also define _FORTIFY_SOURCE to turn memcpy() etc into checking, slower versions. For more info, see:
* http://wiki.debian.org/Hardening
* https://wiki.ubuntu.com/CompilerFlags

This was not an API change. The memcpy() API has always been that the regions cannot overlap, and has been so for decades. This was just a change in the implementation details.

;}

The restrictions on memcpy() are hardly unique; *most* APIs do not tolerate overlapping memory regions. The memmove() routine is an exception. If you want a nice "safe" way to copy some data between buffers which may or may not overlap, and don't care so much about performance, just use memmove() everywhere.

While forward compatibility is a good thing in general, it is unreasonable for API developers to feel bound to support obvious *misuse* of their APIs which directly contradicts explicit API documentation, which is exactly what is happening here. Given that any broken applications can be trivially patched with a simple LD_PRELOAD, I see no reason not to permit this change to the internal implementation of memcpy() in glibc.

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

What would be the point of slowing down memcpy for all CPUs (by adding an extra check for cached CPU type variable value)? As long as the change doesn't slow down things for other CPUs, and considerably speeds it up on some, it sounds fine...

I fail to see how my previous post, which you replied to, is in any way related to free software purist happy to break Flash.

Indeed I think you did not even read it.

So I'll make an executive summary (but with new elements, for those who follow): in http://www.coding-guidelines.com/cbook/cbook1_2.pdf ; read, starting at pdf page 183, 3.4 behavior, 3.4.1 implementation-defined behavior, 3.4.3 undefined behavior, and 3.4.4 unspecified behavior. You will then hopefully understand why it would indeed be *dangerous* for security (not even talking about performance) in the long term if a widely used implementation starts giving guarantees defining "undefined behaviors", or if the maintainers of such implementation start acting like there seems to be some guarantees. (Think about other compliant implementations.)

If you don't like implementation-defined, undefined and unspecified behaviors in programming languages, use Java. I'm indeed starting to wonder if Linus does not secretly dream about writing operating systems in Java -- look at: some of his responses during the NULL-page mapping debacle, GCC adding optimizations taking advantages of undefined behaviors on integers, and his position on this memcpy implementation.

> On the other side are arguments that users systems are exposed to corruptions due to changes in the behaviour of a library call made for a marginal optimisation of a utility function.

Users systems are exposed to corruptions because they wrote code having undefined behavior in the first place, and there should be neither surprise nor scandal when code containing faulty constructs having undefined behavior starts to behave in an undefined way, because that's precisely the definition of what "undefined behavior" means.

Undefined behavior could has well change observable behavior depending on your power supply, the phase of the moon, and the fact Linus has been personally annoyed by a random bug (the last cause being the most probable in those examples, which is a little weird from an economical perspective, but oh well). Blame glibc maintainers all that you want, but you'll soon have multiple targets when the next advance in GCC expose other bugs caused by other undefined behaviors.

> To put it in perspective: I do not want the software I rely on to have ONE randomly inserted bug activated for a 200% improvement of its overall performance.

Under which perspective bugs are not "randomly" inserted? (given a non malicious intent in the first place). Would you be OK with "ONE randomly inserted bug activated" because of a change for support of a new hardware, or a new feature. Do you realize that even a bug fix can activate other bugs? Do you realize that you can easily avoid all that kind of trouble by NOT upgrading your system library ever, if you really want to? Do you understand that optimizations made at system level follow a different economic than optimizations made at application level? Do you understand that compiler/library evolution have participated in the moore law, and that your computer would be maybe 4x slower or produce 4x more heat if we still were in the naive compiler era and if low level layers had not been updated to be efficient with modern processor architecture?

> The proposed benefit is 20% of 2%, or 0.4%. The possible cost is my bank account.

Given the nature of the memory corruption, very unlikely to have that kind of security impact (but not 100 % impossible).

What is really funny is that even without the incriminated patch, the memcpy was NOT a memmove (fortunately). This particular Flash call resulted in corrupted data when copying memory in reverse order because the pointers was in a specific order, and the area overlapping. Calling memcpy with the previous GLibc implementation, and probably 99% of implementations existing on earth, will still result in data corruption if memory area are overlapping in the other order.

So I suggest you run your whole system LD_PRELOAD'ing all processes with a library that calls memmove instead of memcpy, if you are worried too much about that.
I also suggest that you immediately start looking for other bugs more susceptible to have big security impacts than this class, and that you also workaround them in weird way instead of fixing them correctly in the first place.
Maybe it would indeed be easier that you take a really old distribution, with a compiler that does very few optimization, and a very simple libc, and stick to it forever. (Well, you'll still have to do the memcpy/memmove replacement trick, but you'll have very few optimizations, so I guess that will make you happy.)

And oh, I forget to tell you: randomly defining "undefined behaviors" without auditing every components involved in both the system and its construction can sometime expose bugs with an high security impact. See the NULL page mapping debacle.

> I hope that the packagers of the distributions do the sensible thing.
> That is: pull that change out and shoot it.

Yeah, they all are reading LWN comments, waiting for your enlightenments.

If this approach is reasonable.. then just need to link it into memcpy as outlined above to have a trace of last few hundred errors on the system in shared memory waiting to dumped...

Thoughts?

$ dd of=/tmp/data if=/dev/zero count=16

$ g++ test.cpp

$ ./a.out /tmp/data
ret=0x80489de, dest=0x976d008, src=0x8048b4d, len=4, pid=205b

$ cat test.cpp
#include <sys/types.h>
#include <unistd.h>
#include <sys/mman.h>
#include <syscall.h>
#include <stdio.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <errno.h>
#include <sys/mman.h>
#include <syscall.h>
#include <errno.h>
#include <string.h>

#define SHARED_MEM_SIZE 8192

struct data // trace data for a memcpy() error
{
pid_t pid ;
const void *ret ;
const void *dest ;
const void *src ;
size_t len ;
} ;

struct log
{
long int index ; // sequence or index of last entry used
struct data entry[ SHARED_MEM_SIZE/sizeof(struct data) - 1 ] ; // vector of logging instances
} ;
struct log *pLog ;
#define N_ENTRIES (sizeof(pLog->entry)/sizeof(pLog->entry[0]))

void
displayLog(int i)
{
int j = i % N_ENTRIES ; // restrict index.

fprintf(stderr, "ret=%p, dest=%p, src=%p, len=%ld, pid=%lx\n",
pLog->entry[j].ret,
pLog->entry[j].dest,
pLog->entry[j].src,
(long)pLog->entry[j].len,
(unsigned long)pLog->entry[j].pid
) ;

}

void
capture(const void *dest, const void *src, size_t n)
{
int newLoc, oldLoc ;
do {
oldLoc = pLog->index ;
newLoc = pLog->index + 1 ;
} while ( __sync_bool_compare_and_swap( pLog->index, newLoc, oldLoc ) ) ;

int j = newLoc % N_ENTRIES ; // restrict index.
pLog->entry[j].ret = __builtin_return_address(0) ; //__builtin_extract_return_address(ra) ;
pLog->entry[j].dest = dest ;
pLog->entry[j].src = src ;
pLog->entry[j].len = n ;
pLog->entry[j].pid = getpid() ;
}

void *
setup(char *av)
{
void * p = NULL ;
int fd ;

fd = open(av, O_RDWR, 0x777) ; // open the file.
if (fd < 0)
{
fprintf(stderr, "Failed to open file /%s/ errno=%d\n", av, errno) ;
return 0 ;
}
p = mmap(0, SHARED_MEM_SIZE, PROT_WRITE|PROT_READ, MAP_SHARED, fd, 0) ;
if (p == 0)
{
fprintf(stderr, "Failed to mmap file /%s/ errno=%d\n", av, errno) ;
close(fd) ;
return 0 ;
}
// have mapping in p of SHARED_MEM_SIZE bytes
fprintf(stderr, "mapped %s to %p on fd %d\n", av, p, fd) ;

return p ;
}

int main(int ac, char **av)
{
int fd ;

if (ac >1)
{
pLog = (struct log*)setup(av[1]) ;
}
else
fprintf(stderr, "map <file>\n") ;

if (pLog)
{
capture(strdup("pete"), "joe", 4 ) ;
displayLog(pLog->index) ;
}

return 0 ;
}

http://valgrind.org/docs/manual/mc-manual.html#mc-manual....

Not to deprive you of the experience of doing it yourself, which can be instructive. However, you should need to reinvent the wheel if all you want is the use of the tool. At the very least, you can see how valgrind does it. As for automatically invoking it, well that's an exercise for the reader. :)