From patchwork Thu May 12 13:15:03 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Siddhesh Poyarekar X-Patchwork-Id: 53872 Return-Path: X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 51A353836409 for ; Thu, 12 May 2022 13:21:17 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 51A353836409 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1652361677; bh=z05Zb125NyO4IwoKsBk5mMaP4890IBuI0QwmrRvVZdM=; h=To:Subject:Date:In-Reply-To:References:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=cUmOb74mZz1uJv1OQofdQyZwh/Voq7DU5KiQeXUDwkbZwX4NyZVJRuw8RcTBOknjE rgiy0+SKkLVfJyWVifQr5p8UX80VlWRlKoSp4V2K9aVrnLIcU1Y0yxZ3NQgim1Ox2s 12ZcKnDkEhB3GUiSpWybxOFyxx/LkcBsvkJHWHVY= X-Original-To: libc-alpha@sourceware.org Delivered-To: libc-alpha@sourceware.org Received: from aye.pine.relay.mailchannels.net (aye.pine.relay.mailchannels.net [23.83.219.6]) by sourceware.org (Postfix) with ESMTPS id E2D5C3836430 for ; Thu, 12 May 2022 13:15:34 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org E2D5C3836430 X-Sender-Id: dreamhost|x-authsender|siddhesh@gotplt.org Received: from relay.mailchannels.net (localhost [127.0.0.1]) by relay.mailchannels.net (Postfix) with ESMTP id 1B13C6C0F47; Thu, 12 May 2022 13:15:31 +0000 (UTC) Received: from pdx1-sub0-mail-a304.dreamhost.com (unknown [127.0.0.6]) (Authenticated sender: dreamhost) by relay.mailchannels.net (Postfix) with ESMTPA id 8667F6C0ECF; Thu, 12 May 2022 13:15:30 +0000 (UTC) ARC-Seal: i=1; s=arc-2022; d=mailchannels.net; t=1652361330; a=rsa-sha256; cv=none; b=v/fyFJZn/YvSTCUgBWlclnbNJJHXt7yXSsPgXccGJiiXEneEO+SIauvTkLJxnTymJj070I sX81t4NcN2zrAX+8LVcF7cZnGlS/ljXu0a5yGx68MLhrKrYTpj0A1+eUK9TPNUL4+Titym B7AeYSB/WbjTaCmw+fYRTXqX5ZdqM1XkSVdc13kSxi2EyrFn9OQdQStyfa1KSbg2zu/r5m GnBFHZGqMrb1J1zYMuu6pEyrXQbfr1+TbMBEbbnkMPzFhFUqUJd2K/CG0EombGwH9SkhKU aNfTrC7CGsachECyY0DKxjpeOBICMNdfZAi7BecAoZdO80+vaLfspHa8GT7o5Q== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=mailchannels.net; s=arc-2022; t=1652361330; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=z05Zb125NyO4IwoKsBk5mMaP4890IBuI0QwmrRvVZdM=; b=Q4emKadCNjxzvgNjHrgzjQ4OfVtEr5NIPDUg2V+kBcHdefxzuI9PXPcIUYdBHhvXURNNM5 jhGNDjzLwJ9MoYikaZRns6DMlEwSU4iJxBsObXzBqdKqbr7n3rEL/EAgxkPM+l97S1SZ+Q EZjblfU4dTKLRhEFD5v7D3JZ9bY0W/Nsyys54MSfuwIwJsGflAsqQdQ9sZijx53dU7yMS+ MRQp263yu2z4svJPJHd24MqLYJLPR/m0rw3+mYXJsYhX87bhorIrtPEv3CPnzqmdhVkxC8 6KBtjyFsLSJu2Y6WbTf6/aCzRj50u0dA1jTfmzGEfuRlqnyv744wRjoWNCEvoQ== ARC-Authentication-Results: i=1; rspamd-6fcfc4d76-fdzmm; auth=pass smtp.auth=dreamhost smtp.mailfrom=siddhesh@sourceware.org X-Sender-Id: dreamhost|x-authsender|siddhesh@gotplt.org X-MC-Relay: Junk X-MailChannels-SenderId: dreamhost|x-authsender|siddhesh@gotplt.org X-MailChannels-Auth-Id: dreamhost X-Glossy-Abaft: 050e601272699ec1_1652361330912_1530773699 X-MC-Loop-Signature: 1652361330912:1195929084 X-MC-Ingress-Time: 1652361330912 Received: from pdx1-sub0-mail-a304.dreamhost.com (pop.dreamhost.com [64.90.62.162]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384) by 100.105.211.134 (trex/6.7.1); Thu, 12 May 2022 13:15:30 +0000 Received: from rhbox.redhat.com (unknown [103.199.173.7]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) (Authenticated sender: siddhesh@gotplt.org) by pdx1-sub0-mail-a304.dreamhost.com (Postfix) with ESMTPSA id 4KzXML6yrHz28; Thu, 12 May 2022 06:15:26 -0700 (PDT) To: libc-alpha@sourceware.org Subject: [PATCH v3] wcrtomb: Make behavior POSIX compliant Date: Thu, 12 May 2022 18:45:03 +0530 Message-Id: <20220512131503.764504-1-siddhesh@sourceware.org> X-Mailer: git-send-email 2.35.1 In-Reply-To: <20220505184348.3357550-3-siddhesh@sourceware.org> References: <20220505184348.3357550-3-siddhesh@sourceware.org> MIME-Version: 1.0 X-Spam-Status: No, score=-1168.7 required=5.0 tests=BAYES_00, GIT_PATCH_0, JMQ_SPF_NEUTRAL, KAM_DMARC_NONE, KAM_DMARC_STATUS, RCVD_IN_BARRACUDACENTRAL, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_NEUTRAL, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-Patchwork-Original-From: Siddhesh Poyarekar via Libc-alpha From: Siddhesh Poyarekar Reply-To: Siddhesh Poyarekar Cc: fweimer@redhat.com, dickey@his.com Errors-To: libc-alpha-bounces+patchwork=sourceware.org@sourceware.org Sender: "Libc-alpha" The GNU implementation of wcrtomb assumes that there are at least MB_CUR_MAX bytes available in the destination buffer passed to wcrtomb as the first argument. This is not compatible with the POSIX definition, which only requires enough space for the input wide character. This does not break much in practice because when users supply buffers smaller than MB_CUR_MAX (e.g. in ncurses), they compute and dynamically allocate the buffer, which results in enough spare space (thanks to usable_size in malloc and padding in alloca) that no actual buffer overflow occurs. However when the code is built with _FORTIFY_SOURCE, it runs into the hard check against MB_CUR_MAX in __wcrtomb_chk and hence fails. It wasn't evident until now since dynamic allocations would result in wcrtomb not being fortified but since _FORTIFY_SOURCE=3, that limitation is gone, resulting in such code failing. To fix this problem, introduce an internal buffer that is MB_LEN_MAX long and use that to perform the conversion and then copy the resultant bytes into the destination buffer. Also move the fortification check into the main implementation, which checks the result after conversion and aborts if the resultant byte count is greater than the destination buffer size. One complication is that applications that assume the MB_CUR_MAX limitation to be gone may not be able to run safely on older glibcs if they use static destination buffers smaller than MB_CUR_MAX; dynamic allocations will always have enough spare space that no actual overruns will occur. One alternative to fixing this is to bump symbol version to prevent them from running on older glibcs but that seems too strict a constraint. Instead, since these users will only have made this decision on reading the manual, I have put a note in the manual warning them about the pitfalls of having static buffers smaller than MB_CUR_MAX and running them on older glibc. Benchmarking: The wcrtomb microbenchmark shows significant increases in maximum execution time for all locales, ranging from 10x for ar_SA.UTF-8 to 1.5x-2x for nearly everything else. The mean execution time however saw practically no impact, with some results even being quicker, indicating that cache locality has a much bigger role in the overhead. Given that the additional copy uses a temporary buffer inside wcrtomb, it's likely that a hot path will end up putting that buffer (which is responsible for the additional overhead) in a similar place on stack, giving the necessary cache locality to negate the overhead. However in situations where wcrtomb ends up getting called at wildly different spots on the call stack (or is on different call stacks, e.g. with threads or different execution contexts) and is still a hotspot, the performance lag will be visible. Signed-off-by: Siddhesh Poyarekar --- Changes from v2: - Updated manual blurb to refer to "this version" so that it is backport-clean. - Changed copy logic to inline copies of up to 4 bytes. It is 1-7% faster that the previous inlining of only 1 and 2 bytes and a good 10%+ faster than using plain memcpy. 4 bytes is the maximum length for byte encodings in charmaps shipped in glibc. Changes from v1: - Fixed nits suggested by Paul Eggert. debug/tst-fortify.c | 7 ++++++- debug/wcrtomb_chk.c | 8 ++----- include/wchar.h | 4 ++++ manual/charset.texi | 11 +++++----- wcsmbs/wcrtomb.c | 51 ++++++++++++++++++++++++++++++++++++++------- 5 files changed, 61 insertions(+), 20 deletions(-) diff --git a/debug/tst-fortify.c b/debug/tst-fortify.c index 03c9867714..8e94643bf2 100644 --- a/debug/tst-fortify.c +++ b/debug/tst-fortify.c @@ -1478,10 +1478,15 @@ do_test (void) character which has a multibyte representation which does not fit. */ CHK_FAIL_START - char smallbuf[2]; + char smallbuf[1]; if (wcrtomb (smallbuf, L'\x100', &s) != 2) FAIL (); CHK_FAIL_END + + /* Same input with a large enough buffer and we're good. */ + char bigenoughbuf[2]; + if (wcrtomb (bigenoughbuf, L'\x100', &s) != 2) + FAIL (); #endif wchar_t wenough[10]; diff --git a/debug/wcrtomb_chk.c b/debug/wcrtomb_chk.c index 8b6d026560..28c3ea0d2d 100644 --- a/debug/wcrtomb_chk.c +++ b/debug/wcrtomb_chk.c @@ -1,4 +1,5 @@ /* Copyright (C) 2005-2022 Free Software Foundation, Inc. + Copyright The GNU Toolchain Authors. This file is part of the GNU C Library. The GNU C Library is free software; you can redistribute it and/or @@ -25,10 +26,5 @@ size_t __wcrtomb_chk (char *s, wchar_t wchar, mbstate_t *ps, size_t buflen) { - /* We do not have to implement the full wctomb semantics since we - know that S cannot be NULL when we come here. */ - if (buflen < MB_CUR_MAX) - __chk_fail (); - - return __wcrtomb (s, wchar, ps); + return __wcrtomb_internal (s, wchar, ps, buflen); } diff --git a/include/wchar.h b/include/wchar.h index 4267985625..db83297bca 100644 --- a/include/wchar.h +++ b/include/wchar.h @@ -172,6 +172,10 @@ libc_hidden_proto (__mbrtowc) libc_hidden_proto (__mbrlen) extern size_t __wcrtomb (char *__restrict __s, wchar_t __wc, __mbstate_t *__restrict __ps) attribute_hidden; +extern size_t __wcrtomb_internal (char *__restrict __s, wchar_t __wc, + __mbstate_t *__restrict __ps, + size_t __s_size) + attribute_hidden; extern size_t __mbsrtowcs (wchar_t *__restrict __dst, const char **__restrict __src, size_t __len, __mbstate_t *__restrict __ps) diff --git a/manual/charset.texi b/manual/charset.texi index a9b5cb4a37..427db3bc80 100644 --- a/manual/charset.texi +++ b/manual/charset.texi @@ -883,11 +883,12 @@ the string @var{s}. This includes all bytes representing shift sequences. One word about the interface of the function: there is no parameter -specifying the length of the array @var{s}. Instead the function -assumes that there are at least @code{MB_CUR_MAX} bytes available since -this is the maximum length of any byte sequence representing a single -character. So the caller has to make sure that there is enough space -available, otherwise buffer overruns can occur. +specifying the length of the array @var{s}, so the caller has to make sure +that there is enough space available, otherwise buffer overruns can occur. +This version of @theglibc{} does not assume that @var{s} is at least +@var{MB_CUR_MAX} bytes long, but programs that need to run on @glibcadj{} +versions that have this assumption documented in the manual must comply +with this limit. @pindex wchar.h @code{wcrtomb} was introduced in @w{Amendment 1} to @w{ISO C90} and is diff --git a/wcsmbs/wcrtomb.c b/wcsmbs/wcrtomb.c index e17438989f..d68dcdac82 100644 --- a/wcsmbs/wcrtomb.c +++ b/wcsmbs/wcrtomb.c @@ -1,4 +1,5 @@ /* Copyright (C) 1996-2022 Free Software Foundation, Inc. + Copyright The GNU Toolchain Authors. This file is part of the GNU C Library. The GNU C Library is free software; you can redistribute it and/or @@ -20,6 +21,7 @@ #include #include #include +#include #include #include @@ -34,7 +36,7 @@ static mbstate_t state; size_t -__wcrtomb (char *s, wchar_t wc, mbstate_t *ps) +__wcrtomb_internal (char *s, wchar_t wc, mbstate_t *ps, size_t s_size) { char buf[MB_LEN_MAX]; struct __gconv_step_data data; @@ -52,14 +54,11 @@ __wcrtomb (char *s, wchar_t wc, mbstate_t *ps) /* A first special case is if S is NULL. This means put PS in the initial state. */ if (s == NULL) - { - s = buf; - wc = L'\0'; - } + wc = L'\0'; /* Tell where we want to have the result. */ - data.__outbuf = (unsigned char *) s; - data.__outbufend = (unsigned char *) s + MB_CUR_MAX; + data.__outbuf = (unsigned char *) buf; + data.__outbufend = (unsigned char *) buf + sizeof buf; /* Get the conversion functions. */ fcts = get_gconv_fcts (_NL_CURRENT_DATA (LC_CTYPE)); @@ -101,7 +100,37 @@ __wcrtomb (char *s, wchar_t wc, mbstate_t *ps) if (status == __GCONV_OK || status == __GCONV_EMPTY_INPUT || status == __GCONV_FULL_OUTPUT) - result = data.__outbuf - (unsigned char *) s; + { + result = data.__outbuf - (unsigned char *) buf; + + if (s != NULL) + { + if (result > s_size) + __chk_fail (); + + /* The maximum byte size in all charmaps is 4, so inline copies up to + 4 bytes. One must benchmark before bumping this in future because + the returns for inlining will diminish as the number of branches + increases. */ + switch (result) + { + case 4: + s[3] = buf[3]; + /* Fall through. */ + case 3: + s[2] = buf[2]; + /* Fall through. */ + case 2: + s[1] = buf[1]; + /* Fall through. */ + case 1: + s[0] = buf[0]; + break; + default: + memcpy (s, buf, result); + } + } + } else { result = (size_t) -1; @@ -110,5 +139,11 @@ __wcrtomb (char *s, wchar_t wc, mbstate_t *ps) return result; } + +size_t +__wcrtomb (char *s, wchar_t wc, mbstate_t *ps) +{ + return __wcrtomb_internal (s, wc, ps, (size_t) -1); +} weak_alias (__wcrtomb, wcrtomb) libc_hidden_weak (wcrtomb)