From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from userp2130.oracle.com (userp2130.oracle.com [156.151.31.86]) by sourceware.org (Postfix) with ESMTPS id AA7CE385803C for ; Thu, 3 Dec 2020 16:09:37 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org AA7CE385803C Received: from pps.filterd (userp2130.oracle.com [127.0.0.1]) by userp2130.oracle.com (8.16.0.42/8.16.0.42) with SMTP id 0B3FxbGQ111891; Thu, 3 Dec 2020 16:09:32 GMT Received: from userp3030.oracle.com (userp3030.oracle.com [156.151.31.80]) by userp2130.oracle.com with ESMTP id 353dyqxu1b-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Thu, 03 Dec 2020 16:09:32 +0000 Received: from pps.filterd (userp3030.oracle.com [127.0.0.1]) by userp3030.oracle.com (8.16.0.42/8.16.0.42) with SMTP id 0B3G1QV5132929; Thu, 3 Dec 2020 16:07:32 GMT Received: from userv0121.oracle.com (userv0121.oracle.com [156.151.31.72]) by userp3030.oracle.com with ESMTP id 3540g1yart-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Thu, 03 Dec 2020 16:07:31 +0000 Received: from abhmp0012.oracle.com (abhmp0012.oracle.com [141.146.116.18]) by userv0121.oracle.com (8.14.4/8.13.8) with ESMTP id 0B3G7TwV009812; Thu, 3 Dec 2020 16:07:29 GMT Received: from dhcp-10-154-172-239.vpn.oracle.com (/10.154.172.239) by default (Oracle Beehive Gateway v4.0) with ESMTP ; Thu, 03 Dec 2020 08:07:28 -0800 From: Qing Zhao Message-Id: <8A595141-563A-4CDC-AA09-BAB76E5113AA@ORACLE.COM> Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.120.23.2.4\)) Subject: Re: How to traverse all the local variables that declared in the current routine? Date: Thu, 3 Dec 2020 10:07:28 -0600 In-Reply-To: Cc: Richard Sandiford , gcc Patches , kees Cook To: Richard Biener References: <217BE64F-A623-4453-B45B-D38B66B71B72@ORACLE.COM> <15EA64C7-D75F-4CE1-92C8-6940186A512A@ORACLE.COM> <7618DD31-87EC-4003-B1DD-E318E5369A71@ORACLE.COM> <3D01F6C7-2E67-4081-BD5F-4EDEC0227627@ORACLE.COM> X-Mailer: Apple Mail (2.3608.120.23.2.4) X-Proofpoint-Virus-Version: vendor=nai engine=6000 definitions=9824 signatures=668682 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 malwarescore=0 suspectscore=0 phishscore=0 mlxlogscore=999 adultscore=0 mlxscore=0 bulkscore=0 spamscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2009150000 definitions=main-2012030096 X-Proofpoint-Virus-Version: vendor=nai engine=6000 definitions=9824 signatures=668682 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 malwarescore=0 bulkscore=0 clxscore=1015 mlxscore=0 spamscore=0 priorityscore=1501 mlxlogscore=999 suspectscore=0 lowpriorityscore=0 phishscore=0 adultscore=0 impostorscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2009150000 definitions=main-2012030096 X-Spam-Status: No, score=-4.8 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, HTML_MESSAGE, KAM_SHORT, RCVD_IN_MSPIKE_H2, SPF_HELO_PASS, SPF_PASS, TXREP, UNPARSEABLE_RELAY autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Content-Filtered-By: Mailman/MimeDel 2.1.29 X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 03 Dec 2020 16:09:41 -0000 > On Dec 3, 2020, at 2:45 AM, Richard Biener = wrote: >=20 > On Wed, Dec 2, 2020 at 4:36 PM Qing Zhao > wrote: >>=20 >>=20 >>=20 >> On Dec 2, 2020, at 2:45 AM, Richard Biener = wrote: >>=20 >> On Tue, Dec 1, 2020 at 8:49 PM Qing Zhao = wrote: >>=20 >>=20 >> Hi, Richard, >>=20 >> Could you please comment on the following approach: >>=20 >> Instead of adding the zero-initializer quite late at the pass = =E2=80=9Cpass_expand=E2=80=9D, we can add it as early as during = gimplification. >> However, we will mark these new added zero-initializers as = =E2=80=9Cartificial=E2=80=9D. And passing this =E2=80=9Cartificial=E2=80=9D= information to >> =E2=80=9Cpass_early_warn_uninitialized=E2=80=9D and = =E2=80=9Cpass_late_warn_uninitialized=E2=80=9D, in these two = uninitialized variable analysis passes, >> (i.e., in tree-sea-uninit.c) We will update the checking on = =E2=80=9Cssa_undefined_value_p=E2=80=9D to consider =E2=80=9Cartificial=E2= =80=9D zero-initializers. >> (i.e, if the def_stmt is marked with =E2=80=9Cartificial=E2=80=9D, = then it=E2=80=99s a undefined value). >>=20 >> With such approach, we should be able to address all those conflicts. >>=20 >> Do you see any obvious issue with this approach? >>=20 >>=20 >> Yes, DSE will happily elide an explicit zero-init following the >> artificial one leading to false uninit diagnostics. >>=20 >>=20 >> Indeed. This is a big issue. And other optimizations might also be = impacted by the new zero-init, resulting changed behavior >> of uninitialized analysis in the later stage. >=20 > I don't see how the issue can be resolved, you can't get both, uninit > warnings and no uninitialized memory. > People can compile twice, once without -fzero-init to get uninit > warnings and once with -fzero-init to get > the extra "security". So, for GCC, you think that it=E2=80=99s okay to get rid of the = following requirement: C. The implementation needs to keep the current static warning on = uninitialized variables untouched in order to avoid "forking the language=E2=80=9D. Then, we can add explanation in the user documentation of the new = -fzero-init and also=20 that of the -Wuninitialized to inform users that -fzero-init will change = the behavior of -Wuninitialized. In order to get the warnings, -fzero-init should not be added at the = same time? With this requirement being eliminated, implementation will be much = easier.=20 We can add the new initialization during simplification phase. Then this = new option will work for all languages. Is this reasonable? thanks. Qing >=20 > Richard. >=20 >>=20 >> What's the intended purpose of the zero-init? >>=20 >>=20 >>=20 >> The purpose of this new option is: (from the original LLVM patch = submission): >>=20 >> "Add an option to initialize automatic variables with either a = pattern or with >> zeroes. The default is still that automatic variables are = uninitialized. Also >> add attributes to request uninitialized on a per-variable basis, = mainly to disable >> initialization of large stack arrays when deemed too expensive. >>=20 >> This isn't meant to change the semantics of C and C++. Rather, it's = meant to be >> a last-resort when programmers inadvertently have some undefined = behavior in >> their code. This patch aims to make undefined behavior hurt less, = which >> security-minded people will be very happy about. Notably, this means = that >> there's no inadvertent information leak when: >>=20 >> =E2=80=A2 The compiler re-uses stack slots, and a value is used = uninitialized. >> =E2=80=A2 The compiler re-uses a register, and a value is used = uninitialized. >> =E2=80=A2 Stack structs / arrays / unions with padding are copied. >> This patch only addresses stack and register information leaks. = There's many >> more infoleaks that we could address, and much more undefined = behavior that >> could be tamed. Let's keep this patch focused, and I'm happy to = address related >> issues elsewhere." >>=20 >> For more details, please refer to the LLVM code review discussion on = this patch: >> https://reviews.llvm.org/D54604 >>=20 >>=20 >> I also wrote a simple writeup for this task based on my study and = discussion with >> Kees Cook (cc=E2=80=99ing him) as following: >>=20 >>=20 >> thanks. >>=20 >> Qing >>=20 >> Support stack variables auto-initialization in GCC >>=20 >> 11/19/2020 >>=20 >> Qing Zhao >>=20 >> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D >>=20 >>=20 >> ** Background of the task: >>=20 >> The correponding GCC bugzilla RFE was created on 9/3/2018: >> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=3D87210 >>=20 >> A similar option for LLVM (around Nov, 2018) >> https://lists.llvm.org/pipermail/cfe-dev/2018-November/060172.html >> had invoked a lot of discussion before committed. >>=20 >> (The following are quoted from the comments of Alexander Potapenko in >> GCC bug 87210): >>=20 >> Finally, on Oct, 2019, upstream Clang supports force initialization >> of stack variables under the -ftrivial-auto-var-init flag. >>=20 >> -ftrivial-auto-var-init=3Dpattern initializes local variables with a = 0xAA pattern >> (actually it's more complicated, see https://reviews.llvm.org/D54604) >>=20 >> -ftrivial-auto-var-init=3Dzero provides zero-initialization of = locals. >> This mode isn't officially supported yet and is hidden behind an = additional >> = -enable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clang = flag. >> This is done to avoid creating a C++ dialect where all variables are >> zero-initialized. >>=20 >> Starting v5.2, Linux kernel has a CONFIG_INIT_STACK_ALL config that = performs >> the build with -ftrivial-auto-var-init=3Dpattern. This one isn't = widely adopted >> yet, partially because initializing locals with 0xAA isn't fast = enough. >>=20 >> Linus Torvalds is quite positive about zero-initializing the locals = though, >> see https://lkml.org/lkml/2019/7/30/1303: >>=20 >> "when a compiler has an option to initialize stack variables, it >> would probably _also_ be a very good idea for that compiler to then >> support a variable attribute that says "don't initialize _this_ >> variable, I will do that manually". >> I also think that the "initialize with poison" is >> pointless and wrong. Yes, it can find bugs, but it doesn't really = help >> improve the general situation, and people see it as a debugging tool, >> not a "improve code quality and improve the life of kernel = developers" >> tool. >>=20 >> So having a flag similar to -ftrivial-auto-var-init=3Dzero in GCC = will be >> appreciated by the Linux kernel community. >>=20 >> currently, kernel is using a gcc plugin to support stack variables >> auto-initialization: >> = https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/sc= ripts/gcc-plugins/structleak_plugin.c >>=20 >> ** Current situation: >>=20 >> A. Both Microsoft compiler and CLANG (APPLE AND GOOGLE) support = pattern init and >> zero init already; >> http://lists.llvm.org/pipermail/cfe-dev/2020-April/065221.html >> = https://msrc-blog.microsoft.com/2020/05/13/solving-uninitialized-stack-mem= ory-on-windows/ >> Pattern init is used in development build for debugging purpose, zero = init is >> used in production build for security purpose. >>=20 >> B. for CLANG, even though zero init is controlled by >> = "-fenable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clang= ", >> many end users have used it for production build. >> this functionality cannot be removed anymore. >> = "-fenable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clang= " >> might be changed to more meaningful name later in CLANG. >>=20 >>=20 >> ** My proposal: >>=20 >> A. add a new GCC option: (same name and meaning as CLANG) >> -ftrivial-auto-var-init=3D[pattern|zero], similar pattern init as = CLANG; >>=20 >> B. add a new attribute for variable: >> __attribute((uninitialized) >> the marked variable is uninitialized intentionaly for performance = purpose. >>=20 >> C. The implementation needs to keep the current static warning on = uninitialized >> variables untouched in order to avoid "forking the language=E2=80=9D. >>=20 >>=20 >>=20 >> On Nov 25, 2020, at 3:11 AM, Richard Biener = wrote: >>=20 >>=20 >>=20 >> I am planing to add a new phase immediately after = =E2=80=9Cpass_late_warn_uninitialized=E2=80=9D to initialize all = auto-variables that are >> not explicitly initialized in the declaration, the basic idea is = following: >>=20 >> ** The proposal: >>=20 >> A. add a new GCC option: (same name and meaning as CLANG) >> -ftrivial-auto-var-init=3D[pattern|zero], similar pattern init as = CLANG; >>=20 >> B. add a new attribute for variable: >> __attribute((uninitialized) >> the marked variable is uninitialized intentionaly for performance = purpose. >>=20 >> C. The implementation needs to keep the current static warning on = uninitialized >> variables untouched in order to avoid "forking the language". >>=20 >>=20 >> ** The implementation: >>=20 >> There are two major requirements for the implementation: >>=20 >> 1. all auto-variables that do not have an explicit initializer should = be initialized to >> zero by this option. (Same behavior as CLANG) >>=20 >> 2. keep the current static warning on uninitialized variables = untouched. >>=20 >> In order to satisfy 1, we should check whether an auto-variable has = initializer >> or not; >> In order to satisfy 2, we should add this new transformation after >> "pass_late_warn_uninitialized". >>=20 >> So, we should be able to check whether an auto-variable has = initializer or not after =E2=80=9Cpass_late_warn_uninitialized=E2=80=9D, >> If Not, then insert an initialization for it. >>=20 >> For this purpose, I guess that =E2=80=9CFOR_EACH_LOCAL_DECL=E2=80=9D = might be better? >>=20 >>=20 >> I think both as long as they are source-level auto-variables. Then = which one is better? >>=20 >>=20 >> Another issue is, in order to check whether an auto-variable has = initializer, I plan to add a new bit in =E2=80=9Cdecl_common=E2=80=9D = as: >> /* In a VAR_DECL, this is DECL_IS_INITIALIZED. */ >> unsigned decl_is_initialized :1; >>=20 >> /* IN VAR_DECL, set when the decl is initialized at the declaration. = */ >> #define DECL_IS_INITIALIZED(NODE) \ >> (DECL_COMMON_CHECK (NODE)->decl_common.decl_is_initialized) >>=20 >> set this bit when setting DECL_INITIAL for the variables in FE. then = keep it >> even though DECL_INITIAL might be NULLed. >>=20 >>=20 >> For locals it would be more reliable to set this = flag-Wmaybe-uninitialized. >>=20 >>=20 >>=20 >> You mean I can set the flag =E2=80=9CDECL_IS_INITIALIZED (decl)=E2=80=9D= inside the routine =E2=80=9Cgimpley_decl_expr=E2=80=9D (gimplify.c) as = following: >>=20 >> if (VAR_P (decl) && !DECL_EXTERNAL (decl)) >> { >> tree init =3D DECL_INITIAL (decl); >> ... >> if (init && init !=3D error_mark_node) >> { >> if (!TREE_STATIC (decl)) >> { >> DECL_IS_INITIALIZED(decl) =3D 1; >> } >>=20 >> Is this enough for all Frontends? Are there other places that I need = to maintain this bit? >>=20 >>=20 >>=20 >> Do you have any comment and suggestions? >>=20 >>=20 >> As said above - do you want to cover registers as well as locals? >>=20 >>=20 >> All the locals from the source-code point of view should be covered. = (=46rom my study so far, looks like that Clang adds that phase in FE). >> If GCC adds this phase in FE, then the following design requirement >>=20 >> C. The implementation needs to keep the current static warning on = uninitialized >> variables untouched in order to avoid "forking the language=E2=80=9D. >>=20 >> cannot be satisfied. Since gcc=E2=80=99s uninitialized variables = analysis is applied quite late. >>=20 >> So, we have to add this new phase after = =E2=80=9Cpass_late_warn_uninitialized=E2=80=9D. >>=20 >> I'd do >> the actual zeroing during RTL expansion instead since otherwise you >> have to figure youself whether a local is actually used (see = expand_stack_vars) >>=20 >>=20 >> Adding this new transformation during RTL expansion is okay. I will = check on this in more details to see how to add it to RTL expansion = phase. >>=20 >>=20 >> Note that optimization will already made have use of "uninitialized" = state >> of locals so depending on what the actual goal is here "late" may be = too late. >>=20 >>=20 >> This is a really good point=E2=80=A6 >>=20 >> In order to avoid optimization to use the =E2=80=9Cuninitialized=E2=80= =9D state of locals, we should add the zeroing phase as early as = possible (adding it in FE might be best >> for this issue). However, if we have to met the following = requirement: >>=20 >>=20 >> So is optimization supposed to pick up zero or is it supposed to act >> as if the initializer >> is unknown? >>=20 >> C. The implementation needs to keep the current static warning on = uninitialized >> variables untouched in order to avoid "forking the language=E2=80=9D. >>=20 >> We have to move the new phase after all the uninitialized analysis is = done in order to avoid =E2=80=9Cforking the language=E2=80=9D. >>=20 >> So, this is a problem that is not easy to resolve. >>=20 >>=20 >> Indeed, those are conflicting goals. >>=20 >> Do you have suggestion on this? >>=20 >>=20 >> No, not any easy ones. Doing more of the uninit analysis early = (there >> is already an early >> uninit pass) which would mean doing IPA analysis turing GCC into more >> of a static analysis >> tool. Theres the analyzer now, not sure if that can employ an early >> LTO phase for example. >>=20 >>=20 >>=20 >>=20 >> Richard.