GP-5869 Fix for some out of bounds array indices and shift amounts

Ghidra/Features/Decompiler/src/decompile/cpp/.gitignore

@@ -7,7 +7,7 @@ decomp_dbg
 decomp_opt
 ghidra_dbg
 ghidra_opt
-ghidra_test_dbg
+decomp_test_dbg
 sleigh_dbg
 com_dbg
 com_opt

Ghidra/Features/Decompiler/src/decompile/cpp/Makefile

@@ -99,7 +99,7 @@ SPECIAL=consolemain sleighexample test
 # Any additional modules for the command line decompiler
 EXTRA= $(filter-out $(CORE) $(DECCORE) $(SLEIGH) $(GHIDRA) $(SLACOMP) $(SPECIAL),$(ALL_NAMES))
 
-EXECS=decomp_dbg decomp_opt ghidra_test_dbg ghidra_dbg ghidra_opt sleigh_dbg sleigh_opt libdecomp_dbg.a libdecomp.a
+EXECS=decomp_dbg decomp_opt decomp_test_dbg ghidra_dbg ghidra_opt sleigh_dbg sleigh_opt libdecomp_dbg.a libdecomp.a
 
 # Possible conditional compilation flags
 #     __TERMINAL__             # Turn on terminal support for console mode
@@ -191,7 +191,7 @@ endif
 ifeq ($(MAKECMDGOALS),decomp_opt)
 	DEPNAMES=com_opt/depend
 endif
-ifneq (,$(filter $(MAKECMDGOALS),ghidra_test_dbg test))
+ifneq (,$(filter $(MAKECMDGOALS),decomp_test_dbg test))
 	DEPNAMES=test_dbg/depend
 endif
 ifeq ($(MAKECMDGOALS),reallyclean)
@@ -253,11 +253,12 @@ decomp_dbg:	$(COMMANDLINE_DBG_OBJS)
 decomp_opt:	$(COMMANDLINE_OPT_OBJS)
 	$(CXX) $(OPT_CXXFLAGS) $(ARCH_TYPE) -o decomp_opt $(COMMANDLINE_OPT_OBJS) $(BFDLIB) $(LNK)
 
-ghidra_test_dbg:	$(TEST_DEBUG_OBJS)
-	$(CXX) $(DBG_CXXFLAGS) $(ARCH_TYPE) -o ghidra_test_dbg $(TEST_DEBUG_OBJS) $(BFDLIB) $(LNK)
+#decomp_test_dbg:	DBG_CXXFLAGS += -D_GLIBCXX_ASSERTIONS -fsanitize=address,undefined
+decomp_test_dbg:	$(TEST_DEBUG_OBJS)
+	$(CXX) $(DBG_CXXFLAGS) $(ARCH_TYPE) -o decomp_test_dbg $(TEST_DEBUG_OBJS) $(BFDLIB) $(LNK)
 
-test: ghidra_test_dbg
-	./ghidra_test_dbg
+test: decomp_test_dbg
+	./decomp_test_dbg
 
 ghidra_dbg:	$(GHIDRA_DBG_OBJS)
 	$(CXX) $(DBG_CXXFLAGS) $(ADDITIONAL_FLAGS) $(MAKE_STATIC) $(ARCH_TYPE) -o ghidra_dbg $(GHIDRA_DBG_OBJS)

Ghidra/Features/Decompiler/src/decompile/cpp/address.cc

@@ -666,6 +666,8 @@ uintb uintb_negate(uintb in,int4 size)
 uintb sign_extend(uintb in,int4 sizein,int4 sizeout)
 
 {
+  sizein = (sizein < sizeof(uintb)) ? sizein : sizeof(uintb);
+  sizeout = (sizeout < sizeof(uintb)) ? sizeout : sizeof(uintb);
   intb sval = in;
   sval <<= (sizeof(intb) - sizein) * 8;
   uintb res = (uintb)(sval >> (sizeout - sizein) * 8);

Ghidra/Features/Decompiler/src/decompile/cpp/address.hh

@@ -558,7 +558,7 @@ inline intb zero_extend(intb val,int4 bit)
 
 {
   int4 sa = sizeof(intb)*8 - (bit+1);
-  return (intb)((uintb)(val << sa) >> sa);
+  return (intb)(((uintb)val << sa) >> sa);
 }
 
 extern bool signbit_negative(uintb val,int4 size);	///< Return true if the sign-bit is set

Ghidra/Features/Decompiler/src/decompile/cpp/coreaction.cc

@@ -2724,8 +2724,9 @@ int4 ActionSetCasts::apply(Funcdata &data)
       if (opc == CPUI_CAST) continue;
       if (opc == CPUI_PTRADD) {	// Check for PTRADD that no longer fits its pointer
 	int4 sz = (int4)op->getIn(2)->getOffset();
-	TypePointer *ct = (TypePointer *)op->getIn(0)->getHighTypeReadFacing(op);
-	if ((ct->getMetatype() != TYPE_PTR)||(ct->getPtrTo()->getAlignSize() != AddrSpace::addressToByteInt(sz, ct->getWordSize())))
+	Datatype *ct = op->getIn(0)->getHighTypeReadFacing(op);
+	if (ct->getMetatype() != TYPE_PTR ||
+	    ((TypePointer *)ct)->getPtrTo()->getAlignSize() != AddrSpace::addressToByteInt(sz, ((TypePointer *)ct)->getWordSize()))
 	  data.opUndoPtradd(op,true);
       }
       else if (opc == CPUI_PTRSUB) {	// Check for PTRSUB that no longer fits pointer

Ghidra/Features/Decompiler/src/decompile/cpp/fspec.cc

@@ -2901,7 +2901,6 @@ void ProtoModelMerged::decode(Decoder &decoder)
   }
   decoder.closeElement(elemId);
   ((ParamListMerged *)input)->finalize();
-  ((ParamListMerged *)output)->finalize();
 }
 
 void ParameterBasic::setTypeLock(bool val)

Ghidra/Features/Decompiler/src/decompile/cpp/funcdata_varnode.cc

@@ -610,11 +610,17 @@ void Funcdata::setHighLevel(void)
 /// Properties like boolean flags and \e consume bits are copied as appropriate.
 /// \param vn is the existing Varnode
 /// \param newVn is the new Varnode that has its properties set
-/// \param lsbOffset is the significance offset of the new Varnode within the exising
+/// \param lsbOffset is the significance offset of the new Varnode within the existing Varnode
 void Funcdata::transferVarnodeProperties(Varnode *vn,Varnode *newVn,int4 lsbOffset)
 
 {
-  uintb newConsume = (vn->getConsume() >> 8*lsbOffset) & calc_mask(newVn->getSize());
+  uintb newConsume = ~((uintb)0);	// Make sure any bits shifted in above the precision of Varnode::consume are set
+  if (lsbOffset < sizeof(uintb)) {
+    uintb fillBits = 0;
+    if (lsbOffset != 0)
+      fillBits = newConsume << 8*(sizeof(uintb) - lsbOffset);
+    newConsume = ((vn->getConsume() >> 8*lsbOffset) | fillBits) & calc_mask(newVn->getSize());
+  }
 
   uint4 vnFlags = vn->getFlags() & (Varnode::directwrite|Varnode::addrforce);
 

Ghidra/Features/Decompiler/src/decompile/cpp/heritage.cc

@@ -1739,15 +1739,16 @@ void Heritage::splitByRefinement(Varnode *vn,const Address &addr,const vector<in
   uint4 diff = (uint4)spc->wrapOffset(curaddr.getOffset() - addr.getOffset());
   int4 cutsz = refine[diff];
   if (sz <= cutsz) return;	// Already refined
-  while(sz > 0) {
-    Varnode *vn2 = fd->newVarnode(cutsz,curaddr);
-    split.push_back(vn2);
-    curaddr = curaddr + cutsz;
+  split.push_back(fd->newVarnode(cutsz,curaddr));
   sz -= cutsz;
+  while(sz > 0) {
+    curaddr = curaddr + cutsz;
     diff = (uint4)spc->wrapOffset(curaddr.getOffset() - addr.getOffset());
     cutsz = refine[diff];
     if (cutsz > sz)
       cutsz = sz;		// Final piece
+    split.push_back(fd->newVarnode(cutsz,curaddr));
+    sz -= cutsz;
   }
 }
 

Ghidra/Features/Decompiler/src/decompile/cpp/jumptable.cc

@@ -721,8 +721,8 @@ Varnode *GuardRecord::quasiCopy(Varnode *vn,int4 &bitsPreserved)
 {
   bitsPreserved = mostsigbit_set(vn->getNZMask()) + 1;
   if (bitsPreserved == 0) return vn;
-  uintb mask = 1;
-  mask <<= bitsPreserved;
+  uintb mask = 1 << 1;
+  mask <<= (bitsPreserved - 1);
   mask -= 1;
   PcodeOp *op = vn->getDef();
   Varnode *constVn;

Ghidra/Features/Decompiler/src/decompile/cpp/op.cc

@@ -665,8 +665,9 @@ uintb PcodeOp::getNZMaskLocal(bool cliploop) const
     resmask &= fullmask;
     break;
   case CPUI_PIECE:
+    sa = getIn(1)->getSize();
     resmask = getIn(0)->getNZMask();
-    resmask <<= 8*getIn(1)->getSize();
+    resmask = (sa < sizeof(uintb)) ? resmask << 8*sa : 0;
     resmask |= getIn(1)->getNZMask();
     break;
   case CPUI_INT_MULT:

Ghidra/Features/Decompiler/src/decompile/cpp/opbehavior.cc

@@ -746,6 +746,8 @@ uintb OpBehaviorPiece::evaluateBinary(int4 sizeout,int4 sizein,uintb in1,uintb i
 uintb OpBehaviorSubpiece::evaluateBinary(int4 sizeout,int4 sizein,uintb in1,uintb in2) const
 
 {
+  if (in2 >= sizeof(uintb))
+    return 0;
   uintb res = (in1>>(in2*8)) & calc_mask(sizeout);
   return res;
 }

Ghidra/Features/Decompiler/src/decompile/cpp/ruleaction.cc

@@ -896,10 +896,15 @@ int4 RulePullsubMulti::applyOp(PcodeOp *op,Funcdata &data)
   Varnode *outvn = op->getOut();
   if (outvn->isPrecisLo()||outvn->isPrecisHi()) return 0; // Don't pull apart a double precision object
 
-  // Make sure we don't new add SUBPIECE ops that aren't going to cancel in some way
-  int4 branches = mult->numInput();
-  uintb consume = calc_mask(newSize) << 8*minByte;
+  // Make sure we don't add new SUBPIECE ops that aren't going to cancel in some way
+  if (minByte > sizeof(uintb)) return 0;
+  uintb consume;
+  if (minByte < sizeof(uintb))
+    consume = calc_mask(newSize) << 8*minByte;
+  else
+    consume = 0;
   consume = ~consume;			// Check for use of bits outside of what gets truncated later
+  int4 branches = mult->numInput();
   for(int4 i=0;i<branches;++i) {
     Varnode *inVn = mult->getIn(i);
     if ((consume & inVn->getConsume()) != 0) {	// Check if bits not truncated are still used
@@ -961,6 +966,7 @@ int4 RulePullsubIndirect::applyOp(PcodeOp *op,Funcdata &data)
 
   Varnode *vn = op->getIn(0);
   if (!vn->isWritten()) return 0;
+  if (vn->getSize() > sizeof(uintb)) return 0;
   PcodeOp *indir = vn->getDef();
   if (indir->code()!=CPUI_INDIRECT) return 0;
   if (indir->getIn(1)->getSpace()->getType()!=IPTR_IOP) return 0;
@@ -6850,19 +6856,18 @@ void RulePtraddUndo::getOpList(vector<uint4> &oplist) const
 int4 RulePtraddUndo::applyOp(PcodeOp *op,Funcdata &data)
 
 {
-  Varnode *basevn;
-  TypePointer *tp;
-
   if (!data.hasTypeRecoveryStarted()) return 0;
   int4 size = (int4)op->getIn(2)->getOffset(); // Size the PTRADD thinks we are pointing
-  basevn = op->getIn(0);
-  tp = (TypePointer *)basevn->getTypeReadFacing(op);
-  if (tp->getMetatype() == TYPE_PTR)								// Make sure we are still a pointer
+  Varnode *basevn = op->getIn(0);
+  Datatype *dt = basevn->getTypeReadFacing(op);
+  if (dt->getMetatype() == TYPE_PTR) {								// Make sure we are still a pointer
+    TypePointer *tp = (TypePointer *)dt;
     if (tp->getPtrTo()->getAlignSize()==AddrSpace::addressToByteInt(size,tp->getWordSize())) {	// of the correct size
       Varnode *indVn = op->getIn(1);
       if ((!indVn->isConstant()) || (indVn->getOffset() != 0))					// and that index isn't zero
 	return 0;
     }
+  }
 
   data.opUndoPtradd(op,false);
   return 1;
@@ -7299,8 +7304,9 @@ int4 RulePtrsubCharConstant::applyOp(PcodeOp *op,Funcdata &data)
   Varnode *sb = op->getIn(0);
   Datatype *sbType = sb->getTypeReadFacing(op);
   if (sbType->getMetatype() != TYPE_PTR) return 0;
-  TypeSpacebase *sbtype = (TypeSpacebase *)((TypePointer *)sbType)->getPtrTo();
-  if (sbtype->getMetatype() != TYPE_SPACEBASE) return 0;
+  Datatype *dt = ((TypePointer *)sbType)->getPtrTo();
+  if (dt->getMetatype() != TYPE_SPACEBASE) return 0;
+  TypeSpacebase *sbtype = (TypeSpacebase *)dt;
   Varnode *vn1 = op->getIn(1);
   if (!vn1->isConstant()) return 0;
   Varnode *outvn = op->getOut();
@@ -10871,8 +10877,9 @@ int4 RuleExpandLoad::applyOp(PcodeOp *op,Funcdata &data)
     if (defOp->code() == CPUI_INT_ADD && defOp->getIn(1)->isConstant()) {
       addOp = defOp;
       rootPtr = defOp->getIn(0);
-      offset = defOp->getIn(1)->getOffset();
-      if (offset > 16) return 0;		// INT_ADD offset must be small
+      uintb off = defOp->getIn(1)->getOffset();
+      if (off > 16) return 0;		// INT_ADD offset must be small
+      offset = off;
       if (defOp->getOut()->loneDescend() == (PcodeOp *)0) return 0;	// INT_ADD must be used only once
       elType = rootPtr->getTypeReadFacing(defOp);
     }

Ghidra/Features/Decompiler/src/decompile/cpp/slghsymbol.cc

@@ -2415,7 +2415,7 @@ void ContextOp::decode(Decoder &decoder,SleighBase *trans)
   num = decoder.readSignedInteger(sla::ATTRIB_I);
   shift = decoder.readSignedInteger(sla::ATTRIB_SHIFT);
   mask = decoder.readUnsignedInteger(sla::ATTRIB_MASK);
-  patexp = (PatternValue *)PatternExpression::decodeExpression(decoder,trans);
+  patexp = PatternExpression::decodeExpression(decoder,trans);
   patexp->layClaim();
   decoder.closeElement(el);
 }

Ghidra/Features/Decompiler/src/decompile/cpp/stringmanage.cc

@@ -80,7 +80,7 @@ void StringManager::assignStringData(StringData &data,const uint1 *buf,int4 size
     data.byteData.reserve(newSize + 1);
     const uint1 *ptr = (const uint1 *)resString.c_str();
     data.byteData.assign(ptr,ptr+newSize);
-    data.byteData[newSize] = 0;		// Make sure there is a null terminator
+    data.byteData.push_back(0);		// Make sure there is a null terminator
   }
   data.isTruncated = (numChars >= maximumChars);
 }

Ghidra/Features/Decompiler/src/decompile/cpp/subflow.cc

@@ -473,6 +473,7 @@ bool SubvariableFlow::traceForward(ReplaceVarnode *rvn)
       }
       if (!op->getIn(1)->isConstant()) return false; // Dynamic shift
       sa = (int4)op->getIn(1)->getOffset();
+      if (sa >= sizeof(uintb)*8) return false;	// Beyond precision of mask
       newmask = (rvn->mask << sa) & calc_mask( outvn->getSize() );
       if (newmask == 0) break;	// Subvar is cleared, truncate flow
       if (rvn->mask != (newmask >> sa)) return false; // subvar is clipped
@@ -498,9 +499,12 @@ bool SubvariableFlow::traceForward(ReplaceVarnode *rvn)
       }
       if (!op->getIn(1)->isConstant()) return false;
       sa = (int4)op->getIn(1)->getOffset();
+      if (sa >= sizeof(uintb)*8)
+	newmask = 0;
+      else
 	newmask = rvn->mask >> sa;
       if (newmask == 0) {
-	if (op->code()==CPUI_INT_RIGHT) break; // subvar is set to zero, truncate flow
+	if (op->code()==CPUI_INT_RIGHT) break; // subvar does not pass thru, truncate flow
 	return false;
       }
       if (rvn->mask != (newmask << sa)) return false;
@@ -523,6 +527,7 @@ bool SubvariableFlow::traceForward(ReplaceVarnode *rvn)
       break;
     case CPUI_SUBPIECE:
       sa = (int4)op->getIn(1)->getOffset() * 8;
+      if (sa >= sizeof(uintb)*8) break;
       newmask = (rvn->mask >> sa) & calc_mask(outvn->getSize());
       if (newmask == 0) break;	// subvar is set to zero, truncate flow
       if (rvn->mask != (newmask << sa)) {	// Some kind of truncation of the logical value
@@ -725,6 +730,9 @@ bool SubvariableFlow::traceBackward(ReplaceVarnode *rvn)
   case CPUI_INT_LEFT:
     if (!op->getIn(1)->isConstant()) break; // Dynamic shift
     sa = (int4)op->getIn(1)->getOffset();
+    if (sa >= sizeof(uintb)*8)
+      newmask = 0;
+    else
       newmask = rvn->mask >> sa;	// What mask looks like before shift
     if (newmask == 0) {		// Subvariable filled with shifted zero
       rop = createOp(CPUI_COPY,1,rvn);
@@ -744,6 +752,8 @@ bool SubvariableFlow::traceBackward(ReplaceVarnode *rvn)
   case CPUI_INT_RIGHT:
     if (!op->getIn(1)->isConstant()) break; // Dynamic shift
     sa = (int4)op->getIn(1)->getOffset();
+    if (sa >= sizeof(uintb)*8)
+      break;			// Beyond precision of mask
     newmask = (rvn->mask << sa) & calc_mask(op->getIn(0)->getSize());
     if (newmask == 0) {		// Subvariable filled with shifted zero
       rop = createOp(CPUI_COPY,1,rvn);
@@ -758,6 +768,8 @@ bool SubvariableFlow::traceBackward(ReplaceVarnode *rvn)
   case CPUI_INT_SRIGHT:
     if (!op->getIn(1)->isConstant()) break; // Dynamic shift
     sa = (int4)op->getIn(1)->getOffset();
+    if (sa >= sizeof(uintb)*8)
+      break;			// Beyond precision of mask
     newmask = (rvn->mask << sa) & calc_mask(op->getIn(0)->getSize());
     if ((newmask>>sa) != rvn->mask)
       break;			// subvariable is truncated by shift
@@ -1581,8 +1593,11 @@ int4 RuleSubvarSubpiece::applyOp(PcodeOp *op,Funcdata &data)
   Varnode *vn = op->getIn(0);
   Varnode *outvn = op->getOut();
   int4 flowsize = outvn->getSize();
+  int4 sa = op->getIn(1)->getOffset();
+  if (flowsize + sa > sizeof(uintb))	// Mask must fit in precision
+    return 0;
   uintb mask = calc_mask( flowsize );
-  mask <<= 8*((int4)op->getIn(1)->getOffset());
+  mask <<= 8*sa;
   bool aggressive = outvn->isPtrFlow();
   if (!aggressive) {
     if ((vn->getConsume() & mask) != vn->getConsume()) return 0;

Ghidra/Features/Decompiler/src/decompile/cpp/transform.cc

@@ -452,8 +452,14 @@ TransformVar *TransformManager::newSplit(Varnode *vn,const LaneDescription &desc
     int4 bitpos = description.getPosition(i) * 8;
     TransformVar *newVar = &res[i];
     int4 byteSize = description.getSize(i);
-    if (vn->isConstant())
-      newVar->initialize(TransformVar::constant,vn,byteSize * 8,byteSize, (vn->getOffset() >> bitpos) & calc_mask(byteSize));
+    if (vn->isConstant()) {
+      uintb val;
+      if (bitpos < sizeof(uintb)*8)
+	val = (vn->getOffset() >> bitpos) & calc_mask(byteSize);
+      else
+	val = 0;	// Assume bits beyond precision are 0
+      newVar->initialize(TransformVar::constant,vn,byteSize * 8,byteSize, val);
+    }
     else {
       uint4 type = preserveAddress(vn, byteSize * 8, bitpos) ? TransformVar::piece : TransformVar::piece_temp;
       newVar->initialize(type,vn,byteSize * 8, byteSize, bitpos);
@@ -482,8 +488,14 @@ TransformVar *TransformManager::newSplit(Varnode *vn,const LaneDescription &desc
     int4 bitpos = description.getPosition(startLane + i) * 8 - baseBitPos;
     int4 byteSize = description.getSize(startLane + i);
     TransformVar *newVar = &res[i];
-    if (vn->isConstant())
-      newVar->initialize(TransformVar::constant,vn,byteSize * 8, byteSize, (vn->getOffset() >> bitpos) & calc_mask(byteSize));
+    if (vn->isConstant()) {
+      uintb val;
+      if (bitpos < sizeof(uintb)*8)
+	val = (vn->getOffset() >> bitpos) & calc_mask(byteSize);
+      else
+	val = 0;	// Assume bits beyond precision are 0
+      newVar->initialize(TransformVar::constant,vn,byteSize * 8, byteSize, val);
+    }
     else {
       uint4 type = preserveAddress(vn, byteSize * 8, bitpos) ? TransformVar::piece : TransformVar::piece_temp;
       newVar->initialize(type,vn,byteSize * 8, byteSize, bitpos);

Ghidra/Features/Decompiler/src/decompile/cpp/type.cc

@@ -3731,8 +3731,9 @@ void TypeFactory::recalcPointerSubmeta(Datatype *base,sub_metatype sub)
   top.submeta = sub;			// Search on the incorrect submeta
   iter = tree.lower_bound(&top);
   while(iter != tree.end()) {
-    TypePointer *ptr = (TypePointer *)*iter;
-    if (ptr->getMetatype() != TYPE_PTR) break;
+    Datatype *dt = *iter;
+    if (dt->getMetatype() != TYPE_PTR) break;
+    TypePointer *ptr = (TypePointer *)dt;
     if (ptr->ptrto != base) break;
     ++iter;
     if (ptr->submeta == sub) {