FORTIFY_SOURCE `is a libc feature that provides primitive support for detecting buffer overflows in various functions which operate on memory areas and strings. A limited set of buffer overflows can be detected with this feature, but it provides an additional level of validation for some functions which are the origin of buffer overflow flaws.

(remove the "potential" term: These are buffer overflows, there is not such thing as a false positive at least at level 1.)

Add some more reviewers: It is still WIP, and it needs tmore testing with newer GCC but there are many new files and it's easy for minor details to go unnoticed if I am the only reviewer.

I think the functionality is important as it found 3 bugs already, and the feature is widely available (glibc, Android's bionic, NetBSD, Apple MacOSX), so I am planning to bring it to the tree in small steps: new functionality in libc first, support for buildworld and option knobs afterwards.

Also note: while testing it has is convenient to build the functionality ON by default but I don't plan to commit this with such aggressiveness. NetBSD and others build world with this by default but that is a decision that corresponds elsewhere. Also pending is how ports may want to use it.

The diff is enormous and it pollutes a lot of unrelated places. E.g. the loader Makefiles changes or the forced undef fortify in libc/strings/stdio, as I understand, to avoid recursion.

It is huge. The undefs are necessary to build a fortified libc, otherwise there will be the above mentioned recursion problem.

Can the pollution be minimized somehow ? Can the patch be split into digestable pieces ?

Yes: we can create a new revision with only the changes in include/ and libc/secure/ (plus mtree), which is the core and is mostly new files.
The support for building fortified builds should be in an independent review and I had no plans to commit it in a first pass anyways.

For now it is/was convenient to have both together for testing purposes but as soon as we pass the build tests with modern gcc we can split them functionally.

Nothing was prevented, since this code is after the memccpy() call. Therefore, the error message should be adjusted, since it is valid to have an overlap between src..src+n-1 and dest..dest+n-1 if an occurrence of c prevents an actual overlap.

Additionally, casts to const char * should be added before adding len to avoid depending on a GCC extension, and casts to uintptr_t should be added before comparing to avoid undefined behaviour from comparing (</<=/>/>=) pointers from different arrays.

The diff is enormous and it pollutes a lot of unrelated places. E.g. the loader Makefiles changes or the forced undef fortify in libc/strings/stdio, as I understand, to avoid recursion.
Can the pollution be minimized somehow ? Can the patch be split into digestable pieces ?

Partly the boot loader pollution is like the pollution for SSP. I do wonder why FORTIFY conditionals were added to some places that currently do not have SSP conditionals.

I think compilation features that do not work in boot loader environments will continue to exist and grow, so a more general framework for turning them off may be interesting.

I have been rethinking this and documenting __builtin_object_size only makes sense if we document the difference between GCC and clang.
Those are important in {our||Android} implementation and I have only seen those documented herehttps://copperhead.co/2015/07/27/hardening-bionic

Of course this is to be done in the context of a wider fortify_source documentation.

The diff is enormous and it pollutes a lot of unrelated places. E.g. the loader Makefiles changes or the forced undef fortify in libc/strings/stdio, as I understand, to avoid recursion.
Can the pollution be minimized somehow ? Can the patch be split into digestable pieces ?

The makefile changes mostly correlate with -ffreestanding. I'm thinking about make the MK_SSP and MK_FORTIRY on freestanding. I plan to factor out all of the CFLAGS+= -freestanding to new "make option".

The undefs are needed, if we could commit them before the other parts of the work, than we able to split out and create a separate patch. But in testing phase I and Pedro like to keep the patch in one patch to easier test.

...
Can the pollution be minimized somehow ? Can the patch be split into digestable pieces ?

The makefile changes mostly correlate with -ffreestanding. I'm thinking about make the MK_SSP and MK_FORTIRY on freestanding. I plan to factor out all of the CFLAGS+= -freestanding to new "make option".

That is a good idea.

The undefs are needed, if we could commit them before the other parts of the work, than we able to split out and create a separate patch. But in testing phase I and Pedro like to keep the patch in one patch to easier test.

Actually the undefs are only needed if someone specifies -D_FORTIFY_SOURCE`. At least of first, this should be off by default so technically the undefs are part of the build support.

I think a good "final" test for the framework would be to build world with a recent gcc: we have already tested the rest of the paths with gcc-4.2.1 and clang. After that is done we can separate the build and basic support parts.

The diff is enormous and it pollutes a lot of unrelated places. E.g. the loader Makefiles changes or the forced undef fortify in libc/strings/stdio, as I understand, to avoid recursion.
Can the pollution be minimized somehow ? Can the patch be split into digestable pieces ?

Partly the boot loader pollution is like the pollution for SSP. I do wonder why FORTIFY conditionals were added to some places that currently do not have SSP conditionals.
I think compilation features that do not work in boot loader environments will continue to exist and grow, so a more general framework for turning them off may be interesting.

Some of them added libstand as build dependency, but do not added the -ffreestanding to the CFLAGS. One example is sys/boot/i386/libi386/Makefile.

You'd have to ask the local C standard experts but I don't think declaring a standard function inline is standard compliant. What you should do instead is declare the function normally and then define a macro with the same name that performs the necessary checks. If you don't want to put too much code into the macro you can put it into a "static inline __always_inline" function instead and call this function from the macro. I think this will solve the problems you are seeing in some ports.

The strlcpy isn't a standard function, so we could disable them, when we building c++ programs or if only a small set of the ports failed, we should fix these ports instead. I just looked into namecoins source, and they reimplement unconditionally the strlcpy function, regardless of existing on the system or not, and blindly define BSD or BSD_VISIBLE on their headers.

As a side-note: the exp-run is a regular ports build on a "fortified" system.
It doesn't force a fortification option on the ports.

In general I consider any failure there to be a commit-stopper, however
it would be understandable that some ports (like namecoins apparently)
assume that _FORTIFY_SOURCE lets them use linuxisms and in such cases we should try to clean those.

I did a detailed review, which Phabricator appears to have eaten. I'll try to summarise here, as there was some talk of committing this, and it is a long way away from being ready to go in the tree:

All of the #ifdef clang should be changed to something like __FORTIFY_WORKING_OBJECT_SIZE which can be changed in one place for clang 3.8 (which appears to contain fixes for the relevant bugs).

The implementations in the .c files are full of logic errors. If the size is not known at compile time, you're skipping doing overflow checks, even though these checks don't depend on these things. The overflow checks should be done unconditionally in the underlying functions anyway, irrespective of whether FORTIFY_SOURCE is defined.

__bos is not a human-friendly name. __bos0 is both not human-friendly and exposes implementation details of the underlying builtin. Please rename these to something human friendly.

Many of these checks could be _Static_asserts, triggering at compile time (it's braindead to abort at run time for something that the compiler knows broken at compile time).

There are a number of __predict_false for compile-time constants. These imply that the person who wrote the code doesn't understand what it does and will confuse the reader.

There is no evaluation of the performance impact of these and there are no tests for correctness. The latter is important, as it would have found some of these bugs.

I only got half way down the diff before I got tired of seeing the same mistakes repeated. It needs at least one more round of detailed review before it is ready to go in the tree.

I did a detailed review, which Phabricator appears to have eaten. I'll try to summarise here, as there was some talk of committing this, and it is a long way away from being ready to go in the tree:

I was considering a commit before your review but given the code is big I was waiting for more review.

BTW, to Oliver: please close all the other code review requests you opened it is a mess already.

All of the #ifdef clang should be changed to something like __FORTIFY_WORKING_OBJECT_SIZE which can be changed in one place for clang 3.8 (which appears to contain fixes for the relevant bugs).

Indeed, we have been in touch with Google Chrome developers, and we pointed them to that bug in particular, which is the reason why the issue is being worked on for clang-3.8. They also pointed back at us other three bugs and it's not clear they will be fixed.
I don't like such a long name for a macro. Please propose something shorter that still makes sense

The implementations in the .c files are full of logic errors. If the size is not known at compile time, you're skipping doing overflow checks, even though these checks don't depend on these things. The overflow checks should be done unconditionally in the underlying functions anyway, irrespective of whether FORTIFY_SOURCE is defined.

This will certainly have to be reviewed, however we want FORTIFY_SOURCE to be non-invasive: we will not touch the base headers or code and the replacement functions may have to repeat some of the functionality..

__bos is not a human-friendly name. __bos0 is both not human-friendly and exposes implementation details of the underlying builtin. Please rename these to something human friendly.

These are likely to be controversial no matter what we choose: after the bikeshed leading to __result_use_check I got request to go back to the original __wur and other BSDs have something completely different . In general we like to have this similar to other implementations: bionic has added new checks during all the summer and we would like to make them easier to port them.

I don't really care much about it. Let's get settled on a name : __object_size and __object_size_type0`` ?

Many of these checks could be _Static_asserts, triggering at compile time (it's braindead to abort at run time for something that the compiler knows broken at compile time).

There are a number of __predict_false for compile-time constants. These imply that the person who wrote the code doesn't understand what it does and will confuse the reader.

These great ideas, and the reason why I had wanted you to mentor this project :). These all came from either Google or NetBSD, I don't recall where.

There was at least one undocumented issue used to workaround clang inlining bugs. I think I have been thinking about waiting for the clang update before revisiting that issue.

There is no evaluation of the performance impact of these and there are no tests for correctness. The latter is important, as it would have found some of these bugs.

We do have some tests from NetBSD that haven't been integrated. I honestly am only concerned about correctness at this time. My idea from the start has been to have this disabled and consider it only as an alternative to the sanitizers.

We ran all base through this and then an exp-run over the ports tree. Some overal comments are:

The FORTIFY_SOURCE compile time checks work well only with GCC, and we carry the alloc_size attributes that are really useful for newer versions but not for GCC-4.2.1.

With GCC-4.2.1 we found two bugs: one in libdtrace and another in an snmpd module. Both were also identified by Coverity, which doesn't take merit from the checks because

Not everyone has access to Coverity.

While we have access to Coverity, the bugs were there and were real. So it's clearly good

to have another tool that can find such issues.

The runtime checks with clang found one bug, which was not detected by Coverity but was real. In order to find more similar bugs we need to exercise the code, probably through a fuzzing tool.

From the exp-run over ports:

It appears we don't have any buffer overflow within the toolchain, at least nothing that appears during normal operation. That is quite notable, IMHO.

A test that appeared to be simple and maybe even useless, namely umask_chk, found a bug in GNU emacs that was not detected by FORTIFY_SOURCE in linux.

I only got half way down the diff before I got tired of seeing the same mistakes repeated. It needs at least one more round of detailed review before it is ready to go in the tree.

I have no hurry to get this in the tree. I feel like we may not be far but I have had that
feeling for a long while and I can live with a longer test-fix-review cycle. As long as we have valuable code review (thanks David) the code will only keep improving.

The Symbol.map files are not used as the direct linker input, they are merged together, directed by the Version.def, using the awk script. The Version.def determines the versions used and the inheritance relations. This is done to make some scalability and locality for the versioning.

That said, all used versions must appear in Version.def, but individual Symbol.map files only must define actual symbol's versions.