libc: remove gets

[emaste]

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)

[emaste]

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.

[kib]

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

contrib/netbsd-tests/lib/libc/ssp/h_gets.c

43

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

[dim]

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

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]

exp-run is PR222796.

contrib/libc++/include/cstdio

77

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

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]

contrib/netbsd-tests/lib/libc/ssp/h_gets.c

43

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

[emaste]

readelf -a output collected at https://people.freebsd.org/~emaste/gets/

[kib]

I am fine with the libc bits of the patch.

[jhb]

Can this be committed already? :)

lib/libc/stdio/gets.c

48

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

[emaste]

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]

Update with kib@ and jhb@ feedback

[gerald]

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]

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

[emaste]

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:

patch-gets415 BDownload

[gerald]

In D12298#350595, @emaste wrote:

gcc port patch implementing hacky workaround:

patch-gets415 BDownload

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

[emaste]

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.

[emaste]

Reupload src diff

[emaste]

lib/libc/stdio/fgets.3

124–131

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]

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

[cy]

Fixed. Sorry for missing that.

[emaste]

rebase after rS339656

[jhb]

I think your original title was fine FWIW.

contrib/netbsd-tests/lib/libc/ssp/h_gets.c

44

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]

update from jhb

[emaste]

Updated diff (missed test change last time)