Provide __builtin_alloca*_uninitialized variants

[melver]

Summary

When -ftrivial-auto-var-init= is enabled, allocas unconditionally
receive auto-initialization since [1].

In certain cases, it turns out, this is causing problems. For example,
when using alloca to add a random stack offset, as the Linux kernel does
on syscall entry [2]. In this case, none of the alloca'd stack memory is
ever used, and initializing it should be controllable; furthermore, it
is not always possible to safely call memset (see [2]).

Introduce __builtin_alloca_uninitialized() (and
__builtin_alloca_with_align_uninitialized), which never performs
initialization when -ftrivial-auto-var-init= is enabled.

[1] https://reviews.llvm.org/D60548
[2] https://lkml.kernel.org/r/[e-mail]

[glider]

I second this proposal.
https://reviews.llvm.org/D60548 suggests to handle this case using a pragma, but the required change seems to be more intrusive.

clang/lib/CodeGen/CGBuiltin.cpp

3437

For the sake of completeness, shall we also implement an uninitialized version of __builtin_alloca_with_align()?

[melver]

For completeness, also provide __builtin_alloca_with_align_uninitialized().

clang/lib/CodeGen/CGBuiltin.cpp

3437

You are right, I've added that, too.

[glider]

In D115440#3182585, @melver wrote:

For completeness, also provide __builtin_alloca_with_align_uninitialized().

Please make sure to update the patch description.
(You may have done already, but arc doesn't pull them automatically)

[melver]

GCC devs say that initializing explicit alloca() is a bug, because they aren't "automatic storage": https://lkml.kernel.org/r/[e-mail]
.. which is also the reason why GCC's behaviour differs here at the moment.

I actually agree with that, and alloca auto-init behaviour needs a new -mllvm param.

[jfb]

In D115440#3183778, @melver wrote:

GCC devs say that initializing explicit alloca() is a bug, because they aren't "automatic storage": https://lkml.kernel.org/r/[e-mail]
.. which is also the reason why GCC's behaviour differs here at the moment.

I actually agree with that, and alloca auto-init behaviour needs a new -mllvm param.

GCC developer Segher says:

The space allocated by alloca is not an automatic variable, so of course it is not affected by this compiler flag. And it should not, this flag is explicitly for *small fixed-size* stack variables (initialising others can be much too expensive).

C. Introduce a new __builtin_alloca_uninitialized().

That is completely backwards. That is the normal behaviour of alloca already. Also you can get __builtin_alloca inserted by the compiler (for a variable length array for example), and you typically do not want those initialised either, for the same reasons.

Segher is simply wrong.

Let's consult libc's own documentation https://www.gnu.org/software/libc/manual/html_node/Variable-Size-Automatic.html

It states:

Automatic Storage with Variable Size
The function alloca supports a kind of half-dynamic allocation in which blocks are allocated dynamically but freed automatically.

The manpage for alloca(3), states:

This temporary space is automatically freed when the function that called alloca() returns to its caller".

Refer to C17 which says:

Storage durations of objects
An object has a storage duration that determines its lifetime. There are four storage durations: static, thread, automatic, and allocated.

Clearly alloca is neither static, nor thread, nor allocated (see "memory management functions" too). It's automatic storage.

Further, Segher misunderstand the purpose of automatic initialization. The goal is explicitly to change implementation-defined behavior from "the compiler leaves stack and register slots with whatever value it happened to overlap with (potentially leading to info leaks or UaF)" to "the compiler always overwrites previous values that were in stack and register slots before they are reused (preventing a large number of info leaks and UaF)". Saying "the normal behaviour of alloca already" is correct, but it ignores the objective of the flag: to explicitly *change* that behavior. The flag opts-in to that implementation-defined behavior change. And it has a very specific purpose w.r.t. security. This has measurable results in terms of CVEs avoided.

Additionally the flag is not explicitly for small fixed-sized variables, that was stated very clearly when it was committed, and the documentation is unambiguous about this fact. If initialization is too expensive then developers needs to opt-out with mechanisms such as __attribute__((uninitialized)). The support for VLAs and alloca was done very much on purpose, and if developers turn on variable auto-init then they reasonably expect that all automatic variables are automatically initialized, including VLAs and alloca. The patch you propose here is a good idea, because there's currently no way to opt-out for alloca. We should adopt a solution such as this one, and synchronize with GCC so that developers can use the same code.

[melver]

Let's consult libc's own documentation https://www.gnu.org/software/libc/manual/html_node/Variable-Size-Automatic.html

It states:

Automatic Storage with Variable Size
The function alloca supports a kind of half-dynamic allocation in which blocks are allocated dynamically but freed automatically.

Thanks for clarifying -- this makes it clear it's "automatic storage". But I think we may still get conflicting views because -ftrivial-auto-var-init speaks about "automatic variables", and strictly speaking __builtin_alloca() is not a "variable".

Further, Segher misunderstand the purpose of automatic initialization. The goal is explicitly to change implementation-defined behavior from "the compiler leaves stack and register slots with whatever value it happened to overlap with (potentially leading to info leaks or UaF)" to "the compiler always overwrites previous values that were in stack and register slots before they are reused (preventing a large number of info leaks and UaF)". Saying "the normal behaviour of alloca already" is correct, but it ignores the objective of the flag: to explicitly *change* that behavior. The flag opts-in to that implementation-defined behavior change. And it has a very specific purpose w.r.t. security. This has measurable results in terms of CVEs avoided.

Additionally the flag is not explicitly for small fixed-sized variables, that was stated very clearly when it was committed, and the documentation is unambiguous about this fact. If initialization is too expensive then developers needs to opt-out with mechanisms such as __attribute__((uninitialized)). The support for VLAs and alloca was done very much on purpose, and if developers turn on variable auto-init then they reasonably expect that all automatic variables are automatically initialized, including VLAs and alloca. The patch you propose here is a good idea, because there's currently no way to opt-out for alloca. We should adopt a solution such as this one, and synchronize with GCC so that developers can use the same code.

I agree that having alloca() initialized is desirable normally.

The problem is that it is not at all obvious if -ftrivial-auto-var-init implies "automatic storage" or "automatic variables", only. (I mixed that up above as well!)

The latter is what (I think) led GCC folks to claim (and convinced me, too) that __builtin_alloca() is out-of-scope. That flag talks too much about "automatic stack variables".

If, however, you declare that it's the former, that's fine too. We just have to be very careful with how we document this -- do we want to replace "automatic stack variable" with "automatic stack storage" in documentation? We have to admit that this part is too easy to misinterpret, which is how we ended up here.

I'm going to reopen this.

[melver]

@tstellar , we were told maybe you have some input.

TLDR;

• Clang and GCC appear to behave differently wrt -ftrivial-auto-var-init= on __builtin_alloca() (and VLAs?)
• Clang's behavior is (per @jfb above) correct;
• How to document that better, so that this confusion can be avoided, and GCC can match Clang's behaviour.

Thanks.

[melver]

Hello,

We need a solution for this problem, and because I haven't heard any objections I'll assume the general approach is fine -- @jfb already kindly confirmed that "[...] patch you propose here is a good idea".

Review of the implementation would still be much appreciated!

Many thanks!

[glider]

FWIW the implementation looks good to me.

[rjmccall]

Yes, this LGTM implementation-wise.

clang/include/clang/Basic/Builtins.def

 BUILTIN(__builtin_prefetch, "vvC*.", "nc")
 BUILTIN(__builtin_readcyclecounter, "ULLi", "n")
 BUILTIN(__builtin_trap, "v", "nr")
 BUILTIN(__builtin_debugtrap, "v", "n")
 BUILTIN(__builtin_unreachable, "v", "nr")
 BUILTIN(__builtin_shufflevector, "v."   , "nct")
 BUILTIN(__builtin_convertvector, "v."   , "nct")
 BUILTIN(__builtin_alloca, "v*z"   , "Fn")
+BUILTIN(__builtin_alloca_uninitialized, "v*z", "Fn")
 BUILTIN(__builtin_alloca_with_align, "v*zIz", "Fn")
+BUILTIN(__builtin_alloca_with_align_uninitialized, "v*zIz", "Fn")
 BUILTIN(__builtin_call_with_static_chain, "v.", "nt")
 
 BUILTIN(__builtin_elementwise_abs, "v.", "nct")
 BUILTIN(__builtin_elementwise_max, "v.", "nct")
 BUILTIN(__builtin_elementwise_min, "v.", "nct")
 BUILTIN(__builtin_elementwise_ceil, "v.", "nct")
 BUILTIN(__builtin_reduce_max, "v.", "nct")
 BUILTIN(__builtin_reduce_min, "v.", "nct")

clang/lib/CodeGen/CGBuiltin.cpp

case Builtin::BI__builtin_fpclassify: {
 
     // return Result
     Builder.SetInsertPoint(End);
     return RValue::get(Result);
   }
 
   case Builtin::BIalloca:
   case Builtin::BI_alloca:
+  case Builtin::BI__builtin_alloca_uninitialized:
   case Builtin::BI__builtin_alloca: {
     Value *Size = EmitScalarExpr(E->getArg(0));
     const TargetInfo &TI = getContext().getTargetInfo();
     // The alignment of the alloca should correspond to __BIGGEST_ALIGNMENT__.
     const Align SuitableAlignmentInBytes =
         CGM.getContext()
             .toCharUnitsFromBits(TI.getSuitableAlign())
             .getAsAlign();
     AllocaInst *AI = Builder.CreateAlloca(Builder.getInt8Ty(), Size);
     AI->setAlignment(SuitableAlignmentInBytes);
+    if (BuiltinID != Builtin::BI__builtin_alloca_uninitialized)
       initializeAlloca(*this, AI, Size, SuitableAlignmentInBytes);
     return RValue::get(AI);
   }
 
+  case Builtin::BI__builtin_alloca_with_align_uninitialized:
   case Builtin::BI__builtin_alloca_with_align: {
     Value *Size = EmitScalarExpr(E->getArg(0));
     Value *AlignmentInBitsValue = EmitScalarExpr(E->getArg(1));
     auto *AlignmentInBitsCI = cast<ConstantInt>(AlignmentInBitsValue);
     unsigned AlignmentInBits = AlignmentInBitsCI->getZExtValue();
     const Align AlignmentInBytes =
         CGM.getContext().toCharUnitsFromBits(AlignmentInBits).getAsAlign();
     AllocaInst *AI = Builder.CreateAlloca(Builder.getInt8Ty(), Size);
     AI->setAlignment(AlignmentInBytes);
+    if (BuiltinID != Builtin::BI__builtin_alloca_with_align_uninitialized)
       initializeAlloca(*this, AI, Size, AlignmentInBytes);
     return RValue::get(AI);
   }
 
   case Builtin::BIbzero:
   case Builtin::BI__builtin_bzero: {
     Address Dest = EmitPointerWithAlignment(E->getArg(0));
     Value *SizeVal = EmitScalarExpr(E->getArg(1));
     EmitNonNullArgCheck(RValue::get(Dest.getPointer()), E->getArg(0)->getType(),

clang/lib/Sema/SemaChecking.cpp

case Builtin::BI__builtin_shufflevector:
     return SemaBuiltinShuffleVector(TheCall);
     // TheCall will be freed by the smart pointer here, but that's fine, since
     // SemaBuiltinShuffleVector guts it, but then doesn't release it.
   case Builtin::BI__builtin_prefetch:
     if (SemaBuiltinPrefetch(TheCall))
       return ExprError();
     break;
   case Builtin::BI__builtin_alloca_with_align:
+  case Builtin::BI__builtin_alloca_with_align_uninitialized:
     if (SemaBuiltinAllocaWithAlign(TheCall))
       return ExprError();
     LLVM_FALLTHROUGH;
   case Builtin::BI__builtin_alloca:
+  case Builtin::BI__builtin_alloca_uninitialized:
     Diag(TheCall->getBeginLoc(), diag::warn_alloca)
         << TheCall->getDirectCallee();
     break;
   case Builtin::BI__arithmetic_fence:
     if (SemaBuiltinArithmeticFence(TheCall))
       return ExprError();
     break;
   case Builtin::BI__assume:

clang/test/CodeGenCXX/trivial-auto-var-init.cpp

 // PATTERN:       %[[SIZE:[a-z0-9]+]] = sext i32 %{{.*}} to i64
 // PATTERN-NEXT:  %[[ALLOCA:[a-z0-9]+]] = alloca i8, i64 %[[SIZE]], align 128
 // PATTERN-NEXT:  call void @llvm.memset{{.*}}(i8* align 128 %[[ALLOCA]], i8 -86, i64 %[[SIZE]], i1 false), !annotation [[AUTO_INIT:!.+]]
 void test_alloca_with_align(int size) {
   void *ptr = __builtin_alloca_with_align(size, 1024);
   used(ptr);
 }
 
+// UNINIT-LABEL:  test_alloca_uninitialized(
+// ZERO-LABEL:    test_alloca_uninitialized(
+// ZERO:          %[[SIZE:[a-z0-9]+]] = sext i32 %{{.*}} to i64
+// ZERO-NEXT:     %[[ALLOCA:[a-z0-9]+]] = alloca i8, i64 %[[SIZE]], align [[ALIGN:[0-9]+]]
+// ZERO-NOT:      call void @llvm.memset
+// PATTERN-LABEL: test_alloca_uninitialized(
+// PATTERN:       %[[SIZE:[a-z0-9]+]] = sext i32 %{{.*}} to i64
+// PATTERN-NEXT:  %[[ALLOCA:[a-z0-9]+]] = alloca i8, i64 %[[SIZE]], align [[ALIGN:[0-9]+]]
+// PATTERN-NOT:   call void @llvm.memset
+void test_alloca_uninitialized(int size) {
+  void *ptr = __builtin_alloca_uninitialized(size);
+  used(ptr);
+}
+
+// UNINIT-LABEL:  test_alloca_with_align_uninitialized(
+// ZERO-LABEL:    test_alloca_with_align_uninitialized(
+// ZERO:          %[[SIZE:[a-z0-9]+]] = sext i32 %{{.*}} to i64
+// ZERO-NEXT:     %[[ALLOCA:[a-z0-9]+]] = alloca i8, i64 %[[SIZE]], align 128
+// ZERO-NOT:      call void @llvm.memset
+// PATTERN-LABEL: test_alloca_with_align_uninitialized(
+// PATTERN:       %[[SIZE:[a-z0-9]+]] = sext i32 %{{.*}} to i64
+// PATTERN-NEXT:  %[[ALLOCA:[a-z0-9]+]] = alloca i8, i64 %[[SIZE]], align 128
+// PATTERN-NOT:   call void @llvm.memset
+void test_alloca_with_align_uninitialized(int size) {
+  void *ptr = __builtin_alloca_with_align_uninitialized(size, 1024);
+  used(ptr);
+}
+
 // UNINIT-LABEL:  test_struct_vla(
 // ZERO-LABEL:    test_struct_vla(
 // ZERO:  %[[SIZE:[0-9]+]] = mul nuw i64 %{{.*}}, 16
 // ZERO:  call void @llvm.memset{{.*}}(i8* align 16 %{{.*}}, i8 0, i64 %[[SIZE]], i1 false), !annotation [[AUTO_INIT:!.+]]
 // PATTERN-LABEL: test_struct_vla(
 // PATTERN:  %vla.iszerosized = icmp eq i64 %{{.*}}, 0
 // PATTERN:  br i1 %vla.iszerosized, label %vla-init.cont, label %vla-setup.loop
 // PATTERN: vla-setup.loop:

clang/test/Sema/warn-alloca.c

 }
 
 void test2(int a) {
   __builtin_alloca_with_align(a, 32);
 #ifndef SILENCE
   // expected-warning@-2 {{use of function '__builtin_alloca_with_align' is discouraged; there is no way to check for failure but failure may still occur, resulting in a possibly exploitable security vulnerability}}
 #endif
 }
+
+void test3(int a) {
+  __builtin_alloca_uninitialized(a);
+#ifndef SILENCE
+  // expected-warning@-2 {{use of function '__builtin_alloca_uninitialized' is discouraged; there is no way to check for failure but failure may still occur, resulting in a possibly exploitable security vulnerability}}
+#endif
+}
+
+void test4(int a) {
+  __builtin_alloca_with_align_uninitialized(a, 32);
+#ifndef SILENCE
+  // expected-warning@-2 {{use of function '__builtin_alloca_with_align_uninitialized' is discouraged; there is no way to check for failure but failure may still occur, resulting in a possibly exploitable security vulnerability}}
+#endif
+}