You are viewing a plain text version of this content. The canonical link for it is here.
Posted to oak-commits@jackrabbit.apache.org by md...@apache.org on 2017/03/01 18:06:32 UTC
svn commit: r1785012 - in
/jackrabbit/oak/trunk/oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment:
MapEntry.java MapRecord.java Segment.java SegmentWriter.java
Author: mduerig
Date: Wed Mar 1 18:06:32 2017
New Revision: 1785012
URL: http://svn.apache.org/viewvc?rev=1785012&view=rev
Log:
OAK-5301: Possible null dereference in MapRecord
Accept null values in compareTo method and make read/write usage cases of MapRecord more explicit
Modified:
jackrabbit/oak/trunk/oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/MapEntry.java
jackrabbit/oak/trunk/oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/MapRecord.java
jackrabbit/oak/trunk/oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/Segment.java
jackrabbit/oak/trunk/oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/SegmentWriter.java
Modified: jackrabbit/oak/trunk/oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/MapEntry.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/MapEntry.java?rev=1785012&r1=1785011&r2=1785012&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/MapEntry.java (original)
+++ jackrabbit/oak/trunk/oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/MapEntry.java Wed Mar 1 18:06:32 2017
@@ -29,6 +29,7 @@ import javax.annotation.Nonnull;
import javax.annotation.Nullable;
import com.google.common.collect.ComparisonChain;
+import com.google.common.collect.Ordering;
import org.apache.jackrabbit.oak.spi.state.AbstractChildNodeEntry;
/**
@@ -49,18 +50,66 @@ class MapEntry extends AbstractChildNode
@CheckForNull
private final RecordId value;
- MapEntry(@Nonnull SegmentReader reader, @Nonnull String name,
- @Nonnull RecordId key, @Nullable RecordId value) {
+ private MapEntry(
+ @Nonnull SegmentReader reader,
+ @Nonnull String name,
+ @Nonnull RecordId key,
+ @Nullable RecordId value) {
this.reader = checkNotNull(reader);
this.name = checkNotNull(name);
this.key = checkNotNull(key);
this.value = value;
}
+ /**
+ * Create a new instance of a {@code MapEntry} as it has been read from a segment.
+
+ * @param reader segment reader for reading the node state this map entry's value points to.
+ * @param name name of the key
+ * @param key record id of the key
+ * @param value record id of the value
+ */
+ static MapEntry newMapEntry(
+ @Nonnull SegmentReader reader,
+ @Nonnull String name,
+ @Nonnull RecordId key,
+ @Nonnull RecordId value) {
+ return new MapEntry(reader, name, key, checkNotNull(value));
+ }
+
+ /**
+ * Create a new instance of a {@code MapEntry} to be written to a segment. Here the passed
+ * {@code value} might be {@code null} to indicate an existing mapping for the this {@code key}
+ * should be deleted. In this case calls to {@link #getValue()} should be guarded with a prior
+ * call to {@link #isDeleted()} to prevent the former throwing an {@code IllegalStateException}.
+ *
+ * @param reader segment reader for reading the node state this map entry's value points to.
+ * @param name name of the key
+ * @param key record id of the key
+ * @param value record id of the value or {@code null}.
+ *
+ * @see #isDeleted()
+ * @see #getValue()
+ */
+ static MapEntry newModifiedMapEntry(
+ @Nonnull SegmentReader reader,
+ @Nonnull String name,
+ @Nonnull RecordId key,
+ @Nullable RecordId value) {
+ return new MapEntry(reader, name, key, value);
+ }
+
public int getHash() {
return MapRecord.getHash(name);
}
+ /**
+ * @return {@code true} to indicate that this value is to be deleted.
+ */
+ boolean isDeleted() {
+ return value == null;
+ }
+
//----------------------------------------------------< ChildNodeEntry >--
@Override @Nonnull
@@ -82,9 +131,14 @@ class MapEntry extends AbstractChildNode
return key;
}
- @CheckForNull
+ /**
+ * @return the value of this mapping.
+ * @throws IllegalStateException if {@link #isDeleted()} is {@code true}.
+ */
+ @Nonnull
@Override
public RecordId getValue() {
+ checkState(value != null);
return value;
}
@@ -100,7 +154,7 @@ class MapEntry extends AbstractChildNode
return ComparisonChain.start()
.compare(getHash() & HASH_MASK, that.getHash() & HASH_MASK)
.compare(name, that.name)
- .compare(value, that.value) // FIXME OAK-5301: Possible null dereference in MapRecord
+ .compare(value, that.value, Ordering.natural().nullsLast())
.result();
}
Modified: jackrabbit/oak/trunk/oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/MapRecord.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/MapRecord.java?rev=1785012&r1=1785011&r2=1785012&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/MapRecord.java (original)
+++ jackrabbit/oak/trunk/oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/MapRecord.java Wed Mar 1 18:06:32 2017
@@ -24,6 +24,7 @@ import static com.google.common.collect.
import static java.lang.Integer.bitCount;
import static java.lang.Integer.highestOneBit;
import static java.lang.Integer.numberOfTrailingZeros;
+import static org.apache.jackrabbit.oak.segment.MapEntry.newMapEntry;
import java.util.Arrays;
import java.util.Collections;
@@ -165,7 +166,7 @@ public class MapRecord extends Record {
RecordId key = segment.readRecordId(getRecordNumber(), 8);
if (name.equals(reader.readString(key))) {
RecordId value = segment.readRecordId(getRecordNumber(), 8, 1);
- return new MapEntry(reader, name, key, value);
+ return newMapEntry(reader, name, key, value);
}
}
RecordId base = segment.readRecordId(getRecordNumber(), 8, 2);
@@ -218,7 +219,7 @@ public class MapRecord extends Record {
RecordId valueId = segment.readRecordId(getRecordNumber(), 4 + size * 4, i * 2 + 1);
diff = reader.readString(keyId).compareTo(name);
if (diff == 0) {
- return new MapEntry(reader, name, keyId, valueId);
+ return newMapEntry(reader, name, keyId, valueId);
}
}
@@ -369,7 +370,7 @@ public class MapRecord extends Record {
value = segment.readRecordId(getRecordNumber(), 4 + size * 4, i * 2 + 1);
}
String name = reader.readString(key);
- entries[i] = new MapEntry(reader, name, key, value);
+ entries[i] = newMapEntry(reader, name, key, value);
}
return Arrays.asList(entries);
}
@@ -484,9 +485,7 @@ public class MapRecord extends Record {
} else if (d == 0) {
assert beforeEntry != null;
assert afterEntry != null;
- RecordId beforeValue = beforeEntry.getValue();
- assert beforeValue != null;
- if (!beforeValue.equals(afterEntry.getValue())
+ if (!beforeEntry.getValue().equals(afterEntry.getValue())
&& !diff.childNodeChanged(
beforeEntry.getName(),
beforeEntry.getNodeState(),
Modified: jackrabbit/oak/trunk/oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/Segment.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/Segment.java?rev=1785012&r1=1785011&r2=1785012&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/Segment.java (original)
+++ jackrabbit/oak/trunk/oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/Segment.java Wed Mar 1 18:06:32 2017
@@ -45,6 +45,8 @@ import java.util.UUID;
import javax.annotation.CheckForNull;
import javax.annotation.Nonnull;
+import com.google.common.base.Charsets;
+import com.google.common.collect.AbstractIterator;
import org.apache.commons.io.HexDump;
import org.apache.commons.io.output.ByteArrayOutputStream;
import org.apache.jackrabbit.oak.api.PropertyState;
@@ -53,9 +55,6 @@ import org.apache.jackrabbit.oak.commons
import org.apache.jackrabbit.oak.plugins.memory.PropertyStates;
import org.apache.jackrabbit.oak.segment.RecordNumbers.Entry;
-import com.google.common.base.Charsets;
-import com.google.common.collect.AbstractIterator;
-
/**
* A list of records.
* <p>
@@ -463,6 +462,7 @@ public class Segment {
d.get(buffer, offset, length);
}
+ @Nonnull
RecordId readRecordId(int recordNumber, int rawOffset, int recordIdOffset) {
return internalReadRecordId(pos(recordNumber, rawOffset, recordIdOffset, RECORD_ID_BYTES));
}
@@ -475,6 +475,7 @@ public class Segment {
return readRecordId(recordNumber, 0, 0);
}
+ @Nonnull
private RecordId internalReadRecordId(int pos) {
SegmentId segmentId = dereferenceSegmentId(asUnsigned(data.getShort(pos)));
return new RecordId(segmentId, data.getInt(pos + 2));
@@ -484,6 +485,7 @@ public class Segment {
return value & 0xffff;
}
+ @Nonnull
private SegmentId dereferenceSegmentId(int reference) {
if (reference == 0) {
return id;
Modified: jackrabbit/oak/trunk/oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/SegmentWriter.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/SegmentWriter.java?rev=1785012&r1=1785011&r2=1785012&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/SegmentWriter.java (original)
+++ jackrabbit/oak/trunk/oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/SegmentWriter.java Wed Mar 1 18:06:32 2017
@@ -42,6 +42,7 @@ import static org.apache.jackrabbit.oak.
import static org.apache.jackrabbit.oak.api.Type.NAME;
import static org.apache.jackrabbit.oak.api.Type.NAMES;
import static org.apache.jackrabbit.oak.api.Type.STRING;
+import static org.apache.jackrabbit.oak.segment.MapEntry.newModifiedMapEntry;
import static org.apache.jackrabbit.oak.segment.MapRecord.BUCKETS_PER_LEVEL;
import static org.apache.jackrabbit.oak.segment.RecordWriters.newNodeStateWriter;
@@ -507,7 +508,7 @@ public class SegmentWriter {
}
if (keyId != null) {
- entries.add(new MapEntry(reader, key, keyId, entry.getValue()));
+ entries.add(newModifiedMapEntry(reader, key, keyId, entry.getValue()));
}
}
return writeMapBucket(base, entries, 0);
@@ -573,10 +574,10 @@ public class SegmentWriter {
map.put(entry.getName(), entry);
}
for (MapEntry entry : entries) {
- if (entry.getValue() != null) {
- map.put(entry.getName(), entry);
- } else {
+ if (entry.isDeleted()) {
map.remove(entry.getName());
+ } else {
+ map.put(entry.getName(), entry);
}
}
return writeMapBucket(null, map.values(), level);