From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-oi1-x234.google.com (mail-oi1-x234.google.com [IPv6:2607:f8b0:4864:20::234]) by sourceware.org (Postfix) with ESMTPS id 513153857823 for ; Wed, 27 Apr 2022 18:35:22 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 513153857823 Received: by mail-oi1-x234.google.com with SMTP id s131so2946058oie.1 for ; Wed, 27 Apr 2022 11:35:22 -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:cc:references:from:in-reply-to :content-transfer-encoding; bh=wkKAGBDhGa4icqnPYIA7SjXZSv5qvn3oVdzthBBVpUI=; b=Xg9+9PH6VDDR4vOxpdcJavwXWyqHqDWHzVAzeYF9YoE3Kinq5NjzhtWCG0X32U86We 4EGhkXDNKrN4jxqGXpMipcrMglbMBKqSf+iyTPpu8bhsKhg7T3ByXgcoNnVnvENg2TFt vHNhJ0CstbtFGRqlL/8/6expFf4OX0p93HtzJrMTZP7paVm/BoVL0pRRqTnW14Y3pM1J kyW0/hO7n9eZef7YNLooNUQFSeoVbCmcFgLMXotT5aW/sHM+nw6QlB2ECICxvmopq1C0 75fNpa57jFe8QrZk/jCD9MUg/bEnoH9xYia+hWpo5mDPrPW+flgx92CN/bfPl/cjGZC4 7G/g== X-Gm-Message-State: AOAM530XRHeAsanju/G9ckjR1glEnI+B5XYjVXBozPaTwTWY8gfFnfyG hFOwyo1nWTWsxP645Lo2qvngr69ad6BMSQ== X-Google-Smtp-Source: ABdhPJwfxLXUY7eUGVIkVhNIl3mwzxV5TGR64dUVRna2mC2Y6V6KaSTREfY99OLZO90k9rVINwzFQA== X-Received: by 2002:a05:6808:574:b0:325:6310:617e with SMTP id j20-20020a056808057400b003256310617emr4114802oig.46.1651084521538; Wed, 27 Apr 2022 11:35:21 -0700 (PDT) Received: from ?IPV6:2804:431:c7ca:4214:b613:182d:cdde:5f86? ([2804:431:c7ca:4214:b613:182d:cdde:5f86]) by smtp.gmail.com with ESMTPSA id p14-20020a056830304e00b005b246b673f2sm6184962otr.71.2022.04.27.11.35.19 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 27 Apr 2022 11:35:20 -0700 (PDT) Message-ID: Date: Wed, 27 Apr 2022 15:35:18 -0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.8.1 Subject: Re: [PATCH v2 2/4] Consolidate stdio-lock.h Content-Language: en-US To: Florian Weimer Cc: Adhemerval Zanella via Libc-alpha References: <20220426191523.833171-1-adhemerval.zanella@linaro.org> <20220426191523.833171-3-adhemerval.zanella@linaro.org> <87o80mhd4u.fsf@oldenburg.str.redhat.com> <874k2ee79d.fsf@oldenburg.str.redhat.com> From: Adhemerval Zanella In-Reply-To: <874k2ee79d.fsf@oldenburg.str.redhat.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-14.0 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.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) 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: Wed, 27 Apr 2022 18:35:24 -0000 On 27/04/2022 15:00, Florian Weimer wrote: > * Adhemerval Zanella: > >> On 27/04/2022 10:25, Florian Weimer wrote: >>> * Adhemerval Zanella via Libc-alpha: >>> >>>> diff --git a/sysdeps/generic/stdio-lock.h b/sysdeps/generic/stdio-lock.h >>>> index 14cf458bdd..fd61f0b5b7 100644 >>>> --- a/sysdeps/generic/stdio-lock.h >>>> +++ b/sysdeps/generic/stdio-lock.h >>>> @@ -45,20 +45,13 @@ __libc_lock_define_recursive (typedef, _IO_lock_t) >>>> #define _IO_cleanup_region_end(_doit) \ >>>> __libc_cleanup_region_end (_doit) >>>> >>>> -#if defined _LIBC && IS_IN (libc) >>>> - >>>> -# ifdef __EXCEPTIONS >>>> -# define _IO_acquire_lock(_fp) \ >>>> +#define _IO_acquire_lock(_fp) \ >>>> do { \ >>>> - FILE *_IO_acquire_lock_file \ >>>> - __attribute__((cleanup (_IO_acquire_lock_fct))) \ >>>> - = (_fp); \ >>>> - _IO_flockfile (_IO_acquire_lock_file); >>>> -# else >>>> -# define _IO_acquire_lock(_fp) _IO_acquire_lock_needs_exceptions_enabled >>>> -# endif >>>> -# define _IO_release_lock(_fp) ; } while (0) >>>> - >>>> -#endif >>>> + _IO_cleanup_region_start((void (*) (void *)) &_IO_funlockfile, _fp); \ >>>> + _IO_flockfile (_fp); >>>> +#define _IO_release_lock(_fp) \ >>>> + _IO_funlockfile (_fp); \ >>>> + _IO_cleanup_region_end (0); \ >>>> + } while (0) >>>> >>>> #endif /* stdio-lock.h */ >>> >>> I think this change replaces unwind tables for -fexceptions builds. If >>> GCC can't turn the indirect call to the unlock function into a direct >>> call, this will result in a loss of hardening due to the additional >>> indirect function call. >>> >>> This change may also lose C++ unwinding compatibility for some >>> fopencookie use cases, I think. >> >> This is an internal header where if __EXCEPTIONS is not defined we will >> get a compiler error because of an undefined symbol >> (_IO_acquire_lock_needs_exceptions_enabled). So internally all >> _IO_acquire_lock usage already requires __EXCEPTIONS, so the fallback >> is just unused definitions. > > I see this code generation change in libio/fputc.os. The new code uses > an on-stack pointer saved at the start of the cleanup region: > > + 90: 48 8b 05 00 00 00 00 mov 0x0(%rip),%rax # 97 > + 93: R_X86_64_REX_GOTPCRELX _IO_funlockfile-0x4 > + 97: 48 89 e7 mov %rsp,%rdi > + 9a: 48 89 04 24 mov %rax,(%rsp) > + 9e: e8 00 00 00 00 call a3 > + 9f: R_X86_64_PLT32 __GI___libc_cleanup_push_defer-0x4 > + a3: 8b 45 00 mov 0x0(%rbp),%eax > + a6: 25 00 80 00 00 and $0x8000,%eax > + ab: 0f 85 d1 00 00 00 jne 182 > + b1: 64 4c 8b 2c 25 10 00 mov %fs:0x10,%r13 > + b8: 00 00 > + ba: 48 8b bd 88 00 00 00 mov 0x88(%rbp),%rdi > + c1: 4c 39 6f 08 cmp %r13,0x8(%rdi) > + c5: 74 1a je e1 > + c7: ba 01 00 00 00 mov $0x1,%edx > + cc: f0 0f b1 17 lock cmpxchg %edx,(%rdi) > + d0: 0f 85 a2 00 00 00 jne 178 > + d6: 48 8b bd 88 00 00 00 mov 0x88(%rbp),%rdi > + dd: 4c 89 6f 08 mov %r13,0x8(%rdi) > + e1: 83 47 04 01 addl $0x1,0x4(%rdi) > + e5: 41 bd 01 00 00 00 mov $0x1,%r13d > + eb: e9 3d ff ff ff jmp 2d > + f0: 48 89 e7 mov %rsp,%rdi > + f3: e8 00 00 00 00 call f8 > + f4: R_X86_64_PLT32 __GI___libc_cleanup_pop_restore-0x4 > > This is a from a build with CFLAGS="-O2 -fexceptions -s -DNDEBUG" > (for comparison purposes). > > The old code just inlined the _IO_funlockfile fast path. > > (We seem to lack libc_hidden_proto/libc_hidden_def for _IO_funlockfile.) You are right. The change now uses __libc_cleanup_region_start from libc-lock.h instead of the cleanup version. I am not sure about hardening, but afaiu __libc_cleanup_push_defer should handle C++ unwinding for fopencookie. It seems that the Hurd version is indeed to the best option, so I think for generic it would be better to just use: diff --git a/sysdeps/generic/stdio-lock.h b/sysdeps/generic/stdio-lock.h index 14cf458bdd..a42131f5a5 100644 --- a/sysdeps/generic/stdio-lock.h +++ b/sysdeps/generic/stdio-lock.h @@ -45,20 +45,12 @@ __libc_lock_define_recursive (typedef, _IO_lock_t) #define _IO_cleanup_region_end(_doit) \ __libc_cleanup_region_end (_doit) -#if defined _LIBC && IS_IN (libc) - -# ifdef __EXCEPTIONS -# define _IO_acquire_lock(_fp) \ +#define _IO_acquire_lock(_fp) \ do { \ FILE *_IO_acquire_lock_file \ __attribute__((cleanup (_IO_acquire_lock_fct))) \ = (_fp); \ _IO_flockfile (_IO_acquire_lock_file); -# else -# define _IO_acquire_lock(_fp) _IO_acquire_lock_needs_exceptions_enabled -# endif -# define _IO_release_lock(_fp) ; } while (0) - -#endif +#define _IO_release_lock(_fp) ; } while (0) #endif /* stdio-lock.h */