Page MenuHomeFreeBSD

libc: remove gets
ClosedPublic

Authored by emaste on Sep 9 2017, 8:33 PM.

Details

Summary
gets is unsafe and shouldn't be used (for many years).  Leave it in the
existing symbol version so anything that previously linked aginst it
still runs, but do not allow new software to link against it.

(The compatability/legacy implementation must not be static so that
the symbol and in particular the compat sym gets@FBSD_1.0 make it
into libc.)

 PR:             222796 (exp-run)
 Reported by:    Paul Vixie
 Reviewed by:    allanjude, eadler, gnn, kib, ngie (all earlier)

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

emaste created this revision.Sep 9 2017, 8:33 PM
emaste mentioned this in D12295: Kill gets().
gnn accepted this revision.Sep 9 2017, 8:44 PM
This revision is now accepted and ready to land.Sep 9 2017, 8:44 PM
emaste updated this revision to Diff 33726.Oct 5 2017, 6:23 PM

leave gets in old symver

gets is unsafe and shouldn't be used (for many years). Leave it in the
existing symbol version so anything that previously linked aginst it
still runs, but do not allow new software to link against it.

This revision now requires review to proceed.Oct 5 2017, 6:23 PM
ngie accepted this revision.Oct 6 2017, 3:47 AM
This revision is now accepted and ready to land.Oct 6 2017, 3:47 AM
allanjude accepted this revision.Oct 6 2017, 3:57 AM
kib added a subscriber: kib.Oct 6 2017, 12:52 PM

Can you put the readelf -a output from the patched libc somewhere ?

contrib/netbsd-tests/lib/libc/ssp/h_gets.c
43 ↗(On Diff #33726)

Why this chunk is needed ? Esp. the compat definition.

dim added a comment.Oct 6 2017, 1:46 PM

I take it an exp-run will be done, or has already been done? There's surely some old software in ports that uses this function. :)

contrib/libc++/include/cstdio
77 ↗(On Diff #33726)

I'm not sure if this should be unconditional. If somebody insists on using C++11 or earlier, gets should be available, even if it is unwise to use. (The same could be said for e.g. strcpy and the like.)

If it is conditional on the C++ version, this change could also be upstreamed.

emaste added a comment.Oct 6 2017, 3:12 PM

exp-run is PR222796.

contrib/libc++/include/cstdio
77 ↗(On Diff #33726)

Well, in this review we've explicitly made gets unavailable, regardless of what ISO C or POSIX say, and the same argument applies to C++.

contrib/netbsd-tests/lib/libc/ssp/h_gets.c
43 ↗(On Diff #33726)

This gets() test case won't link unless I can access the actual symbol. Is there another way to explicitly call gets@FBSD1.0? I thought about just dropping the test case altogether, but it seems reasonable to keep it.

kib added inline comments.Oct 6 2017, 3:24 PM
contrib/netbsd-tests/lib/libc/ssp/h_gets.c
43 ↗(On Diff #33726)

Ah, so you want to test the semi-hidden libc function still ? I think you need to explain the intent in the comment somewhere.

kib accepted this revision.Oct 6 2017, 7:18 PM

I am fine with the libc bits of the patch.

eadler accepted this revision.Jan 7 2018, 5:46 PM
jhb added a comment.Jul 3 2018, 6:27 PM

Can this be committed already? :)

lib/libc/stdio/gets.c
47 ↗(On Diff #33726)

Can this be marked static? AFAIK there's no prototype for __gets_unsafe().

emaste added a comment.Jul 3 2018, 7:18 PM

I requested an exp-run in PR222796. I think we could deal with most of the failures post-commit, but a few gcc ports fail and we should probably address those in advance.

emaste updated this revision to Diff 44825.Jul 3 2018, 7:21 PM

Update with kib@ and jhb@ feedback

This revision now requires review to proceed.Jul 3 2018, 7:21 PM
cy added a subscriber: cy.Jul 3 2018, 7:36 PM
kib accepted this revision.Jul 4 2018, 3:07 PM
This revision is now accepted and ready to land.Jul 4 2018, 3:07 PM
emaste marked 3 inline comments as done.Jul 4 2018, 3:09 PM
emaste added a subscriber: gerald.
gerald added a comment.Jul 4 2018, 4:09 PM

I believe the case of libssp in GCC is not that easy.

Looking into this I found the snippet in gcc/libssp/gets-chk.c which I assume is what triggers the current warning or linker error:

char *__gets_chk (char *s, size_t slen)
{

char *ret, *buf;

if (slen >= (size_t) INT_MAX)
  return gets (s);

if (slen <= 8192)
  buf = alloca (slen + 1);
else
  buf = malloc (slen + 1);
if (buf == NULL)
  return gets (s);

ret = fgets (buf, (int) (slen + 1), stdin);
if (ret != NULL)
  {
    size_t len = strlen (buf);
    if (len > 0 && buf[len - 1] == '\n')
      --len;
    if (len == slen)
      __chk_fail ();
    memcpy (s, buf, len);
    s[len] = '\0';
    ret = s;
  }

if (slen > 8192)
  free (buf);
return ret;

}

As you see, gets() is only used as a fall-back when memory allocation fails or would be too big.

I'll drop a note to the author(s), but don't see how to go about this (and in any case FreeBSD-specific patches are not what we should be going for if we can help it in any way. Been there, had to work with it, got the bruises.

emaste added a comment.Jul 4 2018, 4:37 PM

Just tried building lang/gcc7 on my gets-less dev system and see:

/usr/home/emaste/src/freebsd-ports/lang/gcc7/work/gcc-7.3.0/libssp/gets-chk.c: In function '__gets_chk':
/usr/home/emaste/src/freebsd-ports/lang/gcc7/work/gcc-7.3.0/libssp/gets-chk.c:67:12: warning: implicit declaration of function 'gets'; did you mean 'getw'? [-Wimplicit-function-declaration]
     return gets (s);
            ^~~~
            getw
/usr/home/emaste/src/freebsd-ports/lang/gcc7/work/gcc-7.3.0/libssp/gets-chk.c:67:12: warning: return makes pointer from integer without a cast [-Wint-conversion]
     return gets (s);
            ^~~~~~~~
/usr/home/emaste/src/freebsd-ports/lang/gcc7/work/gcc-7.3.0/libssp/gets-chk.c:74:12: warning: return makes pointer from integer without a cast [-Wint-conversion]
     return gets (s);
            ^~~~~~~~

so the package will still build, but libssp will have an unresolveable reference to gets.

Probably the right solution is to add a configure test to check for the presence of gets() and include gets-chk.c in libssp only if it exists (and presumably making the compiler-side support conditional on it as well).

As a hacky workaround something like

#if defined(__FreeBSD__) && __FreeBSD__ >= 12
#define gets(s) abort();
#endif

The hacky workaround would have to be something like #define gets(s) abort(), NULL instead, but I am unable to verify the gets -> __gets_chk transformation works in the first place. Maybe the GCC build does not enable this on FreeBSD?

gcc port patch implementing hacky workaround:

gerald added a comment.Aug 5 2018, 9:41 PM

gcc port patch implementing hacky workaround:

I've got an idea for a different approach that I'm testing right now and plan on
committing to lang/gcc7 (the default port of GCC in the Ports Collection) to
help unstall this change.

Also, I've reached out upstream (GCC) and ideally we can get this addressed
for GCC9 and backported to GCC 8:
https://gcc.gnu.org/ml/gcc/2018-08/threads.html#00023

Reply on mailing list thread:

If the system doesn't have gets, you will not need __gets_chk, either.

I suppose we need a configure test in gcc for the existence of gets and just avoid building the offending file altogether.

This revision was automatically updated to reflect the committed changes.
emaste reopened this revision.Oct 23 2018, 1:10 PM
emaste updated this revision to Diff 49502.Oct 23 2018, 1:15 PM

Reupload src diff

emaste marked an inline comment as done.Oct 23 2018, 1:18 PM
emaste added inline comments.
lib/libc/stdio/fgets.3
124 ↗(On Diff #49502)

Do we need a The function gets_s may also fail and set errno for any of the errors specified for the routine getchar.? This text existed for gets() and not updated to reference gets_s when it was added. @cy?

cy added a comment.Oct 23 2018, 2:10 PM

Yes, this is also true for gets_s(). I will fix that right away.

cy added a comment.Oct 23 2018, 2:16 PM

Fixed. Sorry for missing that.

cy accepted this revision.Oct 23 2018, 2:20 PM
This revision is now accepted and ready to land.Oct 23 2018, 2:20 PM
emaste updated this revision to Diff 49507.Oct 23 2018, 3:04 PM
emaste retitled this revision from libc: remove gets to libc: remove gets (from default symbol version).
emaste edited the summary of this revision. (Show Details)

rebase after rS339656

This revision now requires review to proceed.Oct 23 2018, 3:04 PM
jhb added a comment.Oct 23 2018, 3:36 PM

I think your original title was fine FWIW.

contrib/netbsd-tests/lib/libc/ssp/h_gets.c
44 ↗(On Diff #49507)

I would perhaps move the __sym_compat line up below the unsafe_gets() line and then add a blank line before the gets() wrapper function. You could then amend the last sentence in the comment perhaps to something like:

* The next two lines create an unsafe_gets() function that resolves to gets@FBSD_1.0.

I just find this bit of code non-obvious and think this might make it a bit more clear, but it might also be fine as-is. It is up to you.

emaste updated this revision to Diff 49610.Oct 25 2018, 1:56 PM
emaste retitled this revision from libc: remove gets (from default symbol version) to libc: remove gets.

update from jhb

emaste updated this revision to Diff 49611.Oct 25 2018, 2:10 PM

Updated diff (missed test change last time)

jhb accepted this revision.Oct 25 2018, 5:44 PM
This revision is now accepted and ready to land.Oct 25 2018, 5:44 PM
This revision was automatically updated to reflect the committed changes.