Prepare the system for _FORTIFY_SOURCE

[kevans]

Summary

Notably:

• libc needs to #undef some of the macros from ssp/* for underlying implementations
• ssp/* wants a __RENAME() macro (snatched more or less from NetBSD)

There's some extra hinkiness included for read(), since libc spells it
as "_read" while the rest of the world spells it "read."

[imp]

Re _read, I thought all system calls had three functions foo, _foo and sys_foo depending on which layer of indirection you were trying to bypass... I know that's why libc uses things like _read and _write.

[kevans]

In D32307#729071, @imp wrote:

Re _read, I thought all system calls had three functions foo, _foo and sys_foo depending on which layer of indirection you were trying to bypass... I know that's why libc uses things like _read and _write.

Right, sorry, I need to update that comment.

[imp]

In D32307#729072, @kevans wrote:

In D32307#729071, @imp wrote:

Re _read, I thought all system calls had three functions foo, _foo and sys_foo depending on which layer of indirection you were trying to bypass... I know that's why libc uses things like _read and _write.

Right, sorry, I need to update that comment.

You've likely already thought of this, but if not... do we need to _FORTIFY_SOURCE system calls that FreeBSD has that are 'read like' that aren't in NetBSD?

[arichardson]

lib/libc/amd64/string/bcopy.c

10

(bcopy) will prevent function macro expansion, no need for #undef. Same for the other files.

[kib]

lib/libc/Makefile

22

I am not sure about all current and future uses of this define, but I dislike it being taken from the user namespace.

Usually we use something like _IN_LIBC

[kevans]

lib/libc/Makefile

22

It also wouldn't be used much, but I wonder if it'd be better to just -D_FORTIFY_SOURCE_read=_read and default _FORTIFY_SOURCE_read to read in the header.

[kevans]

Just prevent macro expansion, and use -D_FORTIFY_SOURCE_read instead of defining
some indicator for "in libc" to be misused later.

[imp]

lib/libc/sys/read.c

39 ↗

(On Diff #96308)

you only changed an include...
and nothing else...
So what does this accomplish?

sys/sys/cdefs.h

615–616

I'm torn on this #else...
It means that no compilers that don't define PCC nor GNUC will be an automatic error
EVEN IF THEY DONT USE __RENAME()

So that's bad, right? I'd be tempted to either leave it undefined or # it like you do for kernel standalone....

[kevans]

• Remove include that was shadily added that doesn't seem to be needed now
• Don't break with __RENAME() use unless it's used

The latter means these compilers can still build against our headers as long as
they don't enable _FORTIFY_SOURCE.

[des]

contrib/netbsd-tests/lib/libc/ssp/h_gets.c

37

What if it's defined to 0?

[kevans]

contrib/netbsd-tests/lib/libc/ssp/h_gets.c

37

Meh, why not just #undef it unconditionally? We're not bringing gets() back, and if we do as a macro then it's still going to blow up the below __sym_compat

[kevans]

Just undefine gets unconditionally, note with a comment why

Remove bogus header that crept in

[imp]

I think a rebase is in order... this diff is picking up a lot of "noise" that have nothing to do with fortify....

lib/libc/Makefile

72

seems like this could be its own thing, no?

77

Here (and elsewhere) this change could be its own commit.

118

This is a good change, but could be separate. It's not related to foritfy.

132

nice cleanup, also independent of fortify.

lib/libc/amd64/string/bzero.c

4–5

is this really still around. Yikes!

lib/libc/stdio/sprintf.c

38–39

This stuff is long gone... rebase needed?

[kevans]

In D32307#1023676, @imp wrote:

I think a rebase is in order... this diff is picking up a lot of "noise" that have nothing to do with fortify....

This set of comments doesn't seem to make much sense when viewed on the current version of the review... I suspect it might have produced on the diff of old/new that brought it forward 3 years?

[imp]

this looks good to me now (forget about my other comments... I clicked some something weird maybe?)

lib/libc/Makefile

72

nevermind.. Don't know what I clicked on, but there were lots of noise like this...

[ngie]

I’m not sure how one would go about communicating this change, since it mostly affects developers (Relnotes: yes?), but I would probably send out a heads up about some of the function definitions changing (the extra ellipses), and provide rationale for why they’re changing in the commit. It could confuse developers who use more naive methods of searching for function definitions (like me :)..). All the more reason to encourage others to use alternative tools like cscope or VSCode :).