How to traverse all the local variables that declared in the current routine?
Qing Zhao
QING.ZHAO@ORACLE.COM
Wed Dec 2 15:36:12 GMT 2020
> On Dec 2, 2020, at 2:45 AM, Richard Biener <richard.guenther@gmail.com> wrote:
>
> On Tue, Dec 1, 2020 at 8:49 PM Qing Zhao <QING.ZHAO@oracle.com> wrote:
>>
>> Hi, Richard,
>>
>> Could you please comment on the following approach:
>>
>> Instead of adding the zero-initializer quite late at the pass “pass_expand”, we can add it as early as during gimplification.
>> However, we will mark these new added zero-initializers as “artificial”. And passing this “artificial” information to
>> “pass_early_warn_uninitialized” and “pass_late_warn_uninitialized”, in these two uninitialized variable analysis passes,
>> (i.e., in tree-sea-uninit.c) We will update the checking on “ssa_undefined_value_p” to consider “artificial” zero-initializers.
>> (i.e, if the def_stmt is marked with “artificial”, then it’s a undefined value).
>>
>> With such approach, we should be able to address all those conflicts.
>>
>> Do you see any obvious issue with this approach?
>
> Yes, DSE will happily elide an explicit zero-init following the
> artificial one leading to false uninit diagnostics.
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.
>
> What's the intended purpose of the zero-init?
The purpose of this new option is: (from the original LLVM patch submission):
"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.
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:
• The compiler re-uses stack slots, and a value is used uninitialized.
• The compiler re-uses a register, and a value is used uninitialized.
• 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."
For more details, please refer to the LLVM code review discussion on this patch:
https://reviews.llvm.org/D54604
I also wrote a simple writeup for this task based on my study and discussion with
Kees Cook (cc’ing him) as following:
thanks.
Qing
Support stack variables auto-initialization in GCC
11/19/2020
Qing Zhao
=======================================================
** Background of the task:
The correponding GCC bugzilla RFE was created on 9/3/2018:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87210
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.
(The following are quoted from the comments of Alexander Potapenko in
GCC bug 87210):
Finally, on Oct, 2019, upstream Clang supports force initialization
of stack variables under the -ftrivial-auto-var-init flag.
-ftrivial-auto-var-init=pattern initializes local variables with a 0xAA pattern
(actually it's more complicated, see https://reviews.llvm.org/D54604)
-ftrivial-auto-var-init=zero 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.
Starting v5.2, Linux kernel has a CONFIG_INIT_STACK_ALL config that performs
the build with -ftrivial-auto-var-init=pattern. This one isn't widely adopted
yet, partially because initializing locals with 0xAA isn't fast enough.
Linus Torvalds is quite positive about zero-initializing the locals though,
see https://lkml.org/lkml/2019/7/30/1303:
"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.
So having a flag similar to -ftrivial-auto-var-init=zero in GCC will be
appreciated by the Linux kernel community.
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/scripts/gcc-plugins/structleak_plugin.c
** Current situation:
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-memory-on-windows/
Pattern init is used in development build for debugging purpose, zero init is
used in production build for security purpose.
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.
** My proposal:
A. add a new GCC option: (same name and meaning as CLANG)
-ftrivial-auto-var-init=[pattern|zero], similar pattern init as CLANG;
B. add a new attribute for variable:
__attribute((uninitialized)
the marked variable is uninitialized intentionaly for performance purpose.
C. The implementation needs to keep the current static warning on uninitialized
variables untouched in order to avoid "forking the language”.
>
>> On Nov 25, 2020, at 3:11 AM, Richard Biener <richard.guenther@gmail.com> wrote:
>>
>>
>>
>> I am planing to add a new phase immediately after “pass_late_warn_uninitialized” to initialize all auto-variables that are
>> not explicitly initialized in the declaration, the basic idea is following:
>>
>> ** The proposal:
>>
>> A. add a new GCC option: (same name and meaning as CLANG)
>> -ftrivial-auto-var-init=[pattern|zero], similar pattern init as CLANG;
>>
>> B. add a new attribute for variable:
>> __attribute((uninitialized)
>> the marked variable is uninitialized intentionaly for performance purpose.
>>
>> C. The implementation needs to keep the current static warning on uninitialized
>> variables untouched in order to avoid "forking the language".
>>
>>
>> ** The implementation:
>>
>> There are two major requirements for the implementation:
>>
>> 1. all auto-variables that do not have an explicit initializer should be initialized to
>> zero by this option. (Same behavior as CLANG)
>>
>> 2. keep the current static warning on uninitialized variables untouched.
>>
>> 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".
>>
>> So, we should be able to check whether an auto-variable has initializer or not after “pass_late_warn_uninitialized”,
>> If Not, then insert an initialization for it.
>>
>> For this purpose, I guess that “FOR_EACH_LOCAL_DECL” might be better?
>>
>>
>> I think both as long as they are source-level auto-variables. Then which one is better?
>>
>>
>> Another issue is, in order to check whether an auto-variable has initializer, I plan to add a new bit in “decl_common” as:
>> /* In a VAR_DECL, this is DECL_IS_INITIALIZED. */
>> unsigned decl_is_initialized :1;
>>
>> /* 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)
>>
>> set this bit when setting DECL_INITIAL for the variables in FE. then keep it
>> even though DECL_INITIAL might be NULLed.
>>
>>
>> For locals it would be more reliable to set this flag-Wmaybe-uninitialized.
>>
>>
>>
>> You mean I can set the flag “DECL_IS_INITIALIZED (decl)” inside the routine “gimpley_decl_expr” (gimplify.c) as following:
>>
>> if (VAR_P (decl) && !DECL_EXTERNAL (decl))
>> {
>> tree init = DECL_INITIAL (decl);
>> ...
>> if (init && init != error_mark_node)
>> {
>> if (!TREE_STATIC (decl))
>> {
>> DECL_IS_INITIALIZED(decl) = 1;
>> }
>>
>> Is this enough for all Frontends? Are there other places that I need to maintain this bit?
>>
>>
>>
>> Do you have any comment and suggestions?
>>
>>
>> As said above - do you want to cover registers as well as locals?
>>
>>
>> All the locals from the source-code point of view should be covered. (From 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
>>
>> C. The implementation needs to keep the current static warning on uninitialized
>> variables untouched in order to avoid "forking the language”.
>>
>> cannot be satisfied. Since gcc’s uninitialized variables analysis is applied quite late.
>>
>> So, we have to add this new phase after “pass_late_warn_uninitialized”.
>>
>> 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)
>>
>>
>> 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.
>>
>>
>> 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.
>>
>>
>> This is a really good point…
>>
>> In order to avoid optimization to use the “uninitialized” 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:
>>
>>
>> So is optimization supposed to pick up zero or is it supposed to act
>> as if the initializer
>> is unknown?
>>
>> C. The implementation needs to keep the current static warning on uninitialized
>> variables untouched in order to avoid "forking the language”.
>>
>> We have to move the new phase after all the uninitialized analysis is done in order to avoid “forking the language”.
>>
>> So, this is a problem that is not easy to resolve.
>>
>>
>> Indeed, those are conflicting goals.
>>
>> Do you have suggestion on this?
>>
>>
>> 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.
>>
>>
>>
>>
>> Richard.
More information about the Gcc-patches
mailing list