Page MenuHomeFreeBSD

libc: remove gets
ClosedPublic

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

Diff Detail

Repository
rP FreeBSD ports 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.