_FORTIFY_SOURCE support

[op]

Summary

TODO

Test Plan

make buildworld

[ngie]

Per bde's comments before on #ifdef foo vs #if defined(foo), (and the !defined(foo) idiom), the shorter versions should be used.

On a more relevant note -- why are the ssp testcases causing issues?

include/secure/_socket.h

74

Why not just use s like the manpage states?

RECV(2)                   FreeBSD System Calls Manual                  RECV(2)

NAME
     recv, recvfrom, recvmsg - receive a message from a socket

LIBRARY
     Standard C Library (libc, -lc)

SYNOPSIS
     #include <sys/types.h>
     #include <sys/socket.h>

     ssize_t
     recv(int s, void *buf, size_t len, int flags);

     ssize_t
     recvfrom(int s, void *buf, size_t len, int flags,
         struct sockaddr * restrict from, socklen_t * restrict fromlen);

     ssize_t
     recvmsg(int s, struct msghdr *msg, int flags);

include/secure/_stdio.h

2–5

This seems out of order. Shouldn't the RCS tags be below the licensing text?

include/secure/_unistd.h

3–4

This seems chronologically backwards

include/secure/security.h

3

Olivér Pintér's name has accents here, but not elsewhere. Also, I thought convention was to omit email addresses.

[ngie]

Also, csh scripts for building HardenedBSD? Ow...

I make it an effort not to install tcsh scripts on my systems. If you need help converting the scripts over, I can assist.

[pfg]

In D3043#65670, @ngie wrote:

Per bde's comments before on #ifdef foo vs #if defined(foo), (and the !defined(foo) idiom), the shorter versions should be used.

On a more relevant note -- why are the ssp testcases causing issues?

No issues, just thought we may want to enable them in our local tests: In NetBSD FORTIFY_SOURCE is part of SSP.
This said.. according to the logs the tests do not work on NetBSD either.

[pfg]

In D3043#65675, @ngie wrote:

Also, csh scripts for building HardenedBSD? Ow...

I make it an effort not to install tcsh scripts on my systems. If you need help converting the scripts over, I can assist.

Yeah.. ignore those. It's the first thing I remove every time I test the patches.

and thanks for the notes on #ifdef, I prefer the short form always but I thought it was not mandatory.

[op]

In D3043#65675, @ngie wrote:

Also, csh scripts for building HardenedBSD? Ow...

I make it an effort not to install tcsh scripts on my systems. If you need help converting the scripts over, I can assist.

I already planned to remove these scripts from the final version of the patch.

[op]

include/secure/_socket.h

74

Nice catch, I will convert them.

include/secure/_unistd.h

3–4

There are examples both these and both the reverse order in the tree: https://github.com/freebsd/freebsd/blob/master/sys/kern/imgact_elf.c

[op]

FBSD FORTIFY: add WITHOUT_FORTIFY knob description
FBSD FORTIFY: rename parameter names, requested by ngie
FBSD FORTIFY: move the RCS IDs below the license, requested by ngie
FBSD FORTIFY: add accent to my name, and remove mail address from copyright

[op]

Oliver Pinter (11):

FBSD FORTIFY: blacklist boot/powerpc/uboot from fortified build
FBSD FORTIFY: call the original functions from __vs{,n}printf_chk, and not the __*_real version
FBSD FORTIFY: drop __gets_chk, discussed with Pedro
FBSD FORTIFY: added __getcwd_chk.cpp from bionic
FBSD FORTIFY: adapt __getcwd_chk
FBSD FORTIFY: cleanup _unistd.h
FBSD FORTIFY: take __fread_chk.cpp and __fwrite_chk.cpp from android
FBSD FORTIFY: adapt __f{read,write}_chk
FBSD: fix expression in _stdio.h, this change should be a no-op
FBSD FORTIFY: change ifdef style
FBSD FORTIFY: one more round of style change

[op]

fix the style in sys too

[pfg]

include/secure/security.h

44

This should be __GNUC_PREREQ__(5, 0) for consistency.
But maybe we should have it in cdefs.h ?

[pfg]

Seen on MIPS and POWERPC (gcc-4.2 from base)

> lib/libc/tests/stdio (all)

fmemopen2_test.o: In function `atfu_test_data_length_body':
fmemopen2_test.c:(.text+0xfa4): undefined reference to `__fread_too_big_error'

• fmemopen2_test ---
• [fmemopen2_test] Error code 1

include/secure/_stdio.h

192

It looks like we should also restrict this from some gcc versions. We are using __fread_too_big_error in this path but this causing linking erros with gcc 4.2.

[op]

• fixed style issues
• added fall-back static function to older gcc compilers

[pfg]

include/secure/security.h

37

Better:
#if !defined(clang) && __GNUC_PREREQ(4, 3)

[pfg]

include/secure/security.h

37

Bah.. not so easy.. but there is something wrong there :(

[op]

• fix a typo error_attr -> errordecl

[pfg]

>>> World build completed on Thu Aug 13 21:30:52 UTC 2015

mips.mips64 buildworld completed on Thu Aug 13 21:30:52 UTC 2015
mips.mips buildworld completed on Thu Aug 13 21:30:52 UTC 2015
mips.mipsn32 buildworld completed on Thu Aug 13 21:30:52 UTC 2015
mips.mipsel buildworld completed on Thu Aug 13 21:30:55 UTC 2015

...

Great!

include/secure/security.h

37

#if __GNUC_PREREQ__(4, 3)

works here.

[pfg]

tools/build/options/WITHOUT_FORTIFY

4 ↗

(On Diff #7932)

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

[op]

• FBSD FORTIFY: fix undefined reference error with gcc-4.2 on MIPS
• FBSD FORTIFY: fix a typo error_attr -> errordecl
• FBSD FORTIFY: fix build failure GNUC_PREREQ -> __GNUC_PREREQ
• rebased against recent HEAD

[pfg]

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.

lib/libc/secure/Symbol.map

20

Not sure if it makes sense to carry these empty references.

[kib]

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 ?

lib/libc/secure/__builtin_object_size.3

48

What does it mean that the object does not have any side effects ? Is this a typo ? Or does it indeed means that the object must be PDO (doubt it) ?

lib/libc/secure/fortify_source.c

47

You do not need four comparisions, two are enough.

Look at sys/kern/kern_rangelock.c:ranges_overlap() to see how to do it.

[pfg]

In D3043#68914, @kib wrote:

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.

lib/libc/secure/__builtin_object_size.3

51

the "side effects" concept is surely vague: this manpage comes from NetBSD and it looks like it came from the GCC documentation:

https://gcc.gnu.org/onlinedocs/gcc/Object-Size-Checking.html

It doesn't really make sense for us to document compiler features. It is preferable to create a new fortify_source(7) man page explaining the effect of -D_FORTIFY_SOURCE.

[jilles]

lib/libc/secure/Symbol.map

64

The _chk functions clearly need to be exported, but does __secure_fail need to be exported?

lib/libc/secure/__builtin_object_size.3

51

I think "side effects" are things like function calls and writes to variables in the ptr expression.

However, I think this manual page documenting a pure compiler feature should not be added.

lib/libc/secure/__memccpy_chk.c

61

should have

else
        len = n;

64

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.

lib/libc/secure/__strlen_chk.c

65

Nothing was prevented here, so the message should probably read strlen: detected read past end of buffer

lib/libc/secure/fortify_source.c

47

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.

Also, this should be an inline function.

lib/libc/secure/secure_common.c

42

Please place #include "namespace.h" and #include "un-namespace.h" around the other includes (except libc_private.h which should go after). You will need to use the _ versions of some functions.

This reduces possible interposition and PLT usage in this function.

[jilles]

In D3043#68914, @kib wrote:

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.

[pfg]

In D3043#69265, @jilles wrote:

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.

The problem is that libstand and libkern have similar interfaces to libc and try to reuse the libc headers: this pulls in references to the *_chk functions so FORTIFY_SOURCE has to be disabled.

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.

NetBSD and others (linux mostly) ship with most of the OS fortified but it would be really nice to have the flexibility to choose other alternatives as well.

I have discussed with Oliver about having a single framework to activate things like the memory or address sanitizers currently in clang.

Thanks for all this feedback!

lib/libc/secure/__builtin_object_size.3

51

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 here
https://copperhead.co/2015/07/27/hardening-bionic

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

[op]

In D3043#68914, @kib wrote:

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.

[pfg]

In D3043#69481, @op wrote:

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

[op]

lib/libc/secure/fortify_source.c

47

Yes, I thinking about them too. I move then this function to include/secure/security.h file with the adjustments.

lib/libc/secure/secure_common.c

42

If I'm not wrong, all of the newly added c files needs these changes.

[op]

In D3043#69265, @jilles wrote:

In D3043#68914, @kib wrote:

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.

[op]

lib/libc/secure/fortify_source.c

47

jilles: casting of len to (const char *) does not work with clang:

 op@opn /tmp> make test
 cc -O2 -pipe -pipe   -DHARDENEDBSD  test.c  -o test
 test.c:51:37: error: invalid operands to binary expression ('const char *' and 'const char *')
        if ((uintptr_t)(a) < (uintptr_t)(b + (const char *)l) &&
                                         ~ ^ ~~~~~~~~~~~~~~~
 test.c:52:20: error: invalid operands to binary expression ('const char *' and 'const char *')
            (uintptr_t)(a + (const char *)l) > (uintptr_t)(b))
                        ~ ^ ~~~~~~~~~~~~~~~
2 errors generated.
 *** Error code 1

[op]

FBSD FORTIFY: remove unneeded man page, this page documented a compiler feature instead of the fortify source
FBSD FORTIFY: fix jilles comments and add a comment about overlapping
FBSD FORTIFY: fix jilles comment in __strlen_chk.c
FBSD FORTIFY: optimize and move __fortify_chk_overlap
FBSD FORITIFY: undef _FORTIFY_SOURCE in secure_common.c to avoid recursion
FBSD FORTIFY: added {,un-}namespace.h around includes in secure_common.c
FBSD FORTIFY: fix the build
FBSD FORTIFY: change signature

[op]

lib/libc/secure/Symbol.map

15

Jilles, Konstantin what are you thoughts about this? Should I remove these or keep them?

tools/build/options/WITHOUT_FORTIFY

4 ↗

(On Diff #8035)

Fixed in next version (commited the fix to github).

[pfg]

Note some of the issues detected by the exp-run:
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=202311

include/secure/_string.h

307

From the net-p2p/namecoin port log (using clang and libc++):

...
In file included from src/util.cpp:5:
src/strlcpy.h:23:15: error: redefinition of a 'extern inline' function 'strlcpy' is not supported in C++
inline size_t strlcpy(char *dst, const char *src, size_t siz)

^

/usr/include/secure/_string.h:306:1: note: previous definition is here
strlcpy(char * restrict _d, const char * restrict _s, size_t _n)
^
...

(This builds with FORTIFY_SOURCE=2 enabled by the upstream sources).

331

From the net-p2p/namecoin port log (using clang and libc++):
...
In file included from src/util.cpp:5:
src/strlcpy.h:58:15: error: redefinition of a 'extern inline' function 'strlcat' is not supported in C++
inline size_t strlcat(char *dst, const char *src, size_t siz)

^

/usr/include/secure/_string.h:330:1: note: previous definition is here
strlcat(char * restrict _d, const char * restrict _s, size_t _n)
^
...
(FORTIFY_SOURCE=2 is enable within the upstream code)

sys/sys/cdefs.h

565

Perhaps a parenthesis around __optimize__ >0 is pertinent here.
From the /usr/ports/mail/ifile (exp-run) log:

In file included from /usr/include/stdio.h:39:
/usr/include/sys/cdefs.h:556:43: error: invalid token at start of a preprocessor expression

defined(__OPTIMIZE__) && __OPTIMIZE__ > 0 && !defined(lint)
                                      ^

1 error generated.

---

567

Note that clang-3.8 will have fixes for higher levels of FORTIFY. (3?) so we may want to consider that case here too.

[tijl]

include/secure/_stdio.h

80

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.

[op]

include/secure/_string.h

307

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.

331

Same as the above.

[pfg]

In D3043#70047, @pfg wrote:

Note some of the issues detected by the exp-run:
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=202311

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.

Luckily there are not many cases to worry about.

include/secure/_string.h

307

FORTIFY_SOURCE depends on the system headers being used.
Of course glibc has their doze of NIH and rejected strlcat/strlcpy from their headers.

If only those are affected, it sounds like something that should be fixed in the port (and upstreamed).

[op]

sys/sys/cdefs.h

565

And seems like this is again a bug in the ports:

root@nyi-01:/usr/ports/mail/ifile/work/ifile-1.3.8 # grep -R __OPTIMIZE__ *
argp/argp-fs-xinl.c:#undef __OPTIMIZE__
argp/argp-fs-xinl.c:#define __OPTIMIZE__
argp/argp-xinl.c:#undef __OPTIMIZE__
argp/argp-xinl.c:#define __OPTIMIZE__
argp/argp.h:#ifdef __OPTIMIZE__
argp/argp.h:#endif /* __OPTIMIZE__ */
argp/argp-fmtstream.h:#ifdef __OPTIMIZE__
argp/argp-fmtstream.h:#endif /* __OPTIMIZE__ */
argp/argp.h.orig:#ifdef __OPTIMIZE__
argp/argp.h.orig:#endif /* __OPTIMIZE__ */
root@nyi-01:/usr/ports/mail/ifile/work/ifile-1.3.8 #

argp/argp-fs-xinl.c:

...
     24
     25 #define ARGP_FS_EI
     26 #undef __OPTIMIZE__
     27 #define __OPTIMIZE__
     28 #include "argp-fmtstream.h"
...

they redefine the OPTIMIZE macro with empty value.

So we should add MK_FORTIFY= no to the port's Makefile.

[pfg]

sys/sys/cdefs.h

565

Not an option: MK_FORTIFY may not be available after all. Especially not in 10.x or 9.x.

I think upstream treats __OPTIMIZE__ as boolean in some case, which is against sensible style.

Change the port to

#define __OPTIMIZE__ 0

that should be more portable,

[op]

Oliver Pinter (4):

FBSD FORTIFY: fix spelling in WITHOUT_FORTIFY
FBSD FORTIFY: update gcc build script
FBSD FORTIFY: add the ability do disable FORTIFY_SOURCE per compiler
FBSD FORTIFY: blacklist from fortified build in gcc case the libbfd, objdump and libsqlite3

[theraven]

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.

[pfg]

Hi David;

I was waiting for Oliver to comment but he has been really busy all this week.

Rest assured that your comments are being taken seriously.
From my side, I will of course not commit anything until it's ready and all comments have been addressed..

In D3043#73237, @theraven wrote:

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:

1. 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.
2. 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.

[kib]

hmm, apparently this sat in my account unsubmitted. No idea is it still useful.

lib/libc/secure/Symbol.map

15

The version_gen.awk does not need empty symbol lists AFAIR.

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.

[ngie]

Is this review still valid?

[pfg]

In D3043#205126, @ngie wrote:

Is this review still valid?

The review yes, the code is not.

This is basically what is left out of GSoC 2015 project referred to here:
https://wiki.freebsd.org/SummerOfCode2015/FreeBSDLibcSecurityExtensions

and while it's incomplete and outdated it is important that it doesn't disappear completely.