From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/7973 Path: news.gmane.org!not-for-mail From: Rich Felker Newsgroups: gmane.linux.lib.musl.general Subject: Re: [PATCH] Implement glibc *_chk interfaces for ABI compatibility. Date: Sat, 20 Jun 2015 16:32:48 -0400 Message-ID: <20150620203248.GZ1173@brightrain.aerifal.cx> References: <1434509291-28997-1-git-send-email-josiahw@gmail.com> Reply-To: musl@lists.openwall.com NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-Trace: ger.gmane.org 1434832385 19297 80.91.229.3 (20 Jun 2015 20:33:05 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Sat, 20 Jun 2015 20:33:05 +0000 (UTC) To: musl@lists.openwall.com Original-X-From: musl-return-7986-gllmg-musl=m.gmane.org@lists.openwall.com Sat Jun 20 22:33:04 2015 Return-path: Envelope-to: gllmg-musl@m.gmane.org Original-Received: from mother.openwall.net ([195.42.179.200]) by plane.gmane.org with smtp (Exim 4.69) (envelope-from ) id 1Z6PRo-0002j1-Gw for gllmg-musl@m.gmane.org; Sat, 20 Jun 2015 22:33:04 +0200 Original-Received: (qmail 12026 invoked by uid 550); 20 Jun 2015 20:33:02 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: Original-Received: (qmail 11981 invoked from network); 20 Jun 2015 20:33:01 -0000 Content-Disposition: inline In-Reply-To: <1434509291-28997-1-git-send-email-josiahw@gmail.com> User-Agent: Mutt/1.5.21 (2010-09-15) Original-Sender: Rich Felker Xref: news.gmane.org gmane.linux.lib.musl.general:7973 Archived-At: On Tue, Jun 16, 2015 at 09:48:11PM -0500, Josiah Worcester wrote: > diff --git a/src/compat/confstr_chk.c b/src/compat/confstr_chk.c > new file mode 100644 > index 0000000..61bcd42 > --- /dev/null > +++ b/src/compat/confstr_chk.c > @@ -0,0 +1,8 @@ > +#include > +#include > + > +size_t __confstr_chk(int name, char *buf, size_t len, size_t buflen) > +{ > + if (buflen < len) a_crash(); > + return confstr(name, buf, len); > +} I think this should be something like: size_t r = confstr(name, buf, len diff --git a/src/compat/posix_chk.c b/src/compat/posix_chk.c > new file mode 100644 > index 0000000..7e39754 > --- /dev/null > +++ b/src/compat/posix_chk.c > @@ -0,0 +1,132 @@ > +#include > +#include > +#include > +#include > +#include > +#include > +#include "atomic.h" > + > +ssize_t __read_chk(int fd, void *buf, size_t count, size_t buflen) > +{ > + if (count > buflen) a_crash(); > + return read(fd, buf, count); > +} > + > +ssize_t __readlink_chk(const char *path, void *buf, size_t bufsize, size_t buflen) > +{ > + if (bufsize > buflen) a_crash(); > + return readlink(path, buf, bufsize); > +} > + > +ssize_t __readlinkat_chk(int fd, const char *path, void *buf, size_t bufsize, size_t buflen) > +{ > + if (bufsize > buflen) a_crash(); > + return readlinkat(fd, path, buf, bufsize); > +} > + > +ssize_t __recv_chk(int fd, void *buf, size_t len, size_t buflen, int flags) > +{ > + if (len > buflen) a_crash(); > + return recv(fd, buf, len, flags); > +} > + > +ssize_t __recvfrom_chk(int fd, void *buf, size_t len, size_t buflen, int flags, struct sockaddr *addr, socklen_t *alen) > +{ > + if (len > buflen) a_crash(); > + return recvfrom(fd, buf, len, flags, addr, alen); > +} > + These all have the same issue as confstr, and need the same fix. > +ssize_t __pread_chk(int fd, void *buf, size_t size, size_t ofs, size_t buflen) > +{ > + if (size > buflen) a_crash(); > + return pread(fd, buf, size, ofs); > +} > + > +ssize_t __pread64_chk(int fd, void *buf, size_t size, size_t ofs, size_t buflen) > +{ > + return __pread_chk(fd, buf, size, ofs, buflen); > +} > + These too. > +char *__getcwd_chk(char *buf, size_t len, size_t buflen) > +{ > + if (len > buflen) a_crash(); > + return getcwd(buf, len); > +} And again. Here there's also the case of !buf in which case len is ignored. > +int __gethostname_chk(char *name, size_t len, size_t namelen) > +{ > + if (len > namelen) a_crash(); > + return gethostname(name, len); > +} Same issue. > +int __ttyname_r_chk(int fd, char *name, size_t size, size_t namelen) > +{ > + if (size > namelen) a_crash(); > + return ttyname_r(fd, name, size); > +} Same. > +char *__stpcpy_chk(char *d, const char *s, size_t dlen) > +{ > + size_t slen = strnlen(s, dlen) + 1; > + if (slen > dlen) a_crash(); > + return stpcpy(d, s); > +} This looks like it handles the issue right. > +char *__stpncpy_chk(char *d, const char *s, size_t n, size_t dlen) > +{ > + if (n > dlen) a_crash(); > + return stpncpy(d, s, n); > +} But this does it wrong again. > +wchar_t *__wcpcpy_chk(wchar_t *restrict d, const wchar_t *restrict s, size_t dlen) > +{ > + size_t slen = strnlen(s, dlen) + 1; > + if (slen > dlen) a_crash(); > + return wcpcpy(d, s); > +} strnlen is obviously wrong for wchar_t * input. Please try compiling with the -Werror options musl uses by default which will catch invalid C like this. The check looks right though with that fixed. > +wchar_t *__wcpncpy_chk(wchar_t *restrict d, const wchar_t *restrict s, size_t n, size_t dlen) > +{ > + if (n > dlen) a_crash(); > + return wcpncpy(d, s, n); > +} Same issue again. > +int __vsnprintf_chk(char *s, size_t n, int flag, size_t slen, const char *fmt, va_list ap) > +{ > + if (n > slen) a_crash(); > + return vsnprintf(s, n, fmt, ap); > +} And again here. > +int __vsprintf_chk(char *s, int flag, size_t slen, const char *fmt, va_list ap) > +{ > + int ret; > + if (slen == 0) a_crash(); > + ret = vsnprintf(s, slen, fmt, ap); > + if (ret > slen) a_crash(); > + return ret; > +} That should be ret>=slen, because ret does not include the null termination. But you also need to avoid crashing when ret<0, which is a valid error return. The implicit conversion to size_t would make all errors crash because -1 is > any size_t except SIZE_MAX. > diff --git a/src/compat/realpath_chk.c b/src/compat/realpath_chk.c > new file mode 100644 > index 0000000..cc0089e > --- /dev/null > +++ b/src/compat/realpath_chk.c > @@ -0,0 +1,9 @@ > +#include > +#include > +#include "atomics.h" > + > +char *__realpath_chk(const char *filename, char *resolved, size_t resolved_len) > +{ > + if (resolved_len < PATH_MAX) a_crash(); > + return realpath(filename, resolved); > +} Aside from having the same issue as above, this ignores the case of passing a null pointer, which is always valid. I think the only way to make this function safe is to use a temp buffer of size PATH_MAX on the stack and memcpy the result or crash if strlen is > PATH_MAX. > diff --git a/src/compat/stdio_chk.c b/src/compat/stdio_chk.c > new file mode 100644 > index 0000000..53e514c > --- /dev/null > +++ b/src/compat/stdio_chk.c > @@ -0,0 +1,50 @@ > +#include > +#include > +#include "atomic.h" > +#include "libc.h" > +#include "stdio_impl.h" > + > +char *__fgets_chk(char *s, size_t size, int n, FILE *f) > +{ > + if ((size_t)n > size) a_crash(); > + return fgets(s, n, f); > +} > + > +wchar_t *__fgetws_chk(wchar_t *s, size_t size, int n, FILE *f) > +{ > + if ((size_t)n > size) a_crash(); > + return fgetws(s, n, f); > +} > + > +size_t __fread_chk(void *restrict destv, size_t destvlen, size_t size, size_t nmemb, FILE *restrict f) > +{ > + if (size != 0 && (size * nmemb) / size != nmemb) a_crash(); > + if (size * nmemb > destvlen) a_crash(); > + return fread(destv, size, nmemb, f); > +} Same issue in these. > +char *__gets_chk(char *buf, size_t size) > +{ > + char *ret = buf; > + int c; > + FLOCK(stdin); > + if (!size) return NULL; > + for(;size;buf++,size--) { > + c = getc(stdin); > + if ((c == EOF && feof(stdin)) || c == '\n') { > + *buf = 0; > + FUNLOCK(stdin); > + return ret; > + } > + if (c == EOF) { > + FUNLOCK(stdin); > + return NULL; > + } > + *buf = c; > + } > + a_crash(); > +} I would mildly prefer using flockfile/funlockfile (POSIX namespace) to using stdio internals here, but it's not a big deal. gets is no longer in the C namespace so that seems valid to me (as long as it's moved). > diff --git a/src/compat/string_chk.c b/src/compat/string_chk.c > new file mode 100644 > index 0000000..ba77c9c > --- /dev/null > +++ b/src/compat/string_chk.c > @@ -0,0 +1,98 @@ > +#include > +#include > +#include "atomic.h" > + > +void *__memcpy_chk(void *restrict dest, const void *restrict src, size_t n, size_t destlen) > +{ > + if (n > destlen) a_crash(); > + return memcpy(dest, src, n); > +} > + > +void *__memmove_chk(void *restrict dest, const void *restrict src, size_t n, size_t destlen) > +{ > + if (n > destlen) a_crash(); > + return memmove(dest, src, n); > +} > + > +void *__memset_chk(void *dest, int c, size_t n, size_t destlen) > +{ > + if (n > destlen) a_crash(); > + return memset(dest, c, n); > +} These are okay because they write exact-length n. > +char *__strcat_chk(char *restrict dest, const char *restrict src, size_t destlen) > +{ > + size_t tot; > + tot = strnlen(src, destlen); > + if (tot > SIZE_MAX - strnlen(dest, destlen) - 1) a_crash(); > + tot += strnlen(dest, destlen) + 1; > + if (tot > destlen) a_crash(); > + return strcat(dest, src); > +} > + > +char *__strncat_chk(char *restrict dest, const char *restrict src, size_t n, size_t destlen) > +{ > + size_t tot; > + tot = strnlen(dest, destlen); > + if (tot > SIZE_MAX - strnlen(src, n) - 1) a_crash(); > + tot += strnlen(src, n) + 1; > + if (tot > destlen) a_crash(); > + return strncat(dest, src, n); > +} I think these are right but I'll review them again before commit. > +char *__strncpy_chk(char *restrict dest, const char *restrict src, size_t n, size_t destlen) > +{ > + if (n > destlen) a_crash(); > + return strncpy(dest, src, n); > +} This is okay because strncpy is an exact-length n write. > +wchar_t *__wcscat_chk(wchar_t *restrict dest, const wchar_t *src, size_t destlen) > +{ > + size_t tot; > + tot = wcsnlen(src, destlen); > + if (tot > SIZE_MAX - wcsnlen(dest, destlen) - 1) a_crash(); > + tot += wcsnlen(dest, destlen) + 1; > + if (tot > destlen) a_crash(); > + return wcscat(dest, src); > +} > + > +wchar_t *__wcscpy_chk(wchar_t *restrict dest, const wchar_t *restrict src, size_t destlen) > +{ > + size_t srclen = wcsnlen(src, destlen) + 1; > + if (srclen > destlen) a_crash(); > + return wcscpy(dest, src); > +} > + > +wchar_t *__wcsncat_chk(wchar_t *restrict dest, const wchar_t *restrict src, size_t n, size_t destlen) > +{ > + size_t tot; > + tot = wcsnlen(dest, destlen); > + if (tot > SIZE_MAX - wcsnlen(src, n) - 1) a_crash(); > + tot += wcsnlen(dest, destlen) + 1; > + if (tot > destlen) a_crash(); > + return wcsncat(dest, src, n); > +} > + > +wchar_t *__wcsncpy_chk(wchar_t *restrict dest, const wchar_t *restrict src, size_t n, size_t destlen) > +{ > + if (n > destlen) a_crash(); > + return wcsncpy(dest, src, n); > +} > + > +wchar_t *__wmemcpy_chk(wchar_t *restrict d, const wchar_t *restrict s, size_t n, size_t dlen) > +{ > + if (n > dlen) a_crash(); > + return wmemcpy(d, s, n); > +} > + > +wchar_t *__wmemmove_chk(wchar_t *d, const wchar_t *s, size_t n, size_t dlen) > +{ > + if (n > dlen) a_crash(); > + return wmemmove(d, s, n); > +} > + > +wchar_t *__wmemset_chk(wchar_t *d, wchar_t c, size_t n, size_t dlen) > +{ > + if (n > dlen) a_crash(); > + return wmemset(d, c, n); > +} I didn't notice any problem in the rest of these. > diff --git a/src/compat/ttyname_r_chk.c b/src/compat/ttyname_r_chk.c > new file mode 100644 > index 0000000..50e70e5 > --- /dev/null > +++ b/src/compat/ttyname_r_chk.c > @@ -0,0 +1,7 @@ > +#include > + > +int __ttyname_r_chk(int fd, char *name, size_t size, size_t namelen) > +{ > + if (size > namelen) a_crash(); > + return ttyname_r(fd, name, size); > +} Same issue again. > diff --git a/src/compat/wchar_chk.c b/src/compat/wchar_chk.c > new file mode 100644 > index 0000000..66e35b1 > --- /dev/null > +++ b/src/compat/wchar_chk.c > @@ -0,0 +1,45 @@ > +#include > +#include > +#include "atomic.h" > + > +size_t __mbsnrtowcs_chk(wchar_t *restrict wcs, const char **restrict src, size_t n, size_t wn, mbstate_t *restrict st, size_t wcslen) > +{ > + if (wn > wcslen) a_crash(); > + return msbnrtowcs(wcs, src, n, wn, st); > +} > + > +size_t __mbsrtowcs_chk(wchar_t *restrict ws, const char **restrict src, size_t wn, mbstate_t *restrict st, size_t wslen) > +{ > + if (wn > wslen) a_crash(); > + return mbsrtowcs(ws, src, wn, st); > +} > + > +size_t __mbstowcs_chk(wchar_t *restrict ws, const char *restrict s, size_t wn, size_t wslen) > +{ > + if (wn > wslen) a_crash(); > + return mbstowcs(ws, s, wn); > +} > + > +size_t __wcrtomb_chk(char *restrict s, wchar_t wc, mbstate_t *restrict st, size_t slen) > +{ > + if (slen < MB_CUR_MAX) a_crash(); > + return wcrtomb(s, wc, st); > +} > + > +size_t __wcsnrtombs_chk(char *restrict dst, const wchar_t **restrict wcs, size_t wn, size_t n, mbstate_t *restrict st, size_t dstlen) > +{ > + if (n > dstlen) a_crash(); > + return wcsnrtombs(dst, wcs, wn, n, st); > +} > + > +size_t __wcstombs_chk(char *restrict s, const wchar_t *restrict ws, size_t n, size_t slen) > +{ > + if (n > slen) a_crash(); > + return wcstombs(s, ws, n); > +} > + > +int __wctomb_chk(char *s, wchar_t wc, size_t slen) > +{ > + if (slen < MB_CUR_MAX) a_crash(); > + return wctomb(s, wc); > +} And all of these. Rich