Merge pull request #558 from timll/fixFullPathBuilder

Refine path building from cache and fix backward full path reconstruction

Author: StevenArzt

soot-infoflow/src/soot/jimple/infoflow/data/AccessPathFactory.java

@@ -2,6 +2,7 @@
 
 import java.util.Arrays;
 import java.util.Collection;
+import java.util.Collections;
 import java.util.Set;
 
 import org.slf4j.Logger;
@@ -417,7 +418,7 @@ public AccessPath createAccessPath(Value val, Type valType, AccessPathFragment[]
 
 	private void registerBase(Type eiType, AccessPathFragment[] base) {
 		Set<AccessPathFragment[]> bases = baseRegister.computeIfAbsent(eiType,
-				t -> new TCustomHashSet<>(new HashingStrategy<AccessPathFragment[]>() {
+				t -> Collections.synchronizedSet(new TCustomHashSet<>(new HashingStrategy<AccessPathFragment[]>() {
 
 					@Override
 					public int computeHashCode(AccessPathFragment[] arg0) {
@@ -429,7 +430,7 @@ public boolean equals(AccessPathFragment[] arg0, AccessPathFragment[] arg1) {
 						return Arrays.equals(arg0, arg1);
 					}
 
-				}));
+				})));
 		bases.add(base);
 	}
 

soot-infoflow/src/soot/jimple/infoflow/data/SourceContextAndPath.java

@@ -1,9 +1,6 @@
 package soot.jimple.infoflow.data;
 
-import java.util.ArrayList;
-import java.util.Collections;
-import java.util.Iterator;
-import java.util.List;
+import java.util.*;
 
 import heros.solver.Pair;
 import soot.jimple.Stmt;
@@ -90,19 +87,19 @@ public SourceContextAndPath extendPath(SourceContextAndPath other) {
 		if (this.path == null || other.path == null || other.path.size() <= this.path.size())
 			return null;
 
-		ArrayList<Abstraction> buf = new ArrayList<>(other.path.size() - this.path.size());
+		Stack<Abstraction> pathStack = new Stack<>();
 		Abstraction lastAbs = this.getLastAbstraction();
 		boolean foundCommonAbs = false;
 
 		// Collect all additional abstractions on the cached path
 		Iterator<Abstraction> pathIt = other.path.reverseIterator();
 		while (pathIt.hasNext()) {
 			Abstraction next = pathIt.next();
-			if (next == lastAbs) {
+			if (next == lastAbs || (next.neighbors != null && next.neighbors.contains(lastAbs))) {
 				foundCommonAbs = true;
 				break;
 			}
-			buf.add(next);
+			pathStack.push(next);
 		}
 
 		// If the paths do not have a common abstraction, there's probably something wrong...
@@ -111,15 +108,15 @@ public SourceContextAndPath extendPath(SourceContextAndPath other) {
 
 		// Append the additional abstractions to the new taint propagation path
 		SourceContextAndPath extendedScap = clone();
-		for (Abstraction abs : buf)
-			extendedScap.path.add(abs);
+		while (!pathStack.isEmpty())
+			extendedScap.path.add(pathStack.pop());
 
 		int newCallStackCapacity = other.getCallStackSize() - this.getCallStackSize();
 		// Sanity Check: The callStack of other should always be larger than the one of this
 		if (newCallStackCapacity < 0)
 			return null;
 		if (newCallStackCapacity > 0) {
-			ArrayList<Stmt> callStackBuf = new ArrayList<>(newCallStackCapacity);
+			Stack<Stmt> callStackBuf = new Stack<>();
 			Stmt topStmt = this.callStack == null ? null : this.callStack.getLast();
 
 			// Collect all additional statements on the call stack...
@@ -128,15 +125,15 @@ public SourceContextAndPath extendPath(SourceContextAndPath other) {
 				Stmt next = callStackIt.next();
 				if (next == topStmt)
 					break;
-				callStackBuf.add(next);
+				callStackBuf.push(next);
 			}
 
 			if (callStackBuf.size() > 0) {
 				if (extendedScap.callStack == null)
 					extendedScap.callStack = new ExtensibleList<>();
 				// ...and append them.
-				for (Stmt stmt : callStackBuf)
-					extendedScap.callStack.add(stmt);
+				while (!callStackBuf.isEmpty())
+					extendedScap.callStack.add(callStackBuf.pop());
 			}
 		}
 
@@ -213,32 +210,15 @@ public SourceContextAndPath extendPath(Abstraction abs, InfoflowConfiguration co
 		}
 
 		// Extend the call stack
-		switch (config.getDataFlowDirection()) {
-		case Forwards:
-			if (abs.getCorrespondingCallSite() != null && abs.getCorrespondingCallSite() != abs.getCurrentStmt()) {
-				if (scap == null)
-					scap = this.clone();
-				if (scap.callStack == null)
-					scap.callStack = new ExtensibleList<Stmt>();
-				else if (pathConfig != null && pathConfig.getMaxCallStackSize() > 0
-						&& scap.callStack.size() >= pathConfig.getMaxCallStackSize())
-					return null;
-				scap.callStack.add(abs.getCorrespondingCallSite());
-			}
-			break;
-		case Backwards:
-			if (abs.getCurrentStmt() != null && abs.getCurrentStmt().containsInvokeExpr()
-					&& abs.getCorrespondingCallSite() != abs.getCurrentStmt()) {
-				if (scap == null)
-					scap = this.clone();
-				if (scap.callStack == null)
-					scap.callStack = new ExtensibleList<Stmt>();
-				else if (pathConfig != null && pathConfig.getMaxCallStackSize() > 0
-						&& scap.callStack.size() >= pathConfig.getMaxCallStackSize())
-					return null;
-				scap.callStack.add(abs.getCurrentStmt());
-			}
-			break;
+		if (abs.getCorrespondingCallSite() != null && abs.getCorrespondingCallSite() != abs.getCurrentStmt()) {
+			if (scap == null)
+				scap = this.clone();
+			if (scap.callStack == null)
+				scap.callStack = new ExtensibleList<Stmt>();
+			else if (pathConfig != null && pathConfig.getMaxCallStackSize() > 0
+					&& scap.callStack.size() >= pathConfig.getMaxCallStackSize())
+				return null;
+			scap.callStack.add(abs.getCorrespondingCallSite());
 		}
 
 		this.neighborCounter = abs.getNeighbors() == null ? 0 : abs.getNeighbors().size();

soot-infoflow/src/soot/jimple/infoflow/data/pathBuilders/ContextInsensitivePathBuilder.java

@@ -129,7 +129,7 @@ private boolean checkForSource(Abstraction abs, SourceContextAndPath scap) {
 		SourceContext sourceContext = abs.getSourceContext();
 		results.addResult(scap.getDefinition(), scap.getAccessPath(), scap.getStmt(), sourceContext.getDefinition(),
 				sourceContext.getAccessPath(), sourceContext.getStmt(), sourceContext.getUserData(),
-				scap.getAbstractionPath());
+				scap.getAbstractionPath(), manager);
 		return true;
 	}
 

soot-infoflow/src/soot/jimple/infoflow/data/pathBuilders/ContextInsensitiveSourceFinder.java

@@ -87,7 +87,7 @@ public void run() {
 					results.addResult(flagAbs.getSinkDefinition(), flagAbs.getAbstraction().getAccessPath(),
 							flagAbs.getSinkStmt(), abstraction.getSourceContext().getDefinition(),
 							abstraction.getSourceContext().getAccessPath(), abstraction.getSourceContext().getStmt(),
-							abstraction.getSourceContext().getUserData(), null);
+							abstraction.getSourceContext().getUserData(), null, manager);
 
 					// Sources may not have predecessors
 					assert abstraction.getPredecessor() == null;

soot-infoflow/src/soot/jimple/infoflow/data/pathBuilders/ContextSensitivePathBuilder.java

@@ -33,7 +33,9 @@ public class ContextSensitivePathBuilder extends ConcurrentAbstractionPathBuilde
 	protected ConcurrentIdentityHashMultiMap<Abstraction, SourceContextAndPath> pathCache = new ConcurrentIdentityHashMultiMap<>();
 
 	// Set holds all paths that reach an already cached subpath
-	protected ConcurrentHashSet<Pair<SourceContextAndPath, SourceContextAndPath>> deferredPaths = new ConcurrentHashSet<>();
+	protected ConcurrentHashSet<SourceContextAndPath> deferredPaths = new ConcurrentHashSet<>();
+	// Set holds all paths that reach a source
+	protected ConcurrentHashSet<SourceContextAndPath> sourceReachingScaps = new ConcurrentHashSet<>();
 
 	/**
 	 * Creates a new instance of the {@link ContextSensitivePathBuilder} class
@@ -76,7 +78,7 @@ public SourceFindingTask(Abstraction abstraction) {
 		@Override
 		public void run() {
 			final Set<SourceContextAndPath> paths = pathCache.get(abstraction);
-			final Abstraction pred = abstraction.getPredecessor();
+			Abstraction pred = abstraction.getPredecessor();
 
 			if (pred != null && paths != null) {
 				for (SourceContextAndPath scap : paths) {
@@ -94,14 +96,6 @@ public void run() {
 		}
 
 		private void processAndQueue(Abstraction pred, SourceContextAndPath scap) {
-			// Skip abstractions that don't contain any new information. This might
-			// be the case when a turn unit was added to the abstraction.
-			if (pred.getCorrespondingCallSite() == null && pred.getCurrentStmt() == null
-					&& pred.getTurnUnit() != null) {
-				processAndQueue(pred.getPredecessor(), scap);
-				return;
-			}
-
 			ProcessingResult p = processPredecessor(scap, pred);
 			switch (p.getResult()) {
 			case NEW:
@@ -112,7 +106,7 @@ private void processAndQueue(Abstraction pred, SourceContextAndPath scap) {
 			case CACHED:
 				// In case we already know the subpath, we do append the path after the path
 				// builder terminated
-				deferredPaths.add(new Pair<>(scap, p.getScap()));
+				deferredPaths.add(scap);
 				break;
 			case INFEASIBLE_OR_MAX_PATHS_REACHED:
 				// Nothing to do
@@ -130,7 +124,8 @@ private ProcessingResult processPredecessor(SourceContextAndPath scap, Abstracti
 				if (extendedScap == null)
 					return ProcessingResult.INFEASIBLE_OR_MAX_PATHS_REACHED();
 
-				checkForSource(pred, extendedScap);
+				if (checkForSource(pred, extendedScap))
+					sourceReachingScaps.add(extendedScap);
 				return pathCache.put(pred, extendedScap) ? ProcessingResult.NEW()
 						: ProcessingResult.CACHED(extendedScap);
 			}
@@ -141,44 +136,24 @@ private ProcessingResult processPredecessor(SourceContextAndPath scap, Abstracti
 				return ProcessingResult.INFEASIBLE_OR_MAX_PATHS_REACHED();
 
 			// Check if we are in the right context
-			switch (manager.getConfig().getDataFlowDirection()) {
-			case Forwards:
-				if (pred.getCurrentStmt() != null && pred.getCurrentStmt().containsInvokeExpr()) {
-					// Pop the top item off the call stack. This gives us the item
-					// and the new SCAP without the item we popped off.
-					Pair<SourceContextAndPath, Stmt> pathAndItem = extendedScap.popTopCallStackItem();
-					if (pathAndItem != null) {
-						Stmt topCallStackItem = pathAndItem.getO2();
-						// Make sure that we don't follow an unrealizable path
-						if (topCallStackItem != pred.getCurrentStmt())
-							return ProcessingResult.INFEASIBLE_OR_MAX_PATHS_REACHED();
-
-						// We have returned from a function
-						extendedScap = pathAndItem.getO1();
-					}
-				}
-				break;
-			case Backwards:
-				if (pred.getCorrespondingCallSite() != null
-						&& pred.getCorrespondingCallSite() != pred.getCurrentStmt()) {
-					// Pop the top item off the call stack. This gives us the item
-					// and the new SCAP without the item we popped off.
-					Pair<SourceContextAndPath, Stmt> pathAndItem = extendedScap.popTopCallStackItem();
-					if (pathAndItem != null) {
-						Stmt topCallStackItem = pathAndItem.getO2();
-						// Make sure that we don't follow an unrealizable path
-						if (topCallStackItem != pred.getCorrespondingCallSite())
-							return ProcessingResult.INFEASIBLE_OR_MAX_PATHS_REACHED();
-
-						// We have returned from a function
-						extendedScap = pathAndItem.getO1();
-					}
+			if (pred.getCurrentStmt() != null && pred.getCurrentStmt().containsInvokeExpr()) {
+				// Pop the top item off the call stack. This gives us the item
+				// and the new SCAP without the item we popped off.
+				Pair<SourceContextAndPath, Stmt> pathAndItem = extendedScap.popTopCallStackItem();
+				if (pathAndItem != null) {
+					Stmt topCallStackItem = pathAndItem.getO2();
+					// Make sure that we don't follow an unrealizable path
+					if (topCallStackItem != pred.getCurrentStmt())
+						return ProcessingResult.INFEASIBLE_OR_MAX_PATHS_REACHED();
+
+					// We have returned from a function
+					extendedScap = pathAndItem.getO1();
 				}
-				break;
 			}
 
 			// Add the new path
-			checkForSource(pred, extendedScap);
+			if (checkForSource(pred, extendedScap))
+				sourceReachingScaps.add(extendedScap);
 
 			final int maxPaths = config.getPathConfiguration().getMaxPathsPerAbstraction();
 			if (maxPaths > 0) {
@@ -291,27 +266,14 @@ public void computeTaintPaths(Set<AbstractionAtSink> res) {
 	}
 
 	/**
-	 * Uses the cached path to extend the current path
-	 *
-	 * @param scap SourceContextAndPath of the current abstraction
-	 * @param cachedScap cached SourceContextAndPath to extend scap
+	 * Tries to fill up deferred paths toward a source.
 	 */
-	protected void buildFullPathFromCache(SourceContextAndPath scap, SourceContextAndPath cachedScap) {
-		// Try to extend scap with cachedScap
-		Stack<Pair<SourceContextAndPath, SourceContextAndPath>> workStack = new Stack<>();
-		workStack.push(new Pair<>(scap, cachedScap));
-		while (!workStack.isEmpty()) {
-			Pair<SourceContextAndPath, SourceContextAndPath> p = workStack.pop();
-			scap = p.getO1();
-			cachedScap = p.getO2();
-
-			SourceContextAndPath extendedScap = scap.extendPath(cachedScap);
-			if (extendedScap != null) {
-				Abstraction last = extendedScap.getLastAbstraction();
-				// Try to build the path further using the cache if we didn't reach a source
-				if (!checkForSource(last, extendedScap))
-					for (SourceContextAndPath preds : pathCache.get(last.getPredecessor()))
-						workStack.push(new Pair<>(extendedScap, preds));
+	protected void buildPathsFromCache() {
+		for (SourceContextAndPath deferredScap : deferredPaths) {
+			for (SourceContextAndPath sourceScap : sourceReachingScaps) {
+				SourceContextAndPath fullScap = deferredScap.extendPath(sourceScap);
+				if (fullScap != null)
+					checkForSource(fullScap.getLastAbstraction(), fullScap);
 			}
 		}
 	}
@@ -320,9 +282,7 @@ protected void buildFullPathFromCache(SourceContextAndPath scap, SourceContextAn
 	 * Method that is called when the taint paths have been computed
 	 */
 	protected void onTaintPathsComputed() {
-		for (Pair<SourceContextAndPath, SourceContextAndPath> deferredPair : deferredPaths) {
-			buildFullPathFromCache(deferredPair.getO1(), deferredPair.getO2());
-		}
+		buildPathsFromCache();
 	}
 
 	/**

soot-infoflow/src/soot/jimple/infoflow/data/pathBuilders/RecursivePathBuilder.java

@@ -145,7 +145,7 @@ public void run() {
 					for (SourceContextAndPath context : getPaths(lastTaskId++, abs.getAbstraction(), initialStack)) {
 						results.addResult(abs.getSinkDefinition(), abs.getAbstraction().getAccessPath(),
 								abs.getSinkStmt(), context.getDefinition(), context.getAccessPath(), context.getStmt(),
-								context.getUserData(), context.getAbstractionPath());
+								context.getUserData(), context.getAbstractionPath(), manager);
 					}
 				}
 

soot-infoflow/src/soot/jimple/infoflow/problems/rules/backward/BackwardsImplicitFlowRule.java

@@ -243,6 +243,7 @@ public Collection<Abstraction> propagateCallToReturnFlow(Abstraction d1, Abstrac
 				for (Unit condUnit : condUnits) {
 					Abstraction abs = new Abstraction(sink.getDefinition(), AccessPath.getEmptyAccessPath(), stmt,
 							sink.getUserData(), false, false);
+					abs.setCorrespondingCallSite(stmt);
 					abs.setDominator(condUnit);
 					res.add(abs);
 				}
@@ -252,6 +253,7 @@ public Collection<Abstraction> propagateCallToReturnFlow(Abstraction d1, Abstrac
 							.createAccessPath(sm.getActiveBody().getThisLocal(), false);
 					Abstraction thisTaint = new Abstraction(sink.getDefinition(), thisAp, stmt, sink.getUserData(),
 							false, false);
+					thisTaint.setCorrespondingCallSite(stmt);
 					res.add(thisTaint);
 				}
 

soot-infoflow/src/soot/jimple/infoflow/problems/rules/backward/BackwardsSinkPropagationRule.java

@@ -56,6 +56,7 @@ private Collection<Abstraction> propagate(Abstraction source, Stmt stmt, ByRefer
 					// Create the new taint abstraction
 					Abstraction abs = new Abstraction(sinkInfo.getDefinition(), ap, stmt, sinkInfo.getUserData(), false,
 							false);
+					abs.setCorrespondingCallSite(stmt);
 					abs = abs.deriveNewAbstractionWithTurnUnit(stmt);
 
 					res.add(abs);

soot-infoflow/src/soot/jimple/infoflow/results/InfoflowResults.java

@@ -153,13 +153,6 @@ public void addResult(ISourceSinkDefinition sinkDefinition, AccessPath sink, Stm
 				new ResultSourceInfo(sourceDefinition, source, sourceStmt, pathAgnosticResults));
 	}
 
-	public Pair<ResultSourceInfo, ResultSinkInfo> addResult(ISourceSinkDefinition sinkDefinition, AccessPath sink,
-			Stmt sinkStmt, ISourceSinkDefinition sourceDefinition, AccessPath source, Stmt sourceStmt, Object userData,
-			List<Abstraction> propagationPath) {
-		return addResult(sinkDefinition, sink, sinkStmt, sourceDefinition, source, sourceStmt, userData,
-				propagationPath, null);
-	}
-
 	public Pair<ResultSourceInfo, ResultSinkInfo> addResult(ISourceSinkDefinition sinkDefinition, AccessPath sink,
 			Stmt sinkStmt, ISourceSinkDefinition sourceDefinition, AccessPath source, Stmt sourceStmt, Object userData,
 			List<Abstraction> propagationPath, InfoflowManager manager) {

soot-infoflow/src/soot/jimple/infoflow/taintWrappers/EasyTaintWrapper.java

@@ -354,10 +354,6 @@ public Set<AccessPath> getInverseTaintsForMethodInternal(Stmt stmt, Abstraction
 				if (wrapType == MethodWrapType.KillTaint)
 					return Collections.emptySet();
 
-				// If the base was tainted, one parameter could be responsible for this but we don't know,
-				// maybe it also was tainted before. So we have to keep it.
-				taints.add(taintedPath);
-
 				if (wrapType == MethodWrapType.CreateTaint && taintedAbs.getDominator() != null
 						&& taintedAbs.getDominator() != null)
 					taints.add(AccessPath.getEmptyAccessPath());
@@ -376,6 +372,10 @@ public Set<AccessPath> getInverseTaintsForMethodInternal(Stmt stmt, Abstraction
 			}
 		}
 
+		// Keep the incoming taint if it is not the lhs
+		if (!taintedObj)
+			taints.add(taintedPath);
+
 		// if base object is tainted, we need to taint all parameters
 		if (isSupported && wrapType == MethodWrapType.CreateTaint) {
 			// If we are inside a conditional, we always taint