From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-vk1-xa35.google.com (mail-vk1-xa35.google.com [IPv6:2607:f8b0:4864:20::a35]) by sourceware.org (Postfix) with ESMTPS id D8C1C3858425 for ; Mon, 1 Aug 2022 19:59:57 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org D8C1C3858425 Received: by mail-vk1-xa35.google.com with SMTP id r4so2167274vkf.0 for ; Mon, 01 Aug 2022 12:59:57 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:date:mime-version:user-agent:subject :content-language:to:references:from:organization:in-reply-to :content-transfer-encoding; bh=dgWZID5isatGauD7r8VHNc4ceO8KFSaD3QAW9vsG8QI=; b=3kf48mmPkDMfSH2fEHas254zXdtMmVgFhB3F2ob86qHGGH4s8YTKG5ErZtMtcb24Gk lPQPc7iOvRa5v1KwA0RukPij49rVPEiWnFE97lkFziSkKlMNiohxzpfOR8qPauP0JxDL Ay+I8neIsCLHX7XTb83iOoSFfPKN2sev+TUlm+EDvwVF1DFmNeMnEDL5UGauhjxAOmWc M5eouZ8GghXlgvgFBpY59SyptkbrbTttDvRDnm4qapJpvCOpFkLRjNstmTNKM4sfYpDe MTbkuxMyAcaoVAM+jGBiuxl6auL06ciagOwG2i3fGBLKGZrNyFovUzmOPVzKjJta0IWs TqxA== X-Gm-Message-State: AJIora9klnW1XO1uKqy8EL023UM73xTwAi4z1tPzO6PJolxaAJlcWaJD eVUsAUMyPzM/dyz+FIoA8Z8YNhH6nmxV4A== X-Google-Smtp-Source: AGRyM1sWa2Dd0+i5t1iAb5bv5FIcb4HBpunXBARZk5tRDogEei37l3JYHi3RNOdHBjNoNi/88iNK6g== X-Received: by 2002:a1f:9f92:0:b0:376:3525:998 with SMTP id i140-20020a1f9f92000000b0037635250998mr6380856vke.41.1659383996966; Mon, 01 Aug 2022 12:59:56 -0700 (PDT) Received: from ?IPV6:2804:431:c7cb:1e34:a813:f55c:8e00:64cd? ([2804:431:c7cb:1e34:a813:f55c:8e00:64cd]) by smtp.gmail.com with ESMTPSA id l203-20020a1f25d4000000b00351bcf78967sm7054579vkl.9.2022.08.01.12.59.55 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 01 Aug 2022 12:59:56 -0700 (PDT) Message-ID: <02dcabf5-00fd-41ac-3caf-bafb5985e382@linaro.org> Date: Mon, 1 Aug 2022 16:59:54 -0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0) Gecko/20100101 Thunderbird/102.1.0 Subject: Re: [PATCH 1/3] stdio: Clean up __libc_message after unconditional abort Content-Language: en-US To: libc-alpha@sourceware.org, Florian Weimer References: <09db94615fe22e053d978e93e404c8a6ff64756d.1659350650.git.fweimer@redhat.com> From: Adhemerval Zanella Netto Organization: Linaro In-Reply-To: <09db94615fe22e053d978e93e404c8a6ff64756d.1659350650.git.fweimer@redhat.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-12.9 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP 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-List-Received-Date: Mon, 01 Aug 2022 19:59:59 -0000 On 01/08/22 07:45, Florian Weimer via Libc-alpha wrote: > Since commit ec2c1fcefb200c6cb7e09553f3c6af8815013d83 ("malloc: > Abort on heap corruption, without a backtrace [BZ #21754]"), > __libc_message always terminates the process. Since commit > a289ea09ea843ced6e5277c2f2e63c357bc7f9a3 ("Do not print backtraces > on fatal glibc errors"), the backtrace facility has been removed. > Therefore, remove enum __libc_message_action and the action > argument of __libc_message, and mark __libc_message as _No_return. LGTM, just a comment below. Reviewed-by: Adhemerval Zanella > --- > debug/fortify_fail.c | 4 +--- > include/stdio.h | 9 +------- > malloc/malloc.c | 5 ++-- > sysdeps/posix/libc_fatal.c | 47 +++++++++++++++++--------------------- > 4 files changed, 25 insertions(+), 40 deletions(-) > > diff --git a/debug/fortify_fail.c b/debug/fortify_fail.c > index b1c51662b7..b880f82cb6 100644 > --- a/debug/fortify_fail.c > +++ b/debug/fortify_fail.c > @@ -21,8 +21,6 @@ void > __attribute__ ((noreturn)) > __fortify_fail (const char *msg) > { > - /* The loop is added only to keep gcc happy. */ > - while (1) > - __libc_message (do_abort, "*** %s ***: terminated\n", msg); > + __libc_message ("*** %s ***: terminated\n", msg); > } > libc_hidden_def (__fortify_fail) > diff --git a/include/stdio.h b/include/stdio.h > index a6f7fd43cb..c3e772ad9a 100644 > --- a/include/stdio.h > +++ b/include/stdio.h > @@ -143,18 +143,11 @@ extern int __gen_tempname (char *__tmpl, int __suffixlen, int __flags, > # define __GT_DIR 1 /* create a directory */ > # define __GT_NOCREATE 2 /* just find a name not currently in use */ > > -enum __libc_message_action > -{ > - do_message = 0, /* Print message. */ > - do_abort = 1 << 0, /* Abort. */ > -}; > - > /* Print out MESSAGE (which should end with a newline) on the error output > and abort. */ > extern void __libc_fatal (const char *__message) > __attribute__ ((__noreturn__)); > -extern void __libc_message (enum __libc_message_action action, > - const char *__fnt, ...) attribute_hidden; > +_Noreturn void __libc_message (const char *__fnt, ...) attribute_hidden; > extern void __fortify_fail (const char *msg) __attribute__ ((__noreturn__)); > libc_hidden_proto (__fortify_fail) > > diff --git a/malloc/malloc.c b/malloc/malloc.c > index bd3c76ed31..f3320d2663 100644 > --- a/malloc/malloc.c > +++ b/malloc/malloc.c > @@ -296,8 +296,7 @@ _Noreturn static void > __malloc_assert (const char *assertion, const char *file, unsigned int line, > const char *function) > { > - __libc_message (do_abort, "\ > -Fatal glibc error: malloc assertion failure in %s: %s\n", > + __libc_message ("Fatal glibc error: malloc assertion failure in %s: %s\n", > function, assertion); > __builtin_unreachable (); > } > @@ -5657,7 +5656,7 @@ static void > malloc_printerr (const char *str) > { > #if IS_IN (libc) > - __libc_message (do_abort, "%s\n", str); > + __libc_message ("%s\n", str); > #else > __libc_fatal (str); > #endif > diff --git a/sysdeps/posix/libc_fatal.c b/sysdeps/posix/libc_fatal.c > index 2ee0010b8d..270238495d 100644 > --- a/sysdeps/posix/libc_fatal.c > +++ b/sysdeps/posix/libc_fatal.c > @@ -54,7 +54,7 @@ struct str_list > > /* Abort with an error message. */ > void > -__libc_message (enum __libc_message_action action, const char *fmt, ...) > +__libc_message (const char *fmt, ...) > { > va_list ap; > int fd = -1; > @@ -123,36 +123,31 @@ __libc_message (enum __libc_message_action action, const char *fmt, ...) > > WRITEV_FOR_FATAL (fd, iov, nlist, total); > > - if ((action & do_abort)) > + total = (total + 1 + GLRO(dl_pagesize) - 1) & ~(GLRO(dl_pagesize) - 1); > + struct abort_msg_s *buf = __mmap (NULL, total, > + PROT_READ | PROT_WRITE, > + MAP_ANON | MAP_PRIVATE, -1, 0); > + if (__glibc_likely (buf != MAP_FAILED)) > { > - total = ((total + 1 + GLRO(dl_pagesize) - 1) > - & ~(GLRO(dl_pagesize) - 1)); > - struct abort_msg_s *buf = __mmap (NULL, total, > - PROT_READ | PROT_WRITE, > - MAP_ANON | MAP_PRIVATE, -1, 0); > - if (__glibc_likely (buf != MAP_FAILED)) > - { > - buf->size = total; > - char *wp = buf->msg; > - for (int cnt = 0; cnt < nlist; ++cnt) > - wp = mempcpy (wp, iov[cnt].iov_base, iov[cnt].iov_len); > - *wp = '\0'; > - > - /* We have to free the old buffer since the application might > - catch the SIGABRT signal. */ > - struct abort_msg_s *old = atomic_exchange_acq (&__abort_msg, > - buf); > - if (old != NULL) > - __munmap (old, old->size); > - } > + buf->size = total; > + char *wp = buf->msg; > + for (int cnt = 0; cnt < nlist; ++cnt) > + wp = mempcpy (wp, iov[cnt].iov_base, iov[cnt].iov_len); > + *wp = '\0'; > + > + /* We have to free the old buffer since the application might > + catch the SIGABRT signal. */ > + struct abort_msg_s *old = atomic_exchange_acq (&__abort_msg, > + buf); > + if (old != NULL) > + __munmap (old, old->size); I am aware that this is just replicating the code already in place, but maybe replace the old atomic with atomic_exchange_acquire here. > } > } > > va_end (ap); > > - if ((action & do_abort)) > - /* Kill the application. */ > - abort (); > + /* Kill the application. */ > + abort (); > } > > > @@ -161,6 +156,6 @@ __libc_fatal (const char *message) > { > /* The loop is added only to keep gcc happy. */ > while (1) > - __libc_message (do_abort, "%s", message); > + __libc_message ("%s", message); > } > libc_hidden_def (__libc_fatal)