From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 88814 invoked by alias); 20 Nov 2017 20:03:37 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Received: (qmail 88803 invoked by uid 89); 20 Nov 2017 20:03:37 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-6.9 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_2,KAM_ASCII_DIVIDERS,KB_WAM_FROM_NAME_SINGLEWORD,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.2 spammy=tough, fragile, everyones, 7.1 X-HELO: mail-qk0-f182.google.com X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:from:cc:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=8eldRvGztRyvQnJSGUMuehsx2jOMw9tvaauNR15STS4=; b=d9lHRCztlksTQp5UAs67yinM4w9lRjOuUQtse7g7rq72ZsX0Fy7aF63oYbnOhZ5Llz Jg3U52Yz3x419xQLyApywxQPXOCfLx+vOSer2L5x9QI6t9T4J0/Be4QGQ4lb/eQTkmeg uqC69jvJ+deUm0qvYIgniIQHZD9VPKPZs/i5bgiVJzaEciouPbl8+2flCkR+SqQA/9Ip CbsyePYwZcCKt5B7WhVepndfpto7TU+WBkpRlmOQFHzEn5f/lqiErk9SUufKI1WsmSnQ nuUAsCUaoGNSIqApnAC2t3s2o4KmsdYxvIuxx9hAiXFKAsd+PeSPGEHMHbPuQvaoXYrE BStw== X-Gm-Message-State: AJaThX6/RBsrPOn6WWKGPEl9UgPCUsa24l+mVNrXfMQc50ygtPKaVxFZ NJiAh23bO+jOnjYurYextnvCSQ== X-Google-Smtp-Source: AGs4zMZDQx48PLijz7lTcpOP4+5bLuv+Lr4P7wqONZfJxFJctcyymFppYArBNknCrPE08VTMrCx2AA== X-Received: by 10.55.122.135 with SMTP id v129mr2981826qkc.104.1511208213084; Mon, 20 Nov 2017 12:03:33 -0800 (PST) Subject: Re: [RFC][PATCH] Refactoring FORTIFY To: libc-alpha@sourceware.org References: From: Adhemerval Zanella Cc: George Burgess IV Message-ID: <41677d63-4cec-e24b-e4b7-378aa030a0f7@linaro.org> Date: Mon, 20 Nov 2017 20:03:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-SW-Source: 2017-11/txt/msg00695.txt.bz2 On 11/09/2017 03:26, George Burgess IV wrote: > Hello, > > Attached is a patch that aims to substantially improve FORTIFY's > usefulness with clang, and make defining FORTIFY'ed functions require > less ceremony. > > I'm not looking for a thorough review at this time, nor do I hope to > land this patch in one piece. The goal of this thread is to ensure > that everyone's more or less happy with where I'd like to take glibc's > FORTIFY implementation. Thanks for the patch and patience. The idea sound reasonable and it will be indeed a good addition to extend fortify functionality to clang as well. > > Please note that this change is intended to be a functional nop for > all compilers that aren't clang >= 5.0, which was just released last > Thursday. As a potential side note it would be good to add some short explanation of the internal required to adjust it for clang, for instance that __use_clang_fortify is a compiler defined flag which is (?) define only for clang 5.0 or higher. In any case the document referenced is a good addition for explanation, but I usually prefer to have a more self explanatory detail on the thread and or email itself. > > Diving in: as said, this patch removes a lot of duplication from > FORTIFY in the common case, and makes FORTIFY far more useful for > those who use glibc with clang. Namely, with this patch, clang becomes > capable of emitting compile-time diagnostics on par (*) with GCC's, > and clang's ability to perform run-time checks is substantially > improved over what we have today. > > It essentially does this by wrapping up the majority of the > compiler-specific incantations (declaring __foo_chk_warn, > conditionally calling it, ...) behind a macro, and uses that to stamp > out FORTIFY's compile-time diagnostic bits. While this approach is the > cleanest I've been able to come up with, it has potential downsides: > > - Compile-time diagnostics with GCC are somewhat different than what > they are today. To show this, I've attached tst-chk2-output.diff, > which is a diff of the diagnostics produced by running GCC 7.1 on > debug/tst-chk2.c. I don't find the difference to be substantial, but > it does exist. Taking your provided file as an example I do not see it as blocker for this approach. Although the new warning prints the macros used to parametrize the builtins invocation, I think it still shows enough information for debugging. For instance: -../libio/bits/stdio2.h:64:10: warning: ‘__builtin___snprintf_chk’: specified bound 3 exceeds the size 2 of the destination [-Wstringop-overflow=] - return __builtin___snprintf_chk (__s, __n, __USE_FORTIFY_LEVEL - 1, - ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ - __bos (__s), __fmt, __va_arg_pack ()); - ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +../libio/bits/stdio2.h:79:7: warning: ‘__builtin___snprintf_chk’: specified bound 3 exceeds the size 2 of the destination [-Wstringop-overflow=] + int __result = __FORTIFY_CALL_VA_BUILTIN (snprintf, __s, __n, Also, some seems more convoluted as: In function ‘wmemmove’, inlined from ‘do_test’ at tst-chk1.c:681:3: -../wcsmbs/bits/wchar2.h:77:9: warning: call to ‘__wmemmove_chk_warn’ declared with attribute warning: wmemmove called with length bigger than size of destination buffer - return __wmemmove_chk_warn (__s1, __s2, __n, - ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ - __bos0 (__s1) / sizeof (wchar_t)); - ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +../wcsmbs/bits/wchar2.h:56:42: warning: call to ‘__wmemmove_warn’ declared with attribute warning: wmemmove called with length bigger than size of destination buffer + __FORTIFY_WARNING_ONLY_IF_BOS0_LT2 (__wmemmove_warn, __n, __s1, +../misc/sys/cdefs.h:234:5: note: in definition of macro ‘__FORTIFY_WARNING_ONLY_IF_BOS0_LT2’ + err_fn (); \ + ^~~~~~ Which might trigger an warning with different internal names being used and also with a more convoluted warning which points to an internal name instead direct to a builtin. I still think this is non blocker since the warning is still meaningful and correct to point the api usage issue. > - In very rare cases, the code generated by GCC will be a bit worse > (e.g. slower+larger) with this patch. I know this may be a tough sell, > but please hear me out. :) > > With this patch, we sometimes emit diagnostics by emitting code like: > if (should_emit_compile_time_warning) { > volatile char c = 0; > if (__glibc_unlikely (c)) > function_with_warnattr (); > } > > Where `should_emit_compile_time_warning` always folds to a constant > during compilation. So, 0 diagnostics should mean 0 change in code > quality. > > I don't believe this is a deal-breaker, since: > - if you're using FORTIFY, you presumably care enough to fix > FORTIFY-related warnings, which makes this regression nonexistent for > you, > - if you're using FORTIFY, you know there will be a (small, but > present) performance penalty, > - directly after each __glibc_unlikely(c) branch is call to a FORTIFY > function with known broken input, which should abort the program > anyway, and > - this doesn't apply to *all* diagnostics (e.g. bad calls to open() > don't have this penalty); just ones where we can unify clang's and > GCC's diagnostic emission bits. I also agree potential pessimization on code that triggers the fortify checks is not really a blocker. The only issue I would consider for it would be if hot path for fortified code also get slower/larger, but it does not seem to be the case. > > In any case, please see binary-output.diff for an idea of the > difference this makes on code compiled with GCC 7.1. The function > being compiled was a call to wmemmove with an undersized buffer. With > a sufficiently large buffer, both today's FORTIFY implementation and > the proposed one produce identical bodies for the function in > question. > > Other than that, I know of no regressions that this patch causes with GCC. > > For clang, in very rare cases (read: I've seen ~10 instances of this > testing similar implementations across Android, ChromeOS, and another > very large code base), it can also break existing code. For specifics > on that, and an overview on how clang's FORTIFY implementation works, > please see > https://docs.google.com/document/d/1DFfZDICTbL7RqS74wJVIJ-YnjQOj1SaoqfhbgddFYSM/edit?usp=sharing For 'Incompatibilities between clang and GCC FORTIFY' chapter in your documentation, the first one I do not see really an issue because relying on compiler constant fold strlen is quite fragile. The second one seem more a minor issue, but the a compiler details for a very specialized usage. I do not have a strong opinion if they should be used as blockers. > The "How does clang handle it?" section covers the primary attributes > this patch uses, and the "Incompatibilities between clang and GCC > FORTIFY" section covers places where this patch might break existing > clang users. > > Though I expect clang breakages to be very rare and trivial to fix, > this patch introduces a _CLANG_FORTIFY_DISABLE macro, which can be > used to turn this entire patch into a nop for clang >= 5.0. I really want to avoid a compiler specific flag to deactivate fortify where there is already in place better approach (either adjusting the code or disabling fortify). Also it means it would need to to continue export such macro to maintain compatibility, which also adds a lot of complexity with no so straightforward gains. > > ----- > > I apologize if this is a lot to dump into one email. As said, if we > decide this is an acceptable direction to head in, I hope to land this > patch in many easily reviewable parts later on. Indeed to move forward on review we will need to split this patch by family functions, maybe an initial patch to add the required macros and gcc required refactoring and other for family of functions or by headers with the adjustments for clang. The only issue I see is lacking of testing I do not have an easier solution. One option would to try check for clang with support for fortify on path and add extra rules for build the fortify tests with clang as well, but I am not sure how complex would be to adjust current testcase for such scenario. > > If you have any questions, comments, concerns, etc. please don't > hesitate to voice them. > > Thank you very much for your time, :) > George > > (*) - This means that clang can catch almost as many *types* of bugs > as GCC at compile-time. Due to architectural constraints, clang can > only perform these checks prior to optimizing code. > So, clang is still unable to statically diagnose bugs that require it > to do more than simple constant folding. __builtin_object_size, > however, isn't required to be folded until after we optimize code, so > many dynamic checks can still take advantage of optimizations. >