Don't allow instructions/globals to use alignment > 2**29.

lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp

 //===- NaClBitcodeReader.cpp ----------------------------------------------===//
 //     Internal NaClBitcodeReader implementation
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
@@ skipping 10 matching lines @@
 #include "llvm/IR/DerivedTypes.h"
 #include "llvm/IR/InlineAsm.h"
 #include "llvm/IR/IntrinsicInst.h"
 #include "llvm/IR/Module.h"
 #include "llvm/IR/OperandTraits.h"
 #include "llvm/IR/Operator.h"
 #include "llvm/Support/DataStream.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/MathExtras.h"
 #include "llvm/Support/MemoryBuffer.h"
-#include "llvm/Support/raw_ostream.h"
 using namespace llvm;
 
 
 cl::opt<bool>
 llvm::PNaClAllowLocalSymbolTables(
     "allow-local-symbol-tables",
     cl::desc("Allow (function) local symbol tables in PNaCl bitcode files"),
     cl::init(false));
 
 void NaClBitcodeReader::FreeState() {
@@ skipping 135 matching lines @@
 //===----------------------------------------------------------------------===//
 
 
 std::error_code NaClBitcodeReader::Error(ErrorType E,
                                          const std::string &Message) const {
   if (Verbose)
     *Verbose << "Error: " << Message << "\n";
   return Error(E);
 }
 
+std::error_code NaClBitcodeReader::getAlignmentValue(
+    uint64_t Exponent, unsigned &Alignment) {
+  // Note: Alignement = 2 ** (Exponent - 1).

[jvoung (off chromium), 2014/12/16 23:24:39]

Alignement -> Alignment

[Karl, 2014/12/17 20:52:38]

Done.

+  if (Exponent == 0) {

[jvoung (off chromium), 2014/12/16 23:24:39]

Hmm, I don't think we ever said that "alloca" could not have an align of 0.

I think only for load/store was that ever checked by the ABI checker. I'm not
sure what C/C++ source would give you an alloca with align 0, but it might
better to continue to allow 0. Is the reason for this the "Exp - 1" vs the "(1
<< x) >> 1" ?

[Karl, 2014/12/17 20:52:38]

Good point. I forgot about the alloca default case. Putting back to use two
shifts.

+    Alignment = 1; // Just in case it is accessed.
+    return Error(InvalidValue, "Alignment must be greater than 0");
+  }
+  if (Exponent > 30) { // Note: Exponent is one larger than actual.

[jvoung (off chromium), 2014/12/16 23:24:39]

"than actual" -> "than the limit"?

[jvoung (off chromium), 2014/12/16 23:24:39]

include/llvm/IR/Value.h has "MaximumAlignment", which is "29", so you may be
able to use that as the constant.

The downside of using that is if it changes out from under us.

Maybe use that + 1, and static assert that it is 29? If upstream changes it, we
could then fork the limit or follow... WDYT?

[Karl, 2014/12/17 20:52:38]

Good point. Adding reference to the constant.

[Karl, 2014/12/17 20:52:38]

Done.

+    Alignment = 1; // Just in case it is accessed.
+    std::string Buffer;
+    raw_string_ostream StrBuf(Buffer);
+    StrBuf << "Alignment can't be greater than 2**29. Found: 2**"
+           << (Exponent - 1);
+    return Error(InvalidValue, StrBuf.str());
+  }
+  uint32_t FixedExponent = Exponent - 1;
+  Alignment = 1 << FixedExponent;
+  return std::error_code();
+}
+
 std::error_code NaClBitcodeReader::ParseTypeTable() {
   DEBUG(dbgs() << "-> ParseTypeTable\n");
   if (Stream.EnterSubBlock(naclbitc::TYPE_BLOCK_ID_NEW))
     return Error(InvalidRecord, "Malformed block record");
 
   std::error_code result = ParseTypeTableBody();
   if (!result)
     DEBUG(dbgs() << "<- ParseTypeTable\n");
   return result;
 }
@@ skipping 198 matching lines @@
       switch (Bitcode) {
       default:
         return Reader.Error(NaClBitcodeReader::InvalidValue,
                             "Unknown global variable entry");
       case naclbitc::GLOBALVAR_VAR:
         // Start the definition of a global variable.
         if (ProcessingGlobal || Record.size() != 2)
           return Reader.Error(NaClBitcodeReader::InvalidRecord,
                               "Bad GLOBALVAR_VAR record");
         ProcessingGlobal = true;
-        VarAlignment = (1 << Record[0]) >> 1;
+        if (std::error_code EC =
+            Reader.getAlignmentValue(Record[0], VarAlignment))
+          return EC;
         VarIsConstant = Record[1] != 0;
         // Assume (by default) there is a single initializer.
         VarInitializersNeeded = 1;
         break;
       case naclbitc::GLOBALVAR_COMPOUND:
         // Global variable has multiple initializers. Changes the
         // default number of initializers to the given value in
         // Record[0].
         if (!ProcessingGlobal || !VarType.empty() ||
             VarInitializersNeeded != 1 || Record.size() != 1)
@@ skipping 1057 matching lines @@
       break;
     }
 
     case naclbitc::FUNC_CODE_INST_ALLOCA: { // ALLOCA: [op, align]
       if (Record.size() != 2)
         return Error(InvalidRecord, "Invalid ALLOCA record");
       Value *Size;
       unsigned OpNum = 0;
       if (popValue(Record, &OpNum, NextValueNo, &Size))
         return Error(InvalidRecord, "Invalid ALLOCA record");
-      unsigned Align = Record[1];
-      I = new AllocaInst(Type::getInt8Ty(Context), Size, (1 << Align) >> 1);
+      unsigned Alignment;
+      if (std::error_code EC = getAlignmentValue(Record[1], Alignment))
+        return EC;
+      I = new AllocaInst(Type::getInt8Ty(Context), Size, Alignment);
       break;
     }
     case naclbitc::FUNC_CODE_INST_LOAD: {
       // LOAD: [op, align, ty]
       unsigned OpNum = 0;
       Value *Op;
       if (popValue(Record, &OpNum, NextValueNo, &Op) ||
           Record.size() != 3)
         return Error(InvalidRecord, "Invalid LOAD record");
 
       // Add pointer cast to op.
       Type *T = getTypeByID(Record[2]);
       if (T == nullptr)
         return Error(InvalidType, "Invalid type for load instruction");
       Op = ConvertOpToType(Op, T->getPointerTo(), CurBBNo);
       if (Op == nullptr)
         return Error(InvalidTypeForValue, "Can't convert cast to type");
-      I = new LoadInst(Op, "", false, (1 << Record[OpNum]) >> 1);
+      unsigned Alignment;
+      if (std::error_code EC =
+         getLoadStoreAlignmentValue(Record[OpNum], TheModule->getDataLayout(),
+                                    T, "load", Alignment)) {
+        return EC;
+      }
+      I = new LoadInst(Op, "", false, Alignment);
       break;
     }
     case naclbitc::FUNC_CODE_INST_STORE: {
       // STORE: [ptr, val, align]
       unsigned OpNum = 0;
       Value *Val, *Ptr;
       if (popValue(Record, &OpNum, NextValueNo, &Ptr) ||
           popValue(Record, &OpNum, NextValueNo, &Val) ||
           OpNum+1 != Record.size())
         return Error(InvalidRecord, "Invalid STORE record");
       Val = ConvertOpToScalar(Val, CurBBNo);
-      Ptr = ConvertOpToType(Ptr, Val->getType()->getPointerTo(), CurBBNo);
+      Type *ValType = Val->getType();
+      Ptr = ConvertOpToType(Ptr, ValType->getPointerTo(), CurBBNo);
       if (Ptr == nullptr)
         return Error(InvalidTypeForValue, "Can't convert cast to type");
-      I = new StoreInst(Val, Ptr, false, (1 << Record[OpNum]) >> 1);
+      unsigned Alignment;
+      if (std::error_code EC =
+         getLoadStoreAlignmentValue(Record[OpNum], TheModule->getDataLayout(),

[jvoung (off chromium), 2014/12/16 23:24:39]

A little ambivalent about the extra checks -- I don't think it would be easy to
switch the check on here, but switch it off later when the abi checker runs, so
that there isn't new overhead. On the other hand, the overhead is *probably*
negligible.

[Karl, 2014/12/17 20:52:38]

I decided to remove this, since the bitcode reader accepts 0 (for non-finalized
pexes), but a finalized pexe applies the additional constraints. Hence, we must
wait for the ABI verifier to find the mistake.

+                                    ValType, "store", Alignment)) {
+        return EC;
+      }
+      I = new StoreInst(Val, Ptr, false, Alignment);
       break;
     }
     case naclbitc::FUNC_CODE_INST_CALL:
     case naclbitc::FUNC_CODE_INST_CALL_INDIRECT: {
       // CALL: [cc, fnid, arg0, arg1...]
       // CALL_INDIRECT: [cc, fnid, returnty, args...]
       if ((Record.size() < 2) ||
           (BitCode == naclbitc::FUNC_CODE_INST_CALL_INDIRECT &&
            Record.size() < 3))
         return Error(InvalidRecord, "Invalid CALL record");
@@ skipping 369 matching lines @@
       Materializer->releaseBuffer();
     delete M;
     return EC;
   }
 
   // TODO: Restore the use-lists to the in-memory state when the bitcode was
   // written.  We must defer until the Module has been fully materialized.
 
   return M;
 }