From 4a4cb2a1e4bbc620c8fdb14456de032790381f0b Mon Sep 17 00:00:00 2001 From: emteere <47253321+emteere@users.noreply.github.com> Date: Tue, 5 Nov 2019 07:11:52 -0500 Subject: [PATCH 1/4] GT-3260_emteere cache of address set of memory blocks that are marked execute --- .../database/mem/MemoryManagerTest.java | 85 ++++++++++++++++++- .../src/main/java/ghidra/util/Lock.java | 34 +++++--- .../ghidra/app/util/PseudoDisassembler.java | 23 ++--- .../program/database/mem/MemoryMapDB.java | 15 +++- 4 files changed, 123 insertions(+), 34 deletions(-) diff --git a/Ghidra/Features/Base/src/test.slow/java/ghidra/program/database/mem/MemoryManagerTest.java b/Ghidra/Features/Base/src/test.slow/java/ghidra/program/database/mem/MemoryManagerTest.java index 9d04c3b0c4..5873a532ed 100644 --- a/Ghidra/Features/Base/src/test.slow/java/ghidra/program/database/mem/MemoryManagerTest.java +++ b/Ghidra/Features/Base/src/test.slow/java/ghidra/program/database/mem/MemoryManagerTest.java @@ -296,7 +296,6 @@ public class MemoryManagerTest extends AbstractGhidraHeadedIntegrationTest { block2.setSourceName("Test"); assertEquals("Test", block2.getSourceName()); - } @Test @@ -398,6 +397,90 @@ public class MemoryManagerTest extends AbstractGhidraHeadedIntegrationTest { transactionID = program.startTransaction("Test"); } + @Test + public void testMemoryMapExecuteSet() throws Exception { + + AddressSetView executeSet = mem.getExecuteSet(); + assertTrue(executeSet.isEmpty()); + MemoryBlock block1 = createBlock("Test1", addr(100), 100); + executeSet = mem.getExecuteSet(); + assertTrue(executeSet.isEmpty()); + MemoryBlock block2 = createBlock("Test2", addr(300), 100); + executeSet = mem.getExecuteSet(); + assertTrue(executeSet.isEmpty()); + + MemoryBlock block = mem.getBlock("Test1"); + executeSet = mem.getExecuteSet(); + assertTrue(executeSet.isEmpty()); + + block.setExecute(false); + executeSet = mem.getExecuteSet(); + assertTrue(executeSet.isEmpty()); + + block.setExecute(true); + executeSet = mem.getExecuteSet(); + assertTrue(executeSet.isEmpty() != true); + Address start = block.getStart(); + Address end = block.getEnd(); + assertTrue(executeSet.contains(start,end)); + + // non-existent block + block = mem.getBlock("NoExist"); + assertNull(block); + + program.endTransaction(transactionID, true); + transactionID = program.startTransaction("Test"); + + // now exists + mem.getBlock("Test1").setName("NoExist"); + // Test1 no longer exists + block = mem.getBlock("NoExist"); + executeSet = mem.getExecuteSet(); + start = block.getStart(); + end = block.getEnd(); + // should be same block + assertTrue(executeSet.contains(start,end)); + block.setExecute(false); + executeSet = mem.getExecuteSet(); + assertTrue(executeSet.contains(start,end) == false); + + block2.setExecute(true); + Address start2 = block2.getStart(); + Address end2 = block2.getEnd(); + mem.removeBlock(block2, new TaskMonitorAdapter()); + + program.endTransaction(transactionID, true); + + program.undo(); + + transactionID = program.startTransaction("Test"); + + // should be execute set on block2, deleted, then undone + executeSet = mem.getExecuteSet(); + assertTrue(executeSet.contains(start2,end2) == false); + + // undid set execute block should now be contained + block = mem.getBlock("Test1"); + start = block.getStart(); + end = block.getEnd(); + executeSet = mem.getExecuteSet(); + assertTrue(executeSet.contains(start,end)); + + mem.split(block, addr(150)); + block = mem.getBlock("Test1"); + executeSet = mem.getExecuteSet(); + assertTrue(executeSet.isEmpty() != true); + assertTrue(executeSet.contains(block.getStart(), block.getEnd())); + + // remove block that was split, should still be executable memory + start = block.getStart(); + end = block.getEnd(); + mem.removeBlock(block, new TaskMonitorAdapter()); + executeSet = mem.getExecuteSet(); + assertTrue(executeSet.isEmpty() != true); + assertTrue(executeSet.contains(start, end) == false); + } + @Test public void testSave() throws Exception { MemoryBlock block1 = createBlock("Test1", addr(0), 100); diff --git a/Ghidra/Framework/Project/src/main/java/ghidra/util/Lock.java b/Ghidra/Framework/Project/src/main/java/ghidra/util/Lock.java index d271a346d3..ccacb4059b 100644 --- a/Ghidra/Framework/Project/src/main/java/ghidra/util/Lock.java +++ b/Ghidra/Framework/Project/src/main/java/ghidra/util/Lock.java @@ -1,6 +1,5 @@ /* ### * IP: GHIDRA - * REVIEWED: YES * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -17,18 +16,20 @@ package ghidra.util; /** - * Ghidra synchronization lock. This class allows creation of named locks for + * Ghidra synchronization lock. This class allows creation of named locks for * modifying tables in the Ghidra data base. This class also creates an instance - * of a global lock that must first be obtained when synchronizing using multiple - * of the named locks. + * of a global lock that must first be obtained when synchronizing using + * multiple of the named locks. */ public class Lock { private Thread owner; private int cnt = 0; + private int waiting = 0; private String name; /** * Creates an instance of a lock for synchronization within Ghidra. + * * @param name the name of this lock */ public Lock(String name) { @@ -36,8 +37,8 @@ public class Lock { } /** - * Acquire this synchronization lock. - * (i.e. begin synchronizing on this named lock.) + * Acquire this synchronization lock. (i.e. begin synchronizing on this named + * lock.) */ public synchronized void acquire() { Thread currThread = Thread.currentThread(); @@ -47,15 +48,16 @@ public class Lock { cnt = 1; owner = currThread; return; - } - else if (owner == currThread) { + } else if (owner == currThread) { cnt++; return; } try { + waiting++; wait(); - } - catch (InterruptedException e) { + } catch (InterruptedException e) { + } finally { + waiting--; } } } @@ -70,20 +72,24 @@ public class Lock { if (cnt > 0 && (owner == currThread)) { if (--cnt == 0) { owner = null; - notify(); + // This is purely to help sample profiling. If notify() is called the + // sampler can attribute time to the methods calling this erroneously. For some reason + // the visualvm sampler gets a sample more often when notify() is called. + if (waiting != 0) { + notify(); + } } - } - else { + } else { throw new IllegalStateException("Attempted to release an unowned lock: " + name); } } /** * Gets the thread that currently owns the lock. + * * @return the thread that owns the lock or null. */ public Thread getOwner() { return owner; } - } diff --git a/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/app/util/PseudoDisassembler.java b/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/app/util/PseudoDisassembler.java index f19715de00..eecef33e33 100644 --- a/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/app/util/PseudoDisassembler.java +++ b/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/app/util/PseudoDisassembler.java @@ -95,8 +95,6 @@ public class PseudoDisassembler { private boolean respectExecuteFlag = false; - private AddressSetView executeSet; - /** * Create a pseudo disassembler for the given program. */ @@ -111,18 +109,6 @@ public class PseudoDisassembler { this.programContext = program.getProgramContext(); } - - /** - * @return cached addressSet of executable memory blocks - */ - private AddressSetView getExecuteSet() { - if (executeSet != null) { - return executeSet; - } - - executeSet = memory.getExecuteSet(); - return executeSet; - } /** * Set the maximum number of instructions to check @@ -617,6 +603,7 @@ public class PseudoDisassembler { boolean allowExistingInstructions, boolean mustTerminate) { AddressSet body = new AddressSet(); AddressSet instrStarts = new AddressSet(); + AddressSetView execSet = memory.getExecuteSet(); if (hasLowBitCodeModeInAddrValues(program)) { entryPoint = setTargeContextForDisassembly(procContext, entryPoint); @@ -801,7 +788,6 @@ public class PseudoDisassembler { } } // if respecting execute flag on memory, test to make sure we did flow into non-execute memory - AddressSetView execSet = getExecuteSet(); if (respectExecuteFlag && !execSet.isEmpty() && !execSet.contains(flows[j])) { if (!flows[j].isExternalAddress()) { MemoryBlock block = memory.getBlock(flows[j]); @@ -902,8 +888,8 @@ public class PseudoDisassembler { } // check that body does not wander into non-executable memory - AddressSetView execSet = getExecuteSet(); - if (respectExecuteFlag && !execSet.isEmpty() && !body.subtract(execSet).isEmpty()) { + AddressSetView execSet = memory.getExecuteSet(); + if (respectExecuteFlag && !execSet.isEmpty() && !execSet.contains(body)) { return false; } @@ -914,8 +900,9 @@ public class PseudoDisassembler { return false; } + boolean canHaveOffcutEntry = hasLowBitCodeModeInAddrValues(program); AddressSet strictlyBody = body.subtract(starts); - if (hasLowBitCodeModeInAddrValues(program)) { + if (canHaveOffcutEntry) { strictlyBody.deleteRange(entry, entry.add(1)); } AddressIterator addrIter = diff --git a/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/mem/MemoryMapDB.java b/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/mem/MemoryMapDB.java index 6073315e5d..e0afc13d06 100644 --- a/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/mem/MemoryMapDB.java +++ b/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/mem/MemoryMapDB.java @@ -57,6 +57,8 @@ public class MemoryMapDB implements Memory, ManagerDB, LiveMemoryListener { private AddressSet addrSet = new AddressSet(); private AddressSet initializedLoadedAddrSet = new AddressSet(); private AddressSet allInitializedAddrSet = new AddressSet(); + private AddressSetView executeSet = new AddressSet(); + private MemoryBlock lastBlock;// the last accessed block private LiveMemoryHandler liveMemory; @@ -187,6 +189,7 @@ public class MemoryMapDB implements Memory, ManagerDB, LiveMemoryListener { blocks = newBlocks; addrMap.memoryMapChanged(this); nameBlockMap = new HashMap<>(); + executeSet = null; } public void setLanguage(Language newLanguage) { @@ -396,6 +399,12 @@ public class MemoryMapDB implements Memory, ManagerDB, LiveMemoryListener { // name could have changed nameBlockMap = new HashMap<>(); + + // execute state could have changed. check if in set and shouldn't be or vice/versa + if (executeSet != null && executeSet.contains(block.getStart(), block.getEnd()) != block.isExecute()) { + // don't regenerate now, do lazily later if needed + executeSet = null; + } } void fireBytesChanged(Address addr, int count) { @@ -1972,13 +1981,17 @@ public class MemoryMapDB implements Memory, ManagerDB, LiveMemoryListener { */ @Override public AddressSetView getExecuteSet() { + if (executeSet != null) { + return executeSet; + } AddressSet set = new AddressSet(); for (MemoryBlock block : blocks) { if (block.isExecute()) { set.addRange(block.getStart(), block.getEnd()); } } - return set; + executeSet = new AddressSetViewAdapter(set); + return executeSet; } @Override From 40a7425b3c59f3eaf876a7dac4387da3e03393fc Mon Sep 17 00:00:00 2001 From: emteere <47253321+emteere@users.noreply.github.com> Date: Tue, 5 Nov 2019 08:19:09 -0500 Subject: [PATCH 2/4] GT-3260_emteere removing lock profiling change --- .../src/main/java/ghidra/util/Lock.java | 34 ++++++++----------- 1 file changed, 14 insertions(+), 20 deletions(-) diff --git a/Ghidra/Framework/Project/src/main/java/ghidra/util/Lock.java b/Ghidra/Framework/Project/src/main/java/ghidra/util/Lock.java index ccacb4059b..d271a346d3 100644 --- a/Ghidra/Framework/Project/src/main/java/ghidra/util/Lock.java +++ b/Ghidra/Framework/Project/src/main/java/ghidra/util/Lock.java @@ -1,5 +1,6 @@ /* ### * IP: GHIDRA + * REVIEWED: YES * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -16,20 +17,18 @@ package ghidra.util; /** - * Ghidra synchronization lock. This class allows creation of named locks for + * Ghidra synchronization lock. This class allows creation of named locks for * modifying tables in the Ghidra data base. This class also creates an instance - * of a global lock that must first be obtained when synchronizing using - * multiple of the named locks. + * of a global lock that must first be obtained when synchronizing using multiple + * of the named locks. */ public class Lock { private Thread owner; private int cnt = 0; - private int waiting = 0; private String name; /** * Creates an instance of a lock for synchronization within Ghidra. - * * @param name the name of this lock */ public Lock(String name) { @@ -37,8 +36,8 @@ public class Lock { } /** - * Acquire this synchronization lock. (i.e. begin synchronizing on this named - * lock.) + * Acquire this synchronization lock. + * (i.e. begin synchronizing on this named lock.) */ public synchronized void acquire() { Thread currThread = Thread.currentThread(); @@ -48,16 +47,15 @@ public class Lock { cnt = 1; owner = currThread; return; - } else if (owner == currThread) { + } + else if (owner == currThread) { cnt++; return; } try { - waiting++; wait(); - } catch (InterruptedException e) { - } finally { - waiting--; + } + catch (InterruptedException e) { } } } @@ -72,24 +70,20 @@ public class Lock { if (cnt > 0 && (owner == currThread)) { if (--cnt == 0) { owner = null; - // This is purely to help sample profiling. If notify() is called the - // sampler can attribute time to the methods calling this erroneously. For some reason - // the visualvm sampler gets a sample more often when notify() is called. - if (waiting != 0) { - notify(); - } + notify(); } - } else { + } + else { throw new IllegalStateException("Attempted to release an unowned lock: " + name); } } /** * Gets the thread that currently owns the lock. - * * @return the thread that owns the lock or null. */ public Thread getOwner() { return owner; } + } From 652e689846e196249d9771448b4444ee23a04cde Mon Sep 17 00:00:00 2001 From: emteere <47253321+emteere@users.noreply.github.com> Date: Fri, 8 Nov 2019 11:30:48 -0500 Subject: [PATCH 3/4] GT-3260_emteere changes from code-review --- .../program/database/mem/MemoryMapDB.java | 52 ++++++++++++------- 1 file changed, 32 insertions(+), 20 deletions(-) diff --git a/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/mem/MemoryMapDB.java b/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/mem/MemoryMapDB.java index e0afc13d06..638cd9d2e7 100644 --- a/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/mem/MemoryMapDB.java +++ b/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/mem/MemoryMapDB.java @@ -57,13 +57,13 @@ public class MemoryMapDB implements Memory, ManagerDB, LiveMemoryListener { private AddressSet addrSet = new AddressSet(); private AddressSet initializedLoadedAddrSet = new AddressSet(); private AddressSet allInitializedAddrSet = new AddressSet(); - private AddressSetView executeSet = new AddressSet(); + private AddressSetView executeSet = null; private MemoryBlock lastBlock;// the last accessed block private LiveMemoryHandler liveMemory; - + // lazy hashmap of block names to blocks, must be reloaded if blocks are removed or added - private HashMap nameBlockMap = new HashMap(); + private HashMap nameBlockMap = new HashMap(); private final static MemoryBlock NoBlock = new MemoryBlockStub(); // placeholder for no block, not given out Lock lock; @@ -251,7 +251,7 @@ public class MemoryMapDB implements Memory, ManagerDB, LiveMemoryListener { @Override public AddressSetView getAllInitializedAddressSet() { - return allInitializedAddrSet; + return new AddressSetViewAdapter(allInitializedAddrSet); } /** @@ -262,7 +262,7 @@ public class MemoryMapDB implements Memory, ManagerDB, LiveMemoryListener { if (liveMemory != null) { return this;//all memory is initialized! } - return initializedLoadedAddrSet; + return new AddressSetViewAdapter(initializedLoadedAddrSet); } void checkMemoryWrite(MemoryBlockDB block, Address start, long length) @@ -396,15 +396,12 @@ public class MemoryMapDB implements Memory, ManagerDB, LiveMemoryListener { if (program != null) { program.setChanged(ChangeManager.DOCR_MEMORY_BLOCK_CHANGED, block, null); } - + // name could have changed nameBlockMap = new HashMap<>(); - - // execute state could have changed. check if in set and shouldn't be or vice/versa - if (executeSet != null && executeSet.contains(block.getStart(), block.getEnd()) != block.isExecute()) { - // don't regenerate now, do lazily later if needed - executeSet = null; - } + + // don't regenerate now, do lazily later if needed + executeSet = null; } void fireBytesChanged(Address addr, int count) { @@ -1981,17 +1978,32 @@ public class MemoryMapDB implements Memory, ManagerDB, LiveMemoryListener { */ @Override public AddressSetView getExecuteSet() { - if (executeSet != null) { + AddressSetView set = executeSet; + + if (set == null) { + set = computeExecuteSet(); + } + return set; + } + + /** + * @return executable address set + */ + private AddressSetView computeExecuteSet() { + lock.acquire(); + try { + AddressSet set = new AddressSet(); + for (MemoryBlock block : blocks) { + if (block.isExecute()) { + set.addRange(block.getStart(), block.getEnd()); + } + } + executeSet = new AddressSetViewAdapter(set); return executeSet; } - AddressSet set = new AddressSet(); - for (MemoryBlock block : blocks) { - if (block.isExecute()) { - set.addRange(block.getStart(), block.getEnd()); - } + finally { + lock.release(); } - executeSet = new AddressSetViewAdapter(set); - return executeSet; } @Override From 9edca9afec0cc75c9c67c580a08f937e6ca57884 Mon Sep 17 00:00:00 2001 From: Ryan Kurtz Date: Fri, 15 Nov 2019 08:50:46 -0500 Subject: [PATCH 4/4] GT-3261: Fixing possible IllegalArgumentException in sort comparator. Integer subtraction could result in overflow. --- .../bin/format/macho/commands/SymbolTableCommand.java | 8 +++++--- .../bin/format/macho/dyld/DyldCacheLocalSymbolsInfo.java | 8 +++++--- 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/Ghidra/Features/Base/src/main/java/ghidra/app/util/bin/format/macho/commands/SymbolTableCommand.java b/Ghidra/Features/Base/src/main/java/ghidra/app/util/bin/format/macho/commands/SymbolTableCommand.java index 4ffceafaf0..5bdca403cc 100644 --- a/Ghidra/Features/Base/src/main/java/ghidra/app/util/bin/format/macho/commands/SymbolTableCommand.java +++ b/Ghidra/Features/Base/src/main/java/ghidra/app/util/bin/format/macho/commands/SymbolTableCommand.java @@ -84,9 +84,11 @@ public class SymbolTableCommand extends LoadCommand { nlistList.add(NList.createNList(reader, is32bit)); } // sort the entries by the index in the string table, so don't jump around reading - List sortedList = nlistList.stream().sorted( - (o1, o2) -> o1.getStringTableIndex() - o2.getStringTableIndex()).collect( - Collectors.toList()); + List sortedList = nlistList + .stream() + .sorted((o1, o2) -> Integer.valueOf(o1.getStringTableIndex()) + .compareTo(Integer.valueOf(o2.getStringTableIndex()))) + .collect(Collectors.toList()); // initialize the NList strings from string table long stringTableOffset = stroff; diff --git a/Ghidra/Features/Base/src/main/java/ghidra/app/util/bin/format/macho/dyld/DyldCacheLocalSymbolsInfo.java b/Ghidra/Features/Base/src/main/java/ghidra/app/util/bin/format/macho/dyld/DyldCacheLocalSymbolsInfo.java index c2bf823f48..9811e8c6f6 100644 --- a/Ghidra/Features/Base/src/main/java/ghidra/app/util/bin/format/macho/dyld/DyldCacheLocalSymbolsInfo.java +++ b/Ghidra/Features/Base/src/main/java/ghidra/app/util/bin/format/macho/dyld/DyldCacheLocalSymbolsInfo.java @@ -159,9 +159,11 @@ public class DyldCacheLocalSymbolsInfo implements StructConverter { monitor.incrementProgress(1); } // sort the entries by the index in the string table, so don't jump around reading - List sortedList = nlistList.stream().sorted( - (o1, o2) -> o1.getStringTableIndex() - o2.getStringTableIndex()).collect( - Collectors.toList()); + List sortedList = nlistList + .stream() + .sorted((o1, o2) -> Integer.valueOf(o1.getStringTableIndex()) + .compareTo(Integer.valueOf(o2.getStringTableIndex()))) + .collect(Collectors.toList()); // initialize the NList strings from string table long stringTableOffset = startIndex + stringsOffset;