[llvm-commits] [llvm] r58673 - in /llvm/trunk: include/llvm/CodeGen/Passes.h lib/CodeGen/LLVMTargetMachine.cpp lib/CodeGen/StackProtector.cpp

[Bill Wendling]

On Tue, Nov 4, 2008 at 5:28 AM, Chris Lattner <clattner at apple.com> wrote:
> On Nov 3, 2008, at 6:10 PM, Bill Wendling wrote:
>> URL: http://llvm.org/viewvc/llvm-project?rev=58673&view=rev
>> Log:
>> Initial checkin for stack protectors. Here's what it does:
>
> Cool!  Some questions/comments below:
>
>> @@ -60,6 +60,11 @@
>> EnableFastISelOption("fast-isel", cl::Hidden,
>>   cl::desc("Enable the experimental \"fast\" instruction selector"));
>>
>> +// Enable stack protectors.
>> +static cl::opt<int>
>> +EnableStackProtector("enable-stack-protector", cl::init(0),
>> +                     cl::desc("Use ProPolice as a stack protection
>> method."));
>
> I don't know what "ProPolice" is, but it sounds like it may be a
> trademark or something.  Please just use "stack canary" or something
> generic like that.
>
It's used in the GCC documentation for the flag:

common.opt:
fstack-protector
Common Report Var(flag_stack_protect, 1) Init(-1)
Use propolice as a stack protection method

But I can live without it.

> Also, you should use an enum for this, not magic integer numbers.  I'd
> like to see -enable-stack-protector=all | some | none.
>
Okay.

>> @@ -165,6 +170,8 @@
>>   if (!Fast)
>>     PM.add(createCodeGenPreparePass(getTargetLowering()));
>>
>> +  PM.add(createStackProtectorPass(EnableStackProtector));
>
> Why do you add the pass even when it is not enabled?  Have you looked
> at whether adding this pass ends up breaking analysis chains, and
> requiring them to be run multiple times?  For example, does dominators
> end up being run more frequently with this?  If so, the pass should
> update dominators instead of invalidating it.
>
Okay. No. Don't know.

>> +// random value in it is stored onto the stack before the local
>> variables are
>> +// allocated. Upon exitting the block, the stored value is checked.
>> If it's
>
> typo exiting.
>
>> +// Enable stack protectors.
>> +static cl::opt<unsigned>
>> +SSPBufferSize("ssp-buffer-size", cl::init(8),
>> +              cl::desc("The lower bound for a buffer to be
>> considered for "
>> +                       "stack smashing protection."));
>
> why the contraction ssp?  Please use something like "stack-protector-
> buffer-size" or something like that, for similarity with the enable-
> stack-protector option.
>
It's the same name that's used in GCC. I'm not tied to the name, and
I'm not expecting this to be used much at all.

>> +namespace {
>> +  class VISIBILITY_HIDDEN StackProtector : public FunctionPass {
>> +    // Level == 0  --  Stack protectors are off.
>> +    // Level == 1  --  Stack protectors are on only for some
>> functions.
>> +    // Level == 2  --  Stack protectors are on for all functions.
>> +    int Level;
>
> I think that this should be an enum in Passes.h, which can also be
> used above for the command line option.
>
Okay.

>>
>> ++    /// RequiresStackProtector - Check whether or not this
>> function needs a
>> +    /// stack protector based upon the stack protector level.
>> +    bool RequiresStackProtector();
>
> This should be const.
>
Oh yeah!

>> +/// InsertStackProtectorPrologue - Insert code into the entry block
>> that stores
>> +/// the __stack_chk_guard variable onto the stack.
>> +void StackProtector::InsertStackProtectorPrologue() {
>> +  BasicBlock &Entry = F->getEntryBlock();
>> +  Instruction &InsertPt = Entry.front();
>> +
>> +  const char *StackGuardStr = "__stack_chk_guard";
>> +  StackGuardVar = M->getNamedGlobal(StackGuardStr)
>
> If the module already has a '__stack_chk_guard' global with the wrong
> type, I think this code will crash, see below:
>
Hrh?

>>
>> +  if (!StackGuardVar)
>> +    StackGuardVar = new
>> GlobalVariable(PointerType::getUnqual(Type::Int8Ty),
>> +                                       false,
>> GlobalValue::ExternalLinkage,
>> +                                       0, StackGuardStr, M);
>
> We really want a Module::getOrInsertGlobal() method that works like
> getOrInsertFunction.  It would do the bitcast to solve the problem
> above and factor away this code.  Can you please add this?
>
Sure. That would be nice.

>> +  StackProtFrameSlot = new
>> AllocaInst(PointerType::getUnqual(Type::Int8Ty),
>> +                                      "StackProt_Frame", &InsertPt);
>
> What ensures that this alloca ends up in the right place in the stack
> frame at codegen time?
>
We looked at the code in code gen -- the lowering of allocas. It's
doing the "right thing" with regard to this. There might be some
special magic that needs to be done, though.

>> +  LoadInst *LI = new LoadInst(StackGuardVar, "StackGuard", true,
>> &InsertPt);
>> +  new StoreInst(LI, StackProtFrameSlot, true, &InsertPt);
>
> Please add a comment indicating why these are volatile.  Why is the
> load volatile?
>
This one probably doesn't need to be.

>> +/// InsertStackProtectorEpilogue - Insert code before the return
>> instructions
>> +/// checking the stack value that was stored in the prologue. If it
>> isn't the
>> +/// same as the original value, then call a "failure" function.
>> +void StackProtector::InsertStackProtectorEpilogue() {
>> +  // Create the basic block to jump to when the guard check fails.
>> +  CreateFailBB();
>> +
>> +  Function::iterator I = F->begin(), E = F->end();
>> +  std::vector<BasicBlock*> ReturnBBs;
>> +  ReturnBBs.reserve(F->size());
>> +
>> +  for (; I != E; ++I)
>> +    if (isa<ReturnInst>((*I).getTerminator()))
>> +      ReturnBBs.push_back(I);
>
> Two things: first, use I->getTerminator.  Second, why do you make a
> vector of returns and iterate over the vector?  You should be able to
> handle all of these with one pass over the function without the
> intermediate vector.  Also, F->size() is linear time, so that itself
> does a pass over the function.
>
I'm modifying the Function (inserting new blocks). Won't that
invalidate the iterators?

>> +  for (std::vector<BasicBlock*>::iterator
>> +         II = ReturnBBs.begin(), IE = ReturnBBs.end(); II != IE; +
>> +II) {
>> +    BasicBlock *BB = *II;
>> +    ReturnInst *RI = cast<ReturnInst>(BB->getTerminator());
>> +    Function::iterator InsPt = BB; ++InsPt; // Insertion point for
>> new BB.
>> +
>> +    BasicBlock *NewBB = BasicBlock::Create("SPRet", F, InsPt);
>> +
>> +    // Move the return instruction into the new basic block.
>> +    RI->removeFromParent();
>> +    NewBB->getInstList().insert(NewBB->begin(), RI);
>
> Please use BasicBlock::splitBasicBlock to simplify this code.
>
Okay.

>> +/// CreateFailBB - Create a basic block to jump to when the stack
>> protector
>> +/// check fails.
>> +void StackProtector::CreateFailBB() {
>> +  assert(!FailBB && "Failure basic block already created?!");
>> +  FailBB = BasicBlock::Create("CallStackCheckFailBlk", F);
>> +  std::vector<const Type*> Params;
>> +  Constant *StackChkFail =
>> +    M->getOrInsertFunction("__stack_chk_fail",
>> +                           FunctionType::get(Type::VoidTy, Params,
>> false));
>
> You can eliminate the temporary vector by using the varargs form of
> getOrInsertFunction:
>
> Constant *StackChkFail =
>   M->getOrInsertFunction("__stack_chk_fail", Type::VoidTy, NULL);
>
Ah! Good.

>> +/// RequiresStackProtector - Check whether or not this function
>> needs a stack
>> +/// protector based upon the stack protector level.
>> +bool StackProtector::RequiresStackProtector() {
>> +  switch (Level) {
>> +  default: return false;
>> +  case 2:  return true;
>> +  case 1: {
>
> Please use enums instead of magic numbers :)
>
But the magic makes it work!!! ;-)

>> +    // If the size of the local variables allocated on the stack is
>> greater than
>> +    // SSPBufferSize, then we require a stack protector.
>> +    uint64_t StackSize = 0;
>> +
>> +    for (Function::iterator I = F->begin(), E = F->end(); I != E; +
>> +I) {
>> +      BasicBlock *BB = I;
>> +
>> +      for (BasicBlock::iterator
>> +             II = BB->begin(), IE = BB->end(); II != IE; ++II)
>> +        if (AllocaInst *AI = dyn_cast<AllocaInst>(II))
>> +          if (ConstantInt *CI = dyn_cast<ConstantInt>(AI-
>> >getArraySize())) {
>> +            const APInt &Size = CI->getValue();
>> +            StackSize += Size.getZExtValue() * 8;
>> +          }
>
> This isn't sufficient.  If you have "alloca i32", the array size is
> 1.  You need to use TargetData to multiply the array size by the size
> of the type.  Also, is the protection size in bits or bytes?
>
Bytes, so the "* 8" is wrong. I need to flesh this out some more.

> Does GCC ignore variable sized stack allocations?
>
>> +    if (SSPBufferSize <= StackSize)
>> +      return true;
>
> This check should be moved inside the loop so that you stop scanning a
> function as soon as you see at least enough to qualify for stack
> protection.
>
Okay.

>>
>> +
>> +// [EOF] StackProtector.cpp
>
> Urr?
>
Totally the end of file! ;-)

-bw