GP-4514 Add check preventing multiple unlabeled switch targets

Ghidra/Features/Decompiler/certification.manifest

@@ -28,6 +28,7 @@ src/decompile/datatests/forloop_loaditer.xml||GHIDRA||||END|
 src/decompile/datatests/forloop_thruspecial.xml||GHIDRA||||END|
 src/decompile/datatests/forloop_varused.xml||GHIDRA||||END|
 src/decompile/datatests/forloop_withskip.xml||GHIDRA||||END|
+src/decompile/datatests/ifswitch.xml||GHIDRA||||END|
 src/decompile/datatests/impliedfield.xml||GHIDRA||||END|
 src/decompile/datatests/indproto.xml||GHIDRA||||END|
 src/decompile/datatests/injectoverride.xml||GHIDRA||||END|

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

@@ -2613,30 +2613,47 @@ void BlockBasic::printRaw(ostream &s) const
   }
 }
 
-/// \brief Check if there is meaningful activity between two branch instructions
+/// \brief Check for values created in \b this block that flow outside the block.
 ///
-/// The first branch is assumed to be a CBRANCH one edge of which flows into
-/// the other branch. The flow can be through 1 or 2 blocks.  If either block
-/// performs an operation other than MULTIEQUAL, INDIRECT (or the branch), then
-/// return \b false.
-/// \param first is the CBRANCH operation
-/// \param path is the index of the edge to follow to the other branch
-/// \param last is the other branch operation
-/// \return \b true if there is no meaningful activity
-bool BlockBasic::noInterveningStatement(PcodeOp *first,int4 path,PcodeOp *last)
+/// The block can calculate a value for a BRANCHIND or CBRANCH and can copy values and this method will still
+/// return \b true.  But calculating any value used outside the block, writing to an addressable location,
+/// or performing a CALL or STORE causes the method to return \b false.
+/// \return \b true if no value is created that can be used outside of the block
+bool BlockBasic::noInterveningStatement(void) const
 
 {
-  BlockBasic *curbl = (BlockBasic *)first->getParent()->getOut(path);
-  for(int4 i=0;i<2;++i) {
-    if (!curbl->hasOnlyMarkers()) return false;
-    if (curbl != last->getParent()) {
-      if (curbl->sizeOut() != 1) return false; // Intervening conditional branch
+  list<PcodeOp *>::const_iterator iter;
+  const PcodeOp *bop;
+  OpCode opc;
+
+  for(iter=op.begin();iter!=op.end();++iter) {
+    bop = *iter;
+    if (bop->isMarker()) continue;
+    if (bop->isBranch()) continue;
+    if (bop->getEvalType() == PcodeOp::special) {
+      if (bop->isCall())
+	return false;
+      opc = bop->code();
+      if (opc == CPUI_STORE || opc == CPUI_NEW)
+	return false;
+    }
+    else {
+      opc = bop->code();
+      if (opc == CPUI_COPY || opc == CPUI_SUBPIECE)
+	continue;
+    }
+    const Varnode *outvn = bop->getOut();
+    if (outvn->isAddrTied())
+      return false;
+    list<PcodeOp *>::const_iterator iter = outvn->beginDescend();
+    while(iter!=outvn->endDescend()) {
+      PcodeOp *op = *iter;
+      if (op->getParent() != this)
+	return false;
+      ++iter;
     }
-    else
-      return true;
-    curbl = (BlockBasic *)curbl->getOut(0);
   }
-  return false;
+  return true;
 }
 
 /// If there exists a CPUI_MULTIEQUAL PcodeOp in \b this basic block that takes the given exact list of Varnodes

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

@@ -489,7 +489,7 @@ public:
   list<PcodeOp *>::const_iterator beginOp(void) const { return op.begin(); }	///< Return an iterator to the beginning of the PcodeOps
   list<PcodeOp *>::const_iterator endOp(void) const { return op.end(); }	///< Return an iterator to the end of the PcodeOps
   bool emptyOp(void) const { return op.empty(); }		///< Return \b true if \b block contains no operations
-  static bool noInterveningStatement(PcodeOp *first,int4 path,PcodeOp *last);
+  bool noInterveningStatement(void) const;
   PcodeOp *findMultiequal(const vector<Varnode *> &varArray);		///< Find MULTIEQUAL with given inputs
   static bool liftVerifyUnroll(vector<Varnode *> &varArray,int4 slot);	///< Verify given Varnodes are defined with same PcodeOp
 };

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

@@ -31,6 +31,8 @@ ElementId ELEM_NORMADDR = ElementId("normaddr",215);
 ElementId ELEM_NORMHASH = ElementId("normhash",216);
 ElementId ELEM_STARTVAL = ElementId("startval",217);
 
+const uint8 JumpValues::NO_LABEL = 0xBAD1ABE1BAD1ABE1;
+
 /// \param encoder is the stream encoder
 void LoadTable::encode(Encoder &encoder) const
 
@@ -1355,42 +1357,38 @@ bool JumpBasic::foldInOneGuard(Funcdata *fd,GuardRecord &guard,JumpTable *jump)
 
 {
   PcodeOp *cbranch = guard.getBranch();
-  int4 indpath = guard.getPath();	// Get stored path to indirect block
   BlockBasic *cbranchblock = cbranch->getParent();
-  if (cbranchblock->getFlipPath()) // Based on whether out branches have been flipped
-    indpath = 1 - indpath;	// get actual path to indirect block
-  BlockBasic *guardtarget = (BlockBasic *)cbranchblock->getOut(1-indpath);
-  bool change = false;
-  int4 pos;
-
   // Its possible the guard branch has been converted between the switch recovery and now
   if (cbranchblock->sizeOut() != 2) return false; // In which case, we can't fold it in
+  int4 indpath = guard.getPath();	// Get stored path to indirect block
+  if (cbranchblock->getFlipPath()) // Based on whether out branches have been flipped
+    indpath = 1 - indpath;	// get actual path to indirect block
   BlockBasic *switchbl = jump->getIndirectOp()->getParent();
+  if (cbranchblock->getOut(indpath) != switchbl) // Guard must go directly into switch block
+    return false;
+  BlockBasic *guardtarget = (BlockBasic *)cbranchblock->getOut(1-indpath);
+  int4 pos;
+
   for(pos=0;pos<switchbl->sizeOut();++pos)
     if (switchbl->getOut(pos) == guardtarget) break;
+  if (jump->hasFoldedDefault() && jump->getDefaultBlock() != pos)	// There can be only one folded target
+    return false;
+
+  if (!switchbl->noInterveningStatement())
+    return false;
   if (pos == switchbl->sizeOut()) {
-    if (BlockBasic::noInterveningStatement(cbranch,indpath,switchbl->lastOp())) {
-      // Adjust tables and control flow graph
-      // for new jumptable destination
-      jump->addBlockToSwitch(guardtarget,0xBAD1ABE1);
-      jump->setLastAsMostCommon();
-      fd->pushBranch(cbranchblock,1-indpath,switchbl);
-      guard.clear();
-      change = true;
-    }
+    jump->addBlockToSwitch(guardtarget,JumpValues::NO_LABEL);	// Add new destination to table without a label
+    jump->setLastAsDefault();			// treating it as either the default case or an exit
+    fd->pushBranch(cbranchblock,1-indpath,switchbl);	// Turn branch target into target of the switch instead
   }
   else {
-    // We should probably check that there are no intervening
-    // statements between the guard and the switch. But the
-    // fact that the guard target is also a switch target
-    // is a good indicator that there are none
     uintb val = ((indpath==0)!=(cbranch->isBooleanFlip())) ? 0 : 1;
     fd->opSetInput(cbranch,fd->newConstant(cbranch->getIn(0)->getSize(),val),1);
     jump->setDefaultBlock(pos);	// A guard branch generally targets the default case
-    guard.clear();
-    change = true;
   }
-  return change;
+  jump->setFoldedDefault();	// Mark that the default branch has been folded (and cannot take a label)
+  guard.clear();
+  return true;
 }
 
 JumpBasic::~JumpBasic(void)
@@ -1504,12 +1502,12 @@ void JumpBasic::buildLabels(Funcdata *fd,vector<Address> &addresstable,vector<ui
       try {
 	switchval = backup2Switch(fd,val,normalvn,switchvn);		// Do reverse emulation to get original switch value
       } catch(EvaluationError &err) {
-	switchval = 0xBAD1ABE1;
+	switchval = JumpValues::NO_LABEL;
 	needswarning = 2;
       }
     }
     else
-      switchval = 0xBAD1ABE1;	// If can't reverse, hopefully this is the default or exit, otherwise give "badlabel"
+      switchval = JumpValues::NO_LABEL;	// If can't reverse, hopefully this is the default or exit
     if (needswarning==1)
       fd->warning("This code block may not be properly labeled as switch case",addresstable[label.size()]);
     else if (needswarning==2)
@@ -1524,7 +1522,7 @@ void JumpBasic::buildLabels(Funcdata *fd,vector<Address> &addresstable,vector<ui
 
   while(label.size() < addresstable.size()) {
     fd->warning("Bad switch case",addresstable[label.size()]);
-    label.push_back(0xBAD1ABE1);
+    label.push_back(JumpValues::NO_LABEL);
   }
 }
 
@@ -1628,7 +1626,7 @@ bool JumpBasic2::foldInOneGuard(Funcdata *fd,GuardRecord &guard,JumpTable *jump)
   // So we don't make any special mods, in case there are extra statements in these blocks
 
   // The final block in the table is the single value produced by the model2 guard
-  jump->setLastAsMostCommon();	// It should be the default block
+  jump->setLastAsDefault();	// It should be the default block
   guard.clear();		// Mark that we are folded
   return true;
 }
@@ -1977,7 +1975,7 @@ void JumpBasicOverride::buildLabels(Funcdata *fd,vector<Address> &addresstable,v
     try {
       addr = backup2Switch(fd,values[i],normalvn,switchvn);
     } catch(EvaluationError &err) {
-      addr = 0xBAD1ABE1;
+      addr = JumpValues::NO_LABEL;
     }
     label.push_back(addr);
     if (label.size() >= addresstable.size()) break; // This should never happen
@@ -1985,7 +1983,7 @@ void JumpBasicOverride::buildLabels(Funcdata *fd,vector<Address> &addresstable,v
 
   while(label.size() < addresstable.size()) {
     fd->warning("Bad switch case",addresstable[label.size()]); // This should never happen
-    label.push_back(0xBAD1ABE1);
+    label.push_back(JumpValues::NO_LABEL);
   }
 }
 
@@ -2170,7 +2168,7 @@ void JumpAssisted::buildLabels(Funcdata *fd,vector<Address> &addresstable,vector
       label.push_back(output);
     }
   }
-  label.push_back(0xBAD1ABE1);		// Add fake label to match the defaultAddress
+  label.push_back(JumpValues::NO_LABEL);	// Add fake label to match the defaultAddress
 }
 
 Varnode *JumpAssisted::foldInNormalization(Funcdata *fd,PcodeOp *indop)
@@ -2192,7 +2190,7 @@ bool JumpAssisted::foldInGuards(Funcdata *fd,JumpTable *jump)
 
 {
   int4 origVal = jump->getDefaultBlock();
-  jump->setLastAsMostCommon();			// Default case is always the last block
+  jump->setLastAsDefault();			// Default case is always the last block
   return (origVal != jump->getDefaultBlock());
 }
 
@@ -2348,6 +2346,7 @@ JumpTable::JumpTable(Architecture *g,Address ad)
   maxext = 1;
   recoverystage = 0;
   collectloads = false;
+  defaultIsFolded = false;
 }
 
 /// This is a partial clone of another jump-table. Objects that are specific
@@ -2368,6 +2367,7 @@ JumpTable::JumpTable(const JumpTable *op2)
   maxext = op2->maxext;
   recoverystage = op2->recoverystage;
   collectloads = op2->collectloads;
+  defaultIsFolded = false;
 				// We just clone the addresses themselves
   addresstable = op2->addresstable;
   loadpoints = op2->loadpoints;
@@ -2453,7 +2453,7 @@ int4 JumpTable::getIndexByBlock(const FlowBlock *bl,int4 i) const
   throw LowlevelError("Could not get jumptable index for block");
 }
 
-void JumpTable::setLastAsMostCommon(void)
+void JumpTable::setLastAsDefault(void)
 
 {
   defaultBlock = lastBlock;
@@ -2742,7 +2742,7 @@ void JumpTable::encode(Encoder &encoder) const
     if (spc != (AddrSpace *)0)
       spc->encodeAttributes(encoder,off);
     if (i<label.size()) {
-      if (label[i] != 0xBAD1ABE1)
+      if (label[i] != JumpValues::NO_LABEL)
 	encoder.writeUnsignedInteger(ATTRIB_LABEL, label[i]);
     }
     encoder.closeElement(ELEM_DEST);
@@ -2802,7 +2802,7 @@ void JumpTable::decode(Decoder &decoder)
 
   if (label.size()!=0) {
     while(label.size() < addresstable.size())
-      label.push_back(0xBAD1ABE1);
+      label.push_back(JumpValues::NO_LABEL);
   }
 }
 

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

@@ -184,6 +184,7 @@ public:
   virtual PcodeOp *getStartOp(void) const=0;		///< Get the PcodeOp associated with the current value
   virtual bool isReversible(void) const=0;	///< Return \b true if the current value can be reversed to get a label
   virtual JumpValues *clone(void) const=0;	///< Clone \b this iterator
+  static const uint8 NO_LABEL;			///< Jump-table label reserved to indicate \e no \e label
 };
 
 /// \brief single entry switch variable that can take a range of values
@@ -533,10 +534,12 @@ public:
 
 /// \brief A map from values to control-flow targets within a function
 ///
-/// A JumpTable is attached to a specific CPUI_BRANCHIND and encapsulates all
-/// the information necessary to model the indirect jump as a \e switch statement.
-/// It knows how to map from specific switch variable values to the destination
-/// \e case block and how to label the value.
+/// A JumpTable is attached to a specific CPUI_BRANCHIND and encapsulates all the information necessary
+/// to model the indirect jump as a \e switch statement. It knows how to map from specific switch variable
+/// values to the destination \e case block and how to label the value.  The table also establishes a
+/// \e default target which is either
+///   - the \e default case of the switch or
+///   - the exit point of the switch
 class JumpTable {
 public:
   /// \brief Recovery status for a specific JumpTable
@@ -574,6 +577,7 @@ private:
   uint4 maxext;			///< Maximum extensions to normalize
   int4 recoverystage;		///< 0=no stages recovered, 1=additional stage needed, 2=complete
   bool collectloads;		///< Set to \b true if information about in-memory model data is/should be collected
+  bool defaultIsFolded;		///< The \e default block is the target of a folded CBRANCH (and cannot have a label)
   void recoverModel(Funcdata *fd);	///< Attempt recovery of the jump-table model
   void trivialSwitchOver(void);	///< Switch \b this table over to a trivial model
   void sanityCheck(Funcdata *fd,vector<int4> *loadpoints);	///< Perform sanity check on recovered address targets
@@ -600,9 +604,11 @@ public:
   int4 numIndicesByBlock(const FlowBlock *bl) const;
   int4 getIndexByBlock(const FlowBlock *bl,int4 i) const;
   Address getAddressByIndex(int4 i) const { return addresstable[i]; }	///< Get the i-th address table entry
-  void setLastAsMostCommon(void);		///< Set the most common jump-table target to be the last address in the table
+  void setLastAsDefault(void);		///< Set the \e default jump-table target to be the last address in the table
   void setDefaultBlock(int4 bl) { defaultBlock = bl; }		///< Set out-edge of the switch destination considered to be \e default
   void setLoadCollect(bool val) { collectloads = val; }		///< Set whether LOAD records should be collected
+  void setFoldedDefault(void) { defaultIsFolded = true; }	///< Mark that the \e default block is a folded CBRANCH target
+  bool hasFoldedDefault(void) const { return defaultIsFolded; }	///< Return \b true if the \e default block is a folded CBRANCH target
   void addBlockToSwitch(BlockBasic *bl,uintb lab);		///< Force a given basic-block to be a switch destination
   void switchOver(const FlowInfo &flow);				///< Convert absolute addresses to block indices
   uintb getLabelByIndex(int4 index) const { return label[index]; }	///< Given a \e case index, get its label

Ghidra/Features/Decompiler/src/decompile/datatests/ifswitch.xml

@@ -0,0 +1,44 @@
+<decompilertest>
+<binaryimage arch="x86:LE:64:default:gcc">
+<!-- Switch statement nested in an if that also tests the switch variable -->
+<bytechunk space="ram" offset="0x100000" readonly="true">
+f30f1efa83ff637e0769c7e8030000c3
+83ff147727488d0d3800000089fa4863
+04914801c83effe0b806000000c3b80a
+000000c38d04fd00000000c38d47f3c3
+4863c7c1ff1f4869c06766666648c1f8
+2129f8c3daffffffecffffffd4ffffff
+e0ffffffe8ffffffd4ffffffe0ffffff
+e0ffffffe0ffffffe8ffffffecffffff
+e8ffffffe8ffffffe8ffffffe8ffffff
+e8ffffffe8ffffffe8ffffffe8ffffff
+d4ffffffecffffff04000000
+</bytechunk>
+<symbol space="ram" offset="0x100000" name="testval"/>
+</binaryimage>
+<script>
+  <com>lo fu testval</com>
+  <com>decompile</com>
+  <com>print C</com>
+  <com>quit</com>
+</script>
+<stringmatch name="If/Switch #1" min="1" max="1">default:
+                                                 return param_1 \+ -0xd;</stringmatch>
+<stringmatch name="If/Switch #2" min="1" max="1">if \(99 &lt; param_1\) \{
+						 return param_1 \* 1000;</stringmatch>
+<stringmatch name="If/Switch #3" min="1" max="1">case 0:
+						 return 10;</stringmatch>
+<stringmatch name="If/Switch #4" min="1" max="1">case 1:
+						 case 10:
+						 case 0x14:
+						 return param_1 / 5;</stringmatch>
+<stringmatch name="If/Switch #5" min="1" max="1">case 2:
+						 case 5:
+						 case 0x13:
+						 return 6;</stringmatch>
+<stringmatch name="If/Switch #6" min="1" max="1">case 3:
+						 case 6:
+						 case 7:
+						 case 8:
+						 return param_1 \* 8;</stringmatch>
+</decompilertest>