From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/7952 Path: news.gmane.org!not-for-mail From: Josiah Worcester Newsgroups: gmane.linux.lib.musl.general Subject: Re: [PATCH] Implement glibc *_chk interfaces for ABI compatibility. Date: Wed, 17 Jun 2015 14:07:08 -0500 Message-ID: References: <1434509291-28997-1-git-send-email-josiahw@gmail.com> <20150617093056.GD22285@port70.net> Reply-To: musl@lists.openwall.com NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 X-Trace: ger.gmane.org 1434568054 18856 80.91.229.3 (17 Jun 2015 19:07:34 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Wed, 17 Jun 2015 19:07:34 +0000 (UTC) To: musl@lists.openwall.com, Josiah Worcester Original-X-From: musl-return-7965-gllmg-musl=m.gmane.org@lists.openwall.com Wed Jun 17 21:07:34 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 1Z5IgF-0004TH-9h for gllmg-musl@m.gmane.org; Wed, 17 Jun 2015 21:07:23 +0200 Original-Received: (qmail 24197 invoked by uid 550); 17 Jun 2015 19:07:20 -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 24175 invoked from network); 17 Jun 2015 19:07:20 -0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :content-type; bh=404g4JiaZDJ0BBQbnGaQSK/8+sanleDQl7VPsWhdNkw=; b=boYrRih7c2mhQfJnL2SoPxvrl7Mmco2mttNeXH9NtuDY4B+0X7gOr2CIz9ceZwIeWU NgTVSRveYkRSMYIpfHOPpXu2Gxh5nVnFvgIupumxfQ2Bgj6N7+ITR7KO8/DODFScljOH 14DT8e8urA5KwNoYXdCjHPQEBGnvXgWW2N0gaGueV+D/O5YFkgmj3XJ3i0LYs61tERNT /Qe/ooeBWc/XXJkIXjZ9/O0DRpgMsXm62CzShBGqHWuKUxGJ2L5VrSW9FW2cbQDHJ2uj Zqi0GxSZhC6nVSrecrGRhQvVyj0EqZUhy80X937H/ciZGVjXeaRHqbsS8lj8vlPIsJlK eyfA== X-Received: by 10.152.3.130 with SMTP id c2mr9419312lac.81.1434568028422; Wed, 17 Jun 2015 12:07:08 -0700 (PDT) In-Reply-To: <20150617093056.GD22285@port70.net> Xref: news.gmane.org gmane.linux.lib.musl.general:7952 Archived-At: On Wed, Jun 17, 2015 at 4:30 AM, Szabolcs Nagy wrote: > * Josiah Worcester [2015-06-16 21:48:11 -0500]: >> --- /dev/null >> +++ b/src/compat/gnu_chk.c >> @@ -0,0 +1,18 @@ >> +#define _GNU_SOURCE >> +#include >> +#include >> +#include >> +#include "atomic.h" >> +#include "atomic.h" >> + >> +int __ppoll_chk(struct pollfd *fds, nfds_t n, const struct timespec *to, const sigset_t *mask, size_t fdslen) >> +{ >> + if (fdslen / sizeof(*fds) < n) a_crash(); >> + return ppoll(fds, n, to, mask); >> +} >> + >> +void *__mempcpy_chk(void *dest, const void *src, size_t n, size_t destlen) >> +{ >> + if (destlen < n) a_crash(); >> + return mempcpy(dest, src, n); >> +} > > one atomic is enough > Ooops. >> +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(); >> +} > > this looks wrong if !size > > i think that line should be just removed > Hmm, yeah. When the buffer is empty it should crash, not just report error. >> +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); >> +} > > i think restrict is wrong here > It is. >> +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'd store the strnlen result in a temporary var > > musl is built with -ffreestanding so the compiler cannot > assume strnlen is a pure function. Good call. In common use it won't matter much, but that's potentially an extra string iteration for no good reason. >> +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 *__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); >> +} >> + > > ditto > >> 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); >> +} > > missing atomic.h for a_crash Oops. > shouldnt this be in posix_chk.c? This... I swear I deleted this file before making the patch. It's an old development fragment. The one I actually care about is in posix_chk.c. >> +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); >> +} > > i think this can cause a false positive crash > but glibc seems to do the same.. > > (eg some api passes wchar_t* and the exact mb encoded length of > a string so the output s can be safely shorter than max mb length) > I'm not sure if we should match glibc here or do a better check. Thanks for the input. I'm going to wait a bit before posting a new patch, though, in hopes that more comments are forthcoming.