From 86ac0e1297155df25d37345159327c045e1d6c56 Mon Sep 17 00:00:00 2001 From: emteere <47253321+emteere@users.noreply.github.com> Date: Mon, 2 Nov 2020 10:02:11 -0500 Subject: [PATCH] GT-2884 code review changes, refactored code to simplify and reduce redundant call --- .../function/ApplyFunctionDataTypesCmd.java | 34 ++++-- .../app/analyzers/FunctionStartAnalyzer.java | 104 ++++++++++-------- 2 files changed, 86 insertions(+), 52 deletions(-) diff --git a/Ghidra/Features/Base/src/main/java/ghidra/app/cmd/function/ApplyFunctionDataTypesCmd.java b/Ghidra/Features/Base/src/main/java/ghidra/app/cmd/function/ApplyFunctionDataTypesCmd.java index 6bef7bc83e..c6d5b54420 100644 --- a/Ghidra/Features/Base/src/main/java/ghidra/app/cmd/function/ApplyFunctionDataTypesCmd.java +++ b/Ghidra/Features/Base/src/main/java/ghidra/app/cmd/function/ApplyFunctionDataTypesCmd.java @@ -265,13 +265,9 @@ public class ApplyFunctionDataTypesCmd extends BackgroundCommand { boolean isValidFunctionStart(TaskMonitor monitor, Address address) { // instruction above falls into this one // could be non-returning function, but we can't tell now - Address addrBefore = address.previous(); - if (addrBefore != null) { - Instruction instrBefore; - instrBefore = program.getListing().getInstructionContaining(addrBefore); - if (instrBefore != null && address.equals(instrBefore.getFallThrough())) { - return false; - } + Instruction instrBefore = getInstructionBefore(address); + if (instrBefore != null && address.equals(instrBefore.getFallThrough())) { + return false; } // check if part of a larger code-block @@ -292,6 +288,30 @@ public class ApplyFunctionDataTypesCmd extends BackgroundCommand { return true; } + /** + * Get the instruction directly before this address, makeing sure it is the + * head instruction in a delayslot + * + * @param address to get instruction before + * @return instruction if found, null otherwise + */ + Instruction getInstructionBefore(Address address) { + Address addrBefore = address.previous(); + Instruction instrBefore = null; + + while (addrBefore != null) { + instrBefore = program.getListing().getInstructionContaining(addrBefore); + if (instrBefore == null) { + break; + } + if (!instrBefore.isInDelaySlot()) { + break; + } + addrBefore = instrBefore.getMinAddress().previous(); + } + return instrBefore; + } + private void applyFunction(Symbol sym, FunctionDefinition fdef) { if (fdef == null) { Msg.info(this, "Multiple function definitions for " + sym.getName() + " at " + diff --git a/Ghidra/Features/BytePatterns/src/main/java/ghidra/app/analyzers/FunctionStartAnalyzer.java b/Ghidra/Features/BytePatterns/src/main/java/ghidra/app/analyzers/FunctionStartAnalyzer.java index 858a149ecd..adec352a07 100644 --- a/Ghidra/Features/BytePatterns/src/main/java/ghidra/app/analyzers/FunctionStartAnalyzer.java +++ b/Ghidra/Features/BytePatterns/src/main/java/ghidra/app/analyzers/FunctionStartAnalyzer.java @@ -365,10 +365,11 @@ public class FunctionStartAnalyzer extends AbstractAnalyzer implements PatternFa // if this place is already in a function, we shouldn't start one if (name.startsWith("func")) { - if (checkAlreadyInFunctionAbove(program, addr)) { + Function funcAbove = getFunctionAbove(program, addr); + if (funcAbove == null) { return false; } - if (!checkForFunctionAbove(program, addr)) { + if (checkAlreadyInFunctionAbove(program, addr, funcAbove)) { return false; } } @@ -405,64 +406,77 @@ public class FunctionStartAnalyzer extends AbstractAnalyzer implements PatternFa return true; } - /** - * Check for an existing function above the addr. Addr should not be in the function. - * @param program prpgram to check in - * @param addr address to check - * @return true if there is an existing function above addr that doesn't contain addr + /* + * Check if address if addr is already part of a function just preceding this address. + * If the address is part of another function that is different than the function right + * above, then the pattern should be applied, because it is most likely a unique function + * that is being used by another function as a shared return. */ - private boolean checkForFunctionAbove(Program program, Address addr) { - // make sure there is an end of function before this one, and addr is not in the function - Function func = null; - Address addrBefore = addr.previous(); - func = program.getFunctionManager().getFunctionContaining(addrBefore); - // no function above - if (func == null) { - return false; - } - // addr is in function above - if (func.getBody().contains(addr)) { - return false; - } - return true; + private boolean checkAlreadyInFunctionAbove(Program program, Address addr) { + Function funcAbove = getFunctionAbove(program, addr); + return checkAlreadyInFunctionAbove(program, addr, funcAbove); } - private boolean checkAlreadyInFunctionAbove(Program program, Address addr) { - // make sure there is an end of function before this one, and if just an instruction, doesn't fall into this one. - Function func = null; + /* + * Check if in a function above + * return true if already in function above, false otherwise even if in another function + */ + private boolean checkAlreadyInFunctionAbove(Program program, Address addr, Function funcAbove) { + // if no funcAbove, make sure an instruction, doesn't fall into this one. Address addrBefore = addr.previous(); if (addrBefore == null) { return false; } - func = program.getFunctionManager().getFunctionContaining(addrBefore); - if (func == null) { - Instruction instr = program.getListing().getInstructionContaining(addrBefore); - if (instr != null && addr.equals(instr.getFallThrough())) { - return true; - } - // check for references to this function, address - ReferenceIterator referencesTo = - program.getReferenceManager().getReferencesTo(addr); - for (Reference reference : referencesTo) { - // someone flows to or reads/writes this location, shouldn't be a start - RefType referenceType = reference.getReferenceType(); - if (referenceType.isData() && - !(referenceType.isRead() || referenceType.isWrite())) { - continue; - } - // any other reference to here is bad, since a function or other flow should - // have created the location + if (funcAbove != null) { + // check if in function right above + Function myfunc = program.getFunctionManager().getFunctionContaining(addr); + if (myfunc != null && myfunc.getEntryPoint().equals(funcAbove.getEntryPoint())) { return true; } + // I could be in a different function, just not one above return false; } - // don't do it if I'm in a function - Function myfunc = program.getFunctionManager().getFunctionContaining(addr); - if (myfunc != null && myfunc.getEntryPoint().equals(func.getEntryPoint())) { + + // no function above, but check for references, that would make this a function + // or references that would imply it is part of another function. + Instruction instr = program.getListing().getInstructionContaining(addrBefore); + if (instr != null && addr.equals(instr.getFallThrough())) { return true; } + // check for references to this function, address + ReferenceIterator referencesTo = + program.getReferenceManager().getReferencesTo(addr); + for (Reference reference : referencesTo) { + // someone flows to or reads/writes this location, shouldn't be a start + RefType referenceType = reference.getReferenceType(); + if (referenceType.isData() && + !(referenceType.isRead() || referenceType.isWrite())) { + continue; + } + // any other reference to here is bad, since a function or other flow should + // have created the location + return true; + } + return false; } + + /** + * Get an existing function right above the addr. + * @param program program to check + * @param addr address to check + * @return true if there is an existing function above addr + */ + private Function getFunctionAbove(Program program, Address addr) { + // make sure there is an end of function before this one, and addr is not in the function + Function func = null; + Address addrBefore = addr.previous(); + if (addrBefore == null) { + return null; + } + func = program.getFunctionManager().getFunctionContaining(addrBefore); + return func; + } void bookmarkAction(Program program, Address addr, Match match) { if (setbookmark) {