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.

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.

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 added a subscriber: arichardson.Oct 5 2021, 7:46 AM

arichardson added inline comments.

lib/libc/amd64/string/bcopy.c
8	`(bcopy)` will prevent function macro expansion, no need for #undef. Same for the other files.

kib added a subscriber: kib.Oct 5 2021, 11:17 AM

kib added inline comments.

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 added inline comments.Oct 5 2021, 4:05 PM

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.

emaste added a subscriber: emaste.Oct 5 2021, 8:50 PM

kevans marked an inline comment as done.Oct 5 2021, 9:44 PM

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

Harbormaster completed remote builds in B41979: Diff 96308.Oct 5 2021, 9:45 PM

lwhsu added a subscriber: lwhsu.Feb 7 2022, 1:36 AM

kevans added a reviewer: imp.Sep 25 2022, 1:37 AM

imp requested changes to this revision.Sep 25 2022, 4:57 AM

imp added inline comments.

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

This revision now requires changes to proceed.Sep 25 2022, 4:57 AM

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.

Harbormaster completed remote builds in B57112: Diff 137062.Mon, Apr 15, 7:47 PM

des added a subscriber: des.Fri, Apr 19, 10:27 AM

des added inline comments.

contrib/netbsd-tests/lib/libc/ssp/h_gets.c
37	What if it's defined to 0?

kevans added inline comments.Mon, Apr 22, 4:37 AM

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`

Just undefine gets unconditionally, note with a comment why

Remove bogus header that crept in

Harbormaster completed remote builds in B57271: Diff 137485.Mon, Apr 22, 4:42 AM

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?

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?

kevans added a reviewer: Klara.Mon, Apr 22, 7:22 PM

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

This revision is now accepted and ready to land.Mon, Apr 22, 7:25 PM

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