Import _FORTIFY_SOURCE implementation from NetBSD

[kevans]

Summary

This is a mostly-unmodified copy of the various *_chk implementations
and headers from NetBSD, without yet modifying system headers to start
actually including them. A future commit will also apply the needed
bits to fix ssp/unistd.h.

[imp]

include/ssp/string.h

100

This is always true...

include/ssp/unistd.h

40

why redirect read(2) and none of the other system calls that write back into user space?

I don't see any others in NetBSD's either. Expanding it is a big ask, so before I really do that as more than maybe a suggestion, I'd like to know the thinking here.

[kevans]

include/ssp/string.h

100

Fair point; I'll hack it back out in the second patch that reworks part of these headers for FreeBSD (with a brief explanation that it's tautological here)

include/ssp/unistd.h

40

I think they more or less tried to stick with the common _FORTIFY_SOURCE implementation (glibc). We can expand it, but IMO let's get to the baseline and define expanding it as future work (explicitly in the commit message) -- the baseline should be 'relatively safe' as folks have been running with glibc's implementation for years as it is.

[kib]

Since this code goes directly into our libc source, and not into vendor import area, should it follow style(9)? The most systematic thing I noted is return a; instead of return (a); Another thing is the headers inclusion order.

include/ssp/ssp.h

38

I do not think lint is relevant for FreeBSD.

lib/libc/secure/sprintf_chk.c

47

These lint-style comments (as well LINTLIBRARY) are not relevant either.

[imp]

include/ssp/unistd.h

40

The counter you your argument may also be that fortify_source was done years ago and is showing its age :)....
But what you suggest works for me, so long as we understand that followup work might be good to increase the coverage. Mostly I wanted to understand why it was incomplete.

[kevans]

Straighten out some style and other bits:

• clang || GCC >= 4.8 is a given
• Remove irrelevant LINT/lint bits
• Shuffle around includes in *_chk.c, alphabetical order with ssp/* headers last
• Remove spurious prototypes in *_chk.c; these should be provided in ssp/*
• return x => return (x)

[emaste]

lib/libssp/Versions.def

2

Is this true?

[kevans]

lib/libssp/Versions.def

2

I added the comment based on the greatest of technicalities, that it appeared in our version of libssp in 13.0-CURRENT (since I added our version of libssp in cd0d51baaa4509a1d); happy enough to revise it, though.

[emaste]

lib/libssp/Versions.def

2

Ok it's fine with me 👍

[gbe]

lib/libssp/ssp.3

122

Would be good if you could also mention FreeBSD here.

[pauamma_gundo.com]

lib/libssp/__builtin_object_size.3

101–102

Matters word order. :-)

lib/libssp/ssp.3

121

"in" missing unless .Nx does more than what I remember .Fx doing. (Maybe let upstream know as well?)

[kevans]

• Fix word order
• "in" before .Nx
• Mention FreeBSD (had GNU version, this version came in in 12.2, enhancements to be in... 14.0)

[pauamma_gundo.com]

Manual pages LGTM. Can't attest to the source or the consistency between them.

[pauamma_gundo.com]

Why didn't I approve it last No... oh right; didn't know I could or should.

[imp]

I think this is fine, modulo notes about its completeness, but I'm OK with that and it's not bad.

[kevans]

In D32306#833118, @imp wrote:

I think this is fine, modulo notes about its completeness, but I'm OK with that and it's not bad.

I'm updating the other reviews to address pending comments; I'd like to land this initial batch that matches what NetBSD has, then I'll start going through other existing headers and see what else we can add.

[imp]

In D32306#1020932, @kevans wrote:

In D32306#833118, @imp wrote:

I think this is fine, modulo notes about its completeness, but I'm OK with that and it's not bad.

I'm updating the other reviews to address pending comments; I'd like to land this initial batch that matches what NetBSD has, then I'll start going through other existing headers and see what else we can add.

I'm good with that.

[kib]

So overall this stuff is in principle incompatible with the code like memcmp(*a++, *b++, XXX) ?

include/ssp/ssp.h

83

Can we use gcc ({}) extension there to avoid multiple evaluations for the args?
It still would cause multi-eval when real function is called after __ssp_overlap, though.

[kib]

lib/libc/secure/Symbol.map

7

This should become 1.8

lib/libc/secure/gets_chk.c

43

extern is not needed

[kib]

lib/libssp/Versions.def

6

15.0

[kevans]

In D32306#1024959, @kib wrote:

So overall this stuff is in principle incompatible with the code like memcmp(*a++, *b++, XXX) ?

It doesn't need to be, I'll just need to fix the existing macros to avoid multiple evaluations or, perhaps better, go ahead and convert them all to static inlines.

include/ssp/ssp.h

83

We could, though it looks like __ssp_overlap is really only intended to be used in libc so I think I'll move it into an ssp_private.h in libc.

[kevans]

Fix some nits
Switch to copound statements to avoid double evaluation

[kib]

Do we need to put SPD tags in the added files?

lib/libssp/__builtin_object_size.3

48

What does it mean for the object to not have a side effect?

[kevans]

lib/libssp/__builtin_object_size.3

48

I'll revise this based on experimentation; the point is, as you probably guessed, that ptr is really just an expression that may or may not have side effects. Consider this example, which must be built with -O1 or higher:

#include <stdio.h>                                                                                            
                                                                                                              
int                                                                                                           
main()                                                                                                        
{                                                                                                             
    char buf[64], *p = buf;                                                                                   
                                                                                                              
    printf("%zu\n", __builtin_object_size(p, 0));                                                             
    printf("%zu\n", __builtin_object_size(++p, 0));                                                           
}

The first printf() gets you 64, the second one gets you (size_t)-1 and p remains unadvanced; this seems like a further argument for the conversion to compound statements, easier to reason about and avoids disabling fortification because of a technicality.

[kevans]

Add SPDX tags

Revise _builtin_object_size(3):

• Note clang(1) as well, we'll keep GCC enumerated as it originated there and it's worth it to remain clear that this still works with a GCC build.
• Try to make it clearer that we're talking about the expression that makes up the obj argument.

[kib]

include/ssp/string.h

38

Not needed

lib/libc/secure/fgets_chk.c

36

We have both __RCSID and the first line. Can we leave only one?

lib/libc/secure/gets_chk.c

36

same

lib/libc/secure/memcpy_chk.c

36

same

lib/libssp/__builtin_object_size.3

52

object referenced by
.Fa ptr
?

[kevans]

include/ssp/string.h

57

This doesn't quite work because of, e.g., gems like this from fortune:

460 |                 strcat(strcat(strcpy(tpath, dir), "/"), file);

[kevans]

Address review feedback:

• Remove redundant <sys/cdefs.h>
• Strip redundant __RCSID tags from libc/secure source files
• Slight re-word in __builtin_object_size(3)
• Uglify the macros a little further to avoid variable shadowing in nested calls, since we seem to have some of those, using COUNTER (at least supported by gcc/clang). While we're here, a couple of double-evaluations were pointed out and fixed.

[pauamma_gundo.com]

lib/libssp/__builtin_object_size.3

80

This looks stilted to me. Maybe "knows can be pointed"?

lib/libssp/ssp.3

100

Does this apply to clang as well? If so, I'd mention both.

115

Add clang here if added above.

[kib]

lib/libssp/ssp.3

128

Did it?

[kevans]

lib/libssp/ssp.3

128

This is referring to https://svnweb.freebsd.org/base/?view=revision&revision=r356356, it just didn't get a man page back then and these symbols aborted.

[saper_saper.info]

Related, mostly about documenting this: https://lists.freebsd.org/archives/freebsd-current/2024-May/005969.html

[kevans]

In D32306#1032957, @saper_saper.info wrote:

Related, mostly about documenting this: https://lists.freebsd.org/archives/freebsd-current/2024-May/005969.html

We wouldn't typically force a rebuild of everything for something like this (use of these additional symbols doesn't cause ABI issues anywhere, no automatic feature detection is needed) , but I suppose I could throw something in src/UPDATING to mention it? It doesn't really need special care, though, just the procedure you'd typically need to follow for living on main...

[emaste]

In D32306#1032991, @kevans wrote:

I could throw something in src/UPDATING to mention it?

That sounds good (and sufficient) to me - just providing a heads-up that some special action may be needed.