Page MenuHomeFreeBSD

Import _FORTIFY_SOURCE implementation from NetBSD
Needs ReviewPublic

Authored by kevans on Oct 5 2021, 3:47 AM.

Details

Reviewers
None
Group Reviewers
manpages
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.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 41976
Build 38864: arc lint + arc unit

Event Timeline

kevans requested review of this revision.Oct 5 2021, 3:47 AM
include/ssp/string.h
99

This is always true...

include/ssp/unistd.h
39

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.

include/ssp/string.h
99

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
39

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.

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
37

I do not think lint is relevant for FreeBSD.

lib/libc/secure/sprintf_chk.c
46

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

include/ssp/unistd.h
39

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 marked 3 inline comments as done.

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 added inline comments.
lib/libssp/Versions.def
3

Is this true?

lib/libssp/Versions.def
3

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.

lib/libssp/Versions.def
3

Ok it's fine with me 👍

gbe added inline comments.
lib/libssp/ssp.3
121

Would be good if you could also mention FreeBSD here.

pauamma_gundo.com added inline comments.
lib/libssp/__builtin_object_size.3
100–101

Matters word order. :-)

lib/libssp/ssp.3
120

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