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)
Details
- Reviewers
gnn ngie allanjude kib eadler cy jhb - Group Reviewers
manpages - Commits
- rS351659: libc: remove gets
rP479233: Forward port r478722 | gerald | 2018-09-01 from lang/gcc7:
rP478752: Forward port r478722 | gerald | 2018-09-01 from lang/gcc7:
rP478722: Disable the build/use of libssp/gets-chk since FreeBSD 12 and later
Diff Detail
- Lint
Lint Skipped - Unit
Tests Skipped
Event Timeline
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.
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. |
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. |
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. |
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. |
Can this be committed already? :)
lib/libc/stdio/gets.c | ||
---|---|---|
48 | Can this be marked static? AFAIK there's no prototype for __gets_unsafe(). |
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.
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.
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:
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.
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. |