From 38db0431495b718c56c0b59f2c81d9af8108c665 Mon Sep 17 00:00:00 2001 From: Matthew Khouzam Date: Sat, 1 Oct 2016 23:13:21 -0400 Subject: [PATCH] tmf: annotate TmfContext#location as nullable This fixes a potential race condition if the context is changed after a null check but before a dereference. In the current implementation, it is extremely unlikely to happen, but this may save later investigation time. Change-Id: I9083d16fd8e3728fe829583f4de78384109a91e6 Signed-off-by: Matthew Khouzam Reviewed-on: https://git.eclipse.org/r/82325 Reviewed-by: Patrick Tasse Tested-by: Patrick Tasse Reviewed-by: Hudson CI --- .../tracecompass/btf/core/trace/BtfTrace.java | 45 +++++++++++-------- .../core/parsers/custom/CustomTxtTrace.java | 3 +- .../core/parsers/custom/CustomXmlTrace.java | 7 +-- .../tmf/core/trace/TmfContext.java | 21 ++++----- .../core/trace/experiment/TmfExperiment.java | 18 ++++---- .../tmf/core/trace/text/TextTrace.java | 3 +- 6 files changed, 51 insertions(+), 46 deletions(-) diff --git a/btf/org.eclipse.tracecompass.btf.core/src/org/eclipse/tracecompass/btf/core/trace/BtfTrace.java b/btf/org.eclipse.tracecompass.btf.core/src/org/eclipse/tracecompass/btf/core/trace/BtfTrace.java index b881b9bb21..8d938a083c 100644 --- a/btf/org.eclipse.tracecompass.btf.core/src/org/eclipse/tracecompass/btf/core/trace/BtfTrace.java +++ b/btf/org.eclipse.tracecompass.btf.core/src/org/eclipse/tracecompass/btf/core/trace/BtfTrace.java @@ -144,7 +144,8 @@ public class BtfTrace extends TmfTrace implements ITmfPersistentlyIndexable, ITm fProperties.put(CREATIONDATE, fCreationDate); try { - // DateFormats are inherently unsafe for multithreaded use so we can't make this a field. Just in case. + // DateFormats are inherently unsafe for multithreaded use + // so we can't make this a field. Just in case. final SimpleDateFormat ISO8601DATEFORMAT = new SimpleDateFormat("yyyy-MM-dd'T'HH:mm:ssX"); //$NON-NLS-1$ Date dateTime = ISO8601DATEFORMAT.parse(fCreationDate); fTsOffset = dateTime.getTime() * MICROSECONDS_IN_A_SECOND; @@ -363,9 +364,10 @@ public class BtfTrace extends TmfTrace implements ITmfPersistentlyIndexable, ITm } final TmfContext context = (TmfContext) tmfContext; - if (context.getLocation() == null - || !(context.getLocation().getLocationInfo() instanceof Long) - || NULL_LOCATION.equals(context.getLocation())) { + ITmfLocation location = context.getLocation(); + if (location == null + || !(location.getLocationInfo() instanceof Long) + || NULL_LOCATION.equals(location)) { return null; } @@ -381,21 +383,24 @@ public class BtfTrace extends TmfTrace implements ITmfPersistentlyIndexable, ITm * @return the event from a given line */ private ITmfEvent parseLine(TmfContext context) { - try { - if (!context.getLocation().getLocationInfo().equals(fFileInput.getFilePointer())) { - seekEvent(context.getLocation()); + ITmfLocation location = context.getLocation(); + if (location != null) { + try { + if (!location.getLocationInfo().equals(fFileInput.getFilePointer())) { + seekEvent(location); + } + } catch (IOException e1) { + seekEvent(location); } - } catch (IOException e1) { - seekEvent(context.getLocation()); - } - String line; - try { - line = fFileInput.readLine(); - return parseLine(context.getRank(), line); + String line; + try { + line = fFileInput.readLine(); + return parseLine(context.getRank(), line); - } catch (IOException e) { + } catch (IOException e) { + Activator.logError(e.getMessage(), e); + } } - return null; } @@ -502,9 +507,11 @@ public class BtfTrace extends TmfTrace implements ITmfPersistentlyIndexable, ITm if (signal.getTrace() == this) { try { synchronized (this) { - // Reset the file handle in case it has reached the end of the - // file already. Otherwise, it will not be able to read new data - // pass the previous end. + /* + * Reset the file handle in case it has reached the end of + * the file already. Otherwise, it will not be able to read + * new data pass the previous end. + */ initFile(); } } catch (TmfTraceException e) { diff --git a/tmf/org.eclipse.tracecompass.tmf.core/src/org/eclipse/tracecompass/tmf/core/parsers/custom/CustomTxtTrace.java b/tmf/org.eclipse.tracecompass.tmf.core/src/org/eclipse/tracecompass/tmf/core/parsers/custom/CustomTxtTrace.java index 318a56660b..4327bdfc45 100644 --- a/tmf/org.eclipse.tracecompass.tmf.core/src/org/eclipse/tracecompass/tmf/core/parsers/custom/CustomTxtTrace.java +++ b/tmf/org.eclipse.tracecompass.tmf.core/src/org/eclipse/tracecompass/tmf/core/parsers/custom/CustomTxtTrace.java @@ -264,7 +264,8 @@ public class CustomTxtTrace extends TmfTrace implements ITmfPersistentlyIndexabl } final CustomTxtTraceContext context = (CustomTxtTraceContext) tmfContext; - if (context.getLocation() == null || !(context.getLocation().getLocationInfo() instanceof Long) || NULL_LOCATION.equals(context.getLocation())) { + ITmfLocation location = context.getLocation(); + if (location == null || !(location.getLocationInfo() instanceof Long) || NULL_LOCATION.equals(location)) { return null; } diff --git a/tmf/org.eclipse.tracecompass.tmf.core/src/org/eclipse/tracecompass/tmf/core/parsers/custom/CustomXmlTrace.java b/tmf/org.eclipse.tracecompass.tmf.core/src/org/eclipse/tracecompass/tmf/core/parsers/custom/CustomXmlTrace.java index e562fd880e..50298e6624 100644 --- a/tmf/org.eclipse.tracecompass.tmf.core/src/org/eclipse/tracecompass/tmf/core/parsers/custom/CustomXmlTrace.java +++ b/tmf/org.eclipse.tracecompass.tmf.core/src/org/eclipse/tracecompass/tmf/core/parsers/custom/CustomXmlTrace.java @@ -266,15 +266,16 @@ public class CustomXmlTrace extends TmfTrace implements ITmfPersistentlyIndexabl } final CustomXmlTraceContext context = (CustomXmlTraceContext) tmfContext; - if (context.getLocation() == null || !(context.getLocation().getLocationInfo() instanceof Long) || NULL_LOCATION.equals(context.getLocation())) { + ITmfLocation location = context.getLocation(); + if (location == null || !(location.getLocationInfo() instanceof Long) || NULL_LOCATION.equals(location)) { return null; } CustomXmlEvent event = null; try { // Below +1 for the < - if (fFile.getFilePointer() != (Long) context.getLocation().getLocationInfo() + 1) { - fFile.seek((Long) context.getLocation().getLocationInfo() + 1); + if (fFile.getFilePointer() != (Long) location.getLocationInfo() + 1) { + fFile.seek((Long) location.getLocationInfo() + 1); } final StringBuffer elementBuffer = new StringBuffer("<"); //$NON-NLS-1$ readElement(elementBuffer, fFile); diff --git a/tmf/org.eclipse.tracecompass.tmf.core/src/org/eclipse/tracecompass/tmf/core/trace/TmfContext.java b/tmf/org.eclipse.tracecompass.tmf.core/src/org/eclipse/tracecompass/tmf/core/trace/TmfContext.java index d62c5ee752..8b99e3567a 100644 --- a/tmf/org.eclipse.tracecompass.tmf.core/src/org/eclipse/tracecompass/tmf/core/trace/TmfContext.java +++ b/tmf/org.eclipse.tracecompass.tmf.core/src/org/eclipse/tracecompass/tmf/core/trace/TmfContext.java @@ -14,6 +14,9 @@ package org.eclipse.tracecompass.tmf.core.trace; +import java.util.Objects; + +import org.eclipse.jdt.annotation.Nullable; import org.eclipse.tracecompass.tmf.core.trace.location.ITmfLocation; /** @@ -33,7 +36,7 @@ public class TmfContext implements ITmfContext { // ------------------------------------------------------------------------ // The trace location - private ITmfLocation fLocation; + private @Nullable ITmfLocation fLocation; // The event rank private long fRank; @@ -87,7 +90,7 @@ public class TmfContext implements ITmfContext { // ------------------------------------------------------------------------ @Override - public ITmfLocation getLocation() { + public @Nullable ITmfLocation getLocation() { return fLocation; } @@ -128,11 +131,7 @@ public class TmfContext implements ITmfContext { @Override public int hashCode() { - final int prime = 31; - int result = 1; - result = prime * result + ((fLocation == null) ? 0 : fLocation.hashCode()); - result = prime * result + (int) (fRank ^ (fRank >>> 32)); - return result; + return Objects.hash(fRank, fLocation); } @Override @@ -147,14 +146,10 @@ public class TmfContext implements ITmfContext { return false; } final TmfContext other = (TmfContext) obj; - if (fLocation == null) { - if (other.fLocation != null) { - return false; - } - } else if (!fLocation.equals(other.fLocation)) { + if (fRank != other.fRank) { return false; } - if (fRank != other.fRank) { + if (!Objects.equals(fLocation, other.fLocation)) { return false; } return true; diff --git a/tmf/org.eclipse.tracecompass.tmf.core/src/org/eclipse/tracecompass/tmf/core/trace/experiment/TmfExperiment.java b/tmf/org.eclipse.tracecompass.tmf.core/src/org/eclipse/tracecompass/tmf/core/trace/experiment/TmfExperiment.java index b2e3cea3a9..a426fd3732 100644 --- a/tmf/org.eclipse.tracecompass.tmf.core/src/org/eclipse/tracecompass/tmf/core/trace/experiment/TmfExperiment.java +++ b/tmf/org.eclipse.tracecompass.tmf.core/src/org/eclipse/tracecompass/tmf/core/trace/experiment/TmfExperiment.java @@ -299,9 +299,7 @@ public class TmfExperiment extends TmfTrace implements ITmfPersistentlyIndexable int length = getNbChildren(); // Initialize the location array if necessary - TmfLocationArray locationArray = ((location == null) ? - new TmfLocationArray(length) : - ((TmfExperimentLocation) location).getLocationInfo()); + TmfLocationArray locationArray = ((location == null) ? new TmfLocationArray(length) : ((TmfExperimentLocation) location).getLocationInfo()); ITmfLocation[] locations = locationArray.getLocations(); long[] ranks = locationArray.getRanks(); @@ -424,10 +422,13 @@ public class TmfExperiment extends TmfTrace implements ITmfPersistentlyIndexable } // Update the experiment location - TmfLocationArray locationArray = new TmfLocationArray( - ((TmfExperimentLocation) expContext.getLocation()).getLocationInfo(), - trace, traceContext.getLocation(), traceContext.getRank()); - expContext.setLocation(new TmfExperimentLocation(locationArray)); + ITmfLocation location = expContext.getLocation(); + if (location instanceof TmfExperimentLocation) { + TmfLocationArray locationArray = new TmfLocationArray( + ((TmfExperimentLocation) location).getLocationInfo(), + trace, traceContext.getLocation(), traceContext.getRank()); + expContext.setLocation(new TmfExperimentLocation(locationArray)); + } } } @@ -610,8 +611,7 @@ public class TmfExperiment extends TmfTrace implements ITmfPersistentlyIndexable } safeTimestamp = endTimestamp; if (timeRange != null) { - final TmfTraceRangeUpdatedSignal signal = - new TmfTraceRangeUpdatedSignal(TmfExperiment.this, TmfExperiment.this, timeRange); + final TmfTraceRangeUpdatedSignal signal = new TmfTraceRangeUpdatedSignal(TmfExperiment.this, TmfExperiment.this, timeRange); broadcast(signal); } } diff --git a/tmf/org.eclipse.tracecompass.tmf.core/src/org/eclipse/tracecompass/tmf/core/trace/text/TextTrace.java b/tmf/org.eclipse.tracecompass.tmf.core/src/org/eclipse/tracecompass/tmf/core/trace/text/TextTrace.java index 8c13d91e48..48d1477030 100644 --- a/tmf/org.eclipse.tracecompass.tmf.core/src/org/eclipse/tracecompass/tmf/core/trace/text/TextTrace.java +++ b/tmf/org.eclipse.tracecompass.tmf.core/src/org/eclipse/tracecompass/tmf/core/trace/text/TextTrace.java @@ -275,7 +275,8 @@ public abstract class TextTrace extends TmfTrace imple return null; } TextTraceContext context = tmfContext; - if (context.getLocation() == null || !(context.getLocation().getLocationInfo() instanceof Long) || NULL_LOCATION.equals(context.getLocation())) { + ITmfLocation location = context.getLocation(); + if (location == null || !(location.getLocationInfo() instanceof Long) || NULL_LOCATION.equals(location)) { return null; } -- 2.34.1