From c6f174dffb2f02efec914cf14d2376c53317a167 Mon Sep 17 00:00:00 2001 From: James <49045138+ghidracadabra@users.noreply.github.com> Date: Wed, 11 May 2022 13:23:17 -0400 Subject: [PATCH] GP-2016 addressing code review suggestions GP-2016_short_wide_character_format_strings --- .../string/variadic/FormatStringAnalyzer.java | 62 ++++++++----------- .../string/variadic/PcodeFunctionParser.java | 39 +++++++----- 2 files changed, 52 insertions(+), 49 deletions(-) diff --git a/Ghidra/Features/DecompilerDependent/src/main/java/ghidra/app/plugin/core/string/variadic/FormatStringAnalyzer.java b/Ghidra/Features/DecompilerDependent/src/main/java/ghidra/app/plugin/core/string/variadic/FormatStringAnalyzer.java index a49acf89d0..9a8019a74f 100644 --- a/Ghidra/Features/DecompilerDependent/src/main/java/ghidra/app/plugin/core/string/variadic/FormatStringAnalyzer.java +++ b/Ghidra/Features/DecompilerDependent/src/main/java/ghidra/app/plugin/core/string/variadic/FormatStringAnalyzer.java @@ -99,8 +99,7 @@ public class FormatStringAnalyzer extends AbstractAnalyzer { return true; } - private void run(AddressSetView selection, TaskMonitor monitor) - throws CancelledException { + private void run(AddressSetView selection, TaskMonitor monitor) throws CancelledException { DefinedDataIterator dataIterator = DefinedDataIterator.definedStrings(currentProgram); Map stringsByAddress = new HashMap<>(); @@ -114,7 +113,8 @@ public class FormatStringAnalyzer extends AbstractAnalyzer { FunctionIterator functionIterator = currentProgram.getListing().getFunctions(true); FunctionIterator externalIterator = currentProgram.getListing().getExternalFunctions(); - Iterator programFunctionIterator = IteratorUtils.chainedIterator(functionIterator,externalIterator); + Iterator programFunctionIterator = + IteratorUtils.chainedIterator(functionIterator, externalIterator); Map> namesToParameters = new HashMap<>(); Map namesToReturn = new HashMap<>(); @@ -124,22 +124,21 @@ public class FormatStringAnalyzer extends AbstractAnalyzer { // Find variadic function names and their parameter data types for (Function function : IteratorUtils.asIterable(programFunctionIterator)) { String name = function.getName().strip(); - if (usesVariadicFormatString(function)) { - for (String variadicSubstring : VARIADIC_SUBSTRINGS) { - if (name.contains(variadicSubstring)) { - variadicFunctionNames.add(name); - namesToParameters.put(name, getParameters(function)); - namesToReturn.put(name, function.getReturnType()); - break; - } - } - } + if (usesVariadicFormatString(function)) { + for (String variadicSubstring : VARIADIC_SUBSTRINGS) { + if (name.contains(variadicSubstring)) { + variadicFunctionNames.add(name); + namesToParameters.put(name, getParameters(function)); + namesToReturn.put(name, function.getReturnType()); + break; + } + } + } monitor.checkCanceled(); } Iterator functionsToSearchIterator = selection != null - ? currentProgram.getFunctionManager() - .getFunctionsOverlapping(selection) + ? currentProgram.getFunctionManager().getFunctionsOverlapping(selection) : currentProgram.getFunctionManager().getFunctionsNoStubs(true); // Find functions that call variadic functions @@ -157,14 +156,11 @@ public class FormatStringAnalyzer extends AbstractAnalyzer { } decompile(currentProgram, monitor, stringsByAddress, variadicFunctionNames, - namesToParameters, - namesToReturn, - toDecompile); + namesToParameters, namesToReturn, toDecompile); } private void decompile(Program program, TaskMonitor monitor, - Map stringsByAddress, - Set variadicFunctionNames, + Map stringsByAddress, Set variadicFunctionNames, Map> namesToParameters, Map namesToReturn, Set toDecompile) { @@ -187,11 +183,9 @@ public class FormatStringAnalyzer extends AbstractAnalyzer { } private DecompilerCallback initDecompilerCallback(Program program, - Map stringsByAddress, - Set variadicFuncNames, Map> namesToParameters, - Map namesToReturn) { - return new DecompilerCallback<>(program, - new VariadicSignatureDecompileConfigurer()) { + Map stringsByAddress, Set variadicFuncNames, + Map> namesToParameters, Map namesToReturn) { + return new DecompilerCallback<>(program, new VariadicSignatureDecompileConfigurer()) { @Override public Void process(DecompileResults results, TaskMonitor tMonitor) throws Exception { if (results == null) { @@ -211,8 +205,8 @@ public class FormatStringAnalyzer extends AbstractAnalyzer { pcodeOpASTs.add(pcodeAST); } } - List functionCallDataList = pcodeParser.parseFunctionForCallData( - pcodeOpASTs, stringsByAddress, variadicFuncNames); + List functionCallDataList = pcodeParser + .parseFunctionForCallData(pcodeOpASTs, stringsByAddress, variadicFuncNames); if (functionCallDataList != null && functionCallDataList.size() > 0) { overrideCallList(program, function, functionCallDataList, namesToParameters, namesToReturn); @@ -256,7 +250,7 @@ public class FormatStringAnalyzer extends AbstractAnalyzer { // DecompInterface allows for control of decompilation processes @Override public void configure(DecompInterface decompiler) { - decompiler.toggleCCode(true); // Produce C code + decompiler.toggleCCode(false); //only need syntax tree decompiler.toggleSyntaxTree(true); // Produce syntax tree decompiler.openProgram(currentProgram); decompiler.setSimplificationStyle("normalize"); @@ -266,8 +260,7 @@ public class FormatStringAnalyzer extends AbstractAnalyzer { } } - private ParameterDefinition[] parseParameters(Function function, - Address address, + private ParameterDefinition[] parseParameters(Function function, Address address, String callFunctionName, String formatString, Map> namesToParameters) { @@ -347,8 +340,7 @@ public class FormatStringAnalyzer extends AbstractAnalyzer { private void overrideFunctionCall(Program program, Function function, Address address, String callFunctionName, String formatString, - Map> namesToParameters, - Map namesToReturn) { + Map> namesToParameters, Map namesToReturn) { if (formatString == null) { return; } @@ -361,14 +353,14 @@ public class FormatStringAnalyzer extends AbstractAnalyzer { try { if (createBookmarksEnabled) { BookmarkManager bookmark = program.getBookmarkManager(); - bookmark.setBookmark(address, BookmarkType.ANALYSIS, - "Function Signature Override", + bookmark.setBookmark(address, BookmarkType.ANALYSIS, "Function Signature Override", "Override for call to function " + callFunctionName); } HighFunctionDBUtil.writeOverride(function, address, functionSignature); } catch (InvalidInputException e) { - Msg.error(this, "Error: invalid input given to writeOverride()", e); + Msg.warn(this, + "Error applying override to " + address.toString() + ": " + e.getMessage()); } } diff --git a/Ghidra/Features/DecompilerDependent/src/main/java/ghidra/app/plugin/core/string/variadic/PcodeFunctionParser.java b/Ghidra/Features/DecompilerDependent/src/main/java/ghidra/app/plugin/core/string/variadic/PcodeFunctionParser.java index 573f35a6be..5dd6069e78 100644 --- a/Ghidra/Features/DecompilerDependent/src/main/java/ghidra/app/plugin/core/string/variadic/PcodeFunctionParser.java +++ b/Ghidra/Features/DecompilerDependent/src/main/java/ghidra/app/plugin/core/string/variadic/PcodeFunctionParser.java @@ -19,12 +19,12 @@ import java.util.*; import ghidra.docking.settings.SettingsImpl; import ghidra.program.model.address.Address; -import ghidra.program.model.data.StringDataInstance; -import ghidra.program.model.data.StringDataType; +import ghidra.program.model.data.*; import ghidra.program.model.listing.*; import ghidra.program.model.mem.MemoryBufferImpl; import ghidra.program.model.pcode.PcodeOpAST; import ghidra.program.model.pcode.Varnode; +import ghidra.program.model.symbol.SourceType; /** * Class for parsing functions' Pcode representations and finding variadic @@ -84,7 +84,7 @@ public class PcodeFunctionParser { boolean hasDefinedFormatString = searchForVariadicCallData(ast, addressToCandidateData, functionCallDataList, functionName); if (!hasDefinedFormatString) { - searchForHiddenFormatStrings(ast, functionCallDataList, functionName); + searchForHiddenFormatStrings(ast, functionCallDataList, function); } } } @@ -116,20 +116,28 @@ public class PcodeFunctionParser { // If addrToCandidateData doesn't have format String data for this call // and we are calling a variadic function, parse the String to determine // whether it's a format String. - private void searchForHiddenFormatStrings(PcodeOpAST ast, - List functionCallDataList, String functionName) { + private void searchForHiddenFormatStrings(PcodeOpAST callOp, + List functionCallDataList, Function function) { - Varnode[] inputs = ast.getInputs(); - // Initialize i = 1 to skip first input + Varnode[] inputs = callOp.getInputs(); + // Initialize i = 1 to skip first input, which is the call target for (int i = 1; i < inputs.length; ++i) { Varnode v = inputs[i]; - String formatStringCandidate = findFormatString(v.getAddress()); + Parameter param = function.getParameter(i - 1); + if (param == null || param.getSource().equals(SourceType.DEFAULT)) { + continue; + } + DataType type = param.getDataType(); + if ((type == null) || !(type instanceof Pointer)) { + continue; + } + String formatStringCandidate = findFormatString(v.getAddress(), (Pointer) type); if (formatStringCandidate == null) { continue; } if (formatStringCandidate.contains("%")) { - functionCallDataList.add(new FunctionCallData(ast.getSeqnum().getTarget(), - functionName, formatStringCandidate)); + functionCallDataList.add(new FunctionCallData(callOp.getSeqnum().getTarget(), + function.getName(), formatStringCandidate)); } break; } @@ -145,9 +153,10 @@ public class PcodeFunctionParser { * Looks at bytes at given address and converts to format String * * @param address Address of format String + * @param pointer Pointer "type" of string * @return format String */ - private String findFormatString(Address address) { + private String findFormatString(Address address, Pointer pointer) { if (!address.getAddressSpace().isConstantSpace()) { return null; @@ -159,9 +168,11 @@ public class PcodeFunctionParser { MemoryBufferImpl memoryBuffer = new MemoryBufferImpl(this.program.getMemory(), ramSpaceAddress); - StringDataInstance stringDataInstance = StringDataInstance - .getStringDataInstance(new StringDataType(), memoryBuffer, SettingsImpl.NO_SETTINGS, - BUFFER_LENGTH); + DataType charType = pointer.getDataType(); + //StringDataInstace.getStringDataInstance checks that charType is appropriate + //and returns StringDataInstace.NULL_INSTANCE if not + StringDataInstance stringDataInstance = StringDataInstance.getStringDataInstance(charType, + memoryBuffer, SettingsImpl.NO_SETTINGS, BUFFER_LENGTH); String stringValue = stringDataInstance.getStringValue(); if (stringValue == null) { return null;