Kill gets()
Needs ReviewPublic

Authored by allanjude on Sep 9 2017, 6:03 PM.

Details

Reviewers
emaste
gnn
jhb
lidl
Summary

gets() is unsafe, and has issued a warning against its use (as a compiler warning and runtime stderr output) since its import into the FreeBSD 23+ years ago.

Switch to calling abort() and killing the calling application.

Submitted By: Paul Vixie

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 11456
Build 11812: arc lint + arc unit
allanjude created this revision.Sep 9 2017, 6:03 PM
imp added a comment.Sep 9 2017, 7:10 PM

Wouldn't it be better to just remove gets.c from Makefile.inc?

emaste added a comment.Sep 9 2017, 8:33 PM

D12298 is what I tried during Vixie's talk

jhb added a comment.Sep 9 2017, 8:34 PM

So I do think we probably need some sort of fallback unfortunately. Perhaps default to abort() (or even use abort2() to log a message), but permit an environment variable to be set to permit it to still be used for legacy applications (e.g. LIBC_UNSAFE_GETS=yes or some such, though not sure if we want a broader name for gets()). Another option might be to have a libc_unsafe.so that one can LD_PRELOAD and have the "real" gets() in there and the one in libc just do abort2().

allanjude edited the summary of this revision. (Show Details)Sep 9 2017, 8:35 PM
allanjude added inline comments.Sep 9 2017, 8:43 PM
lib/libc/stdio/gets.c
47

Is there a way to make this an #error instead of a @warn ?

lidl added a comment.Sep 10 2017, 4:24 PM

As much as I agree in spirit with the removal of gets(), or maybe just calling abort2() with
an appropriate message, I think a better course of action is to immediately change
the manpage to say that the function is inherently unsafe to use, deprecated in C11,
and will be removed in the future.

When we can compile the system with -std=c11, we might wish to have function
definition for gets() in stdio.h be bracketed by

#ifdef __STDC_VERSION__ < 201112L
char    *gets(char *);
#else
#error "gets() has been deprecated, consider using fgets() instead"
#endif

(Tweak exact syntax as needed)

I would probably change the implementation of gets() along the line
of John's suggestion, getenv("LIBC_UNSAFE_GETS"), and if that works,
use the existing gets() implementation, otherwise call abort()/abort2().

I think the LD_PRELOAD method is baggage we would have to support
for longer, and therefore not as good a solution.

We should put in a comment for the 12 relnotes that this will be the
last major numbered version of FreeBSD with gets() support... Etc,
for proper notification... If there are other c11 things that we want to
remove, we should note them now, and plan to rip them out as soon
as -current changes to 13.x

This is silly to bikeshed. 12.0 wont be released for at least a year and -CURRENT is for API breaks. Users can run the current -STABLE trees in a jail if they have any shitware they need to support for a long time. It's also trivial to maintain the patch in a corporate tree if needed, or a compat lib that doesn't live in src/.

kmacy added a subscriber: kmacy.Sep 11 2017, 11:19 PM

This is silly to bikeshed. 12.0 wont be released for at least a year and -CURRENT is for API breaks. Users can run the current -STABLE trees in a jail if they have any shitware they need to support for a long time. It's also trivial to maintain the patch in a corporate tree if needed, or a compat lib that doesn't live in src/.

Agreed. I don't understand why we're having this discussion in the context of -CURRENT. If people still need this functionality they should be running 11 or in an 11 chroot.