From a0761226696209ba2f70b752a4f40a2cedce24bd Mon Sep 17 00:00:00 2001
From: Valery Kharseko <vharseko@3a-systems.ru>
Date: Wed, 02 Sep 2026 06:10:07 +0000
Subject: [PATCH] [#890] Persist a compressed schema token before handing it out, and report the ones with no definition (#894)
---
opendj-server-legacy/src/main/java/org/opends/server/api/CompressedSchema.java | 371 +++++++++++++++++++++++++++++++++++++++++++++++++---
1 files changed, 347 insertions(+), 24 deletions(-)
diff --git a/opendj-server-legacy/src/main/java/org/opends/server/api/CompressedSchema.java b/opendj-server-legacy/src/main/java/org/opends/server/api/CompressedSchema.java
index 74088ab..c80a949 100644
--- a/opendj-server-legacy/src/main/java/org/opends/server/api/CompressedSchema.java
+++ b/opendj-server-legacy/src/main/java/org/opends/server/api/CompressedSchema.java
@@ -18,6 +18,7 @@
package org.opends.server.api;
import static org.opends.messages.CoreMessages.*;
+import static org.opends.server.util.StaticUtils.bytesToHexNoSpace;
import java.util.AbstractMap.SimpleImmutableEntry;
import java.util.Collection;
@@ -28,11 +29,15 @@
import java.util.List;
import java.util.Map;
import java.util.Map.Entry;
+import java.util.NoSuchElementException;
import java.util.Set;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.CopyOnWriteArrayList;
import java.util.concurrent.locks.ReentrantLock;
+import org.forgerock.i18n.LocalizableMessage;
+import org.forgerock.i18n.LocalizableMessageDescriptor;
+import org.forgerock.i18n.slf4j.LocalizedLogger;
import org.forgerock.opendj.ldap.AttributeDescription;
import org.forgerock.opendj.ldap.ByteSequenceReader;
import org.forgerock.opendj.ldap.ByteString;
@@ -59,6 +64,8 @@
mayInvoke = false)
public class CompressedSchema
{
+ private static final LocalizedLogger logger = LocalizedLogger.getLoggerForThisClass();
+
/** Encloses all the encode and decode mappings for attribute and object classes. */
private static final class Mappings
{
@@ -84,6 +91,9 @@
}
}
+ /** The most bytes {@link #encodeId(int)} ever writes a schema element ID in. */
+ private static final int MAX_ID_BYTES = 4;
+
private final ServerContext serverContext;
/** Lock serializing all mutations (id registration and schema reload). */
private final ReentrantLock exclusiveLock = new ReentrantLock();
@@ -155,7 +165,19 @@
{
for(int id=0;id<mappings.adDecodeMap.size();id++){
final AttributeDescription ad = mappings.adDecodeMap.get(id);
- loadAttributeToMaps(id, ad.getAttributeType().getNameOrOID(), ad.getOptions(), newMappings);
+ if (ad != null)
+ {
+ loadAttributeToMaps(id, ad.getAttributeType().getNameOrOID(), ad.getOptions(), newMappings);
+ }
+ else
+ {
+ // A decode map can carry a gap: it is padded with null for the ids missing from the
+ // compressed schema it was loaded from. Carry the gap over rather than dereferencing it,
+ // and carry it over as a gap - dropping it would shift the ids of the elements after it,
+ // and would let the next registration hand out an id an already written entry carries.
+ // The ids are walked in order from zero, so the new map holds exactly id elements here.
+ newMappings.adDecodeMap.add(null);
+ }
}
}
@@ -166,7 +188,16 @@
private void reloadObjectClassesMap(Mappings mappings, Mappings newMappings)
{
for(int id=0;id<mappings.ocDecodeMap.size();id++){
- loadObjectClassesToMaps(id, mappings.ocDecodeMap.get(id).values(), newMappings, false);
+ final Map<ObjectClass, String> ocMap = mappings.ocDecodeMap.get(id);
+ if (ocMap != null)
+ {
+ loadObjectClassesToMaps(id, ocMap.values(), newMappings, false);
+ }
+ else
+ {
+ // A gap, as in reloadAttributeTypeMaps().
+ newMappings.ocDecodeMap.add(null);
+ }
}
}
@@ -184,15 +215,16 @@
throws DirectoryException
{
// First decode the encoded attribute description id.
- final int adId = decodeId(reader);
+ final byte[] adIdBytes = readIdBytes(reader, ERR_COMPRESSEDSCHEMA_UNREADABLE_AD_TOKEN);
+ final int adId = decodeId(adIdBytes);
// Before returning the attribute, make sure that the attribute type is not stale.
final Mappings mappings = reloadMappingsIfSchemaChanged();
- final AttributeDescription ad = mappings.adDecodeMap.get(adId);
+ final AttributeDescription ad = decodeMapGet(mappings.adDecodeMap, adId);
if (ad == null)
{
throw new DirectoryException(DirectoryServer.getCoreConfigManager().getServerErrorResultCode(),
- ERR_COMPRESSEDSCHEMA_UNRECOGNIZED_AD_TOKEN.get(adId));
+ ERR_COMPRESSEDSCHEMA_UNRECOGNIZED_AD_TOKEN.get(tokenInMessage(adIdBytes, adId)));
}
AttributeType attrType = ad.getAttributeType();
@@ -224,6 +256,44 @@
}
/**
+ * Returns the element a token addresses, or {@code null} where it addresses none: the token can
+ * be outside the range of the decode map, or address one of the slots the map is padded with for
+ * the ids missing from the compressed schema it was loaded from. Both are reported to the caller
+ * as the unknown token they are, rather than let out of the decode path as an unchecked
+ * exception the callers of that path are not written for.
+ *
+ * @param decodeMap
+ * The decode map to look the token up in.
+ * @param id
+ * The decoded token.
+ * @return The element registered under the token, or {@code null} if there is none.
+ */
+ private static <T> T decodeMapGet(final List<T> decodeMap, final int id)
+ {
+ if (id < 0)
+ {
+ return null;
+ }
+ try
+ {
+ return decodeMap.get(id);
+ }
+ catch (final IndexOutOfBoundsException e)
+ {
+ // Caught rather than kept away by a comparison against size(): size() and get() of a
+ // CopyOnWriteArrayList read the array separately, so the comparison would not make the
+ // lookup safe anyway, and this runs for every attribute of every entry read from a backend -
+ // the common path is left with the single read it had.
+ //
+ // Traced here because the caller turns this into a DirectoryException carrying the token:
+ // the generic catch of Entry.decode(), which used to convert this exception, logged the
+ // stack, and where the token came from is worth keeping for a corrupt store.
+ logger.traceException(e);
+ return null;
+ }
+ }
+
+ /**
* Decodes an object class set from the provided byte string.
*
* @param reader
@@ -237,15 +307,16 @@
final ByteSequenceReader reader) throws DirectoryException
{
// First decode the encoded object class id.
- final int ocId = decodeId(reader);
+ final byte[] ocIdBytes = readIdBytes(reader, ERR_COMPRESSEDSCHEMA_UNREADABLE_OC_TOKEN);
+ final int ocId = decodeId(ocIdBytes);
// Before returning the object classes, make sure that none of them are stale.
final Mappings mappings = reloadMappingsIfSchemaChanged();
- Map<ObjectClass, String> ocMap = mappings.ocDecodeMap.get(ocId);
+ Map<ObjectClass, String> ocMap = decodeMapGet(mappings.ocDecodeMap, ocId);
if (ocMap == null)
{
throw new DirectoryException(DirectoryServer.getCoreConfigManager().getServerErrorResultCode(),
- ERR_COMPRESSEDSCHEMA_UNKNOWN_OC_TOKEN.get(ocId));
+ ERR_COMPRESSEDSCHEMA_UNKNOWN_OC_TOKEN.get(tokenInMessage(ocIdBytes, ocId)));
}
return ocMap;
}
@@ -297,10 +368,7 @@
id = mappings.adEncodeMap.get(ad);
if (id == null)
{
- id = mappings.adDecodeMap.size();
- mappings.adDecodeMap.add(ad);
- mappings.adEncodeMap.put(ad, id);
- storeAttribute(encodeId(id), ad.getAttributeType().getNameOrOID(), ad.getOptions());
+ id = registerAttribute(mappings, ad);
}
return id;
}
@@ -311,6 +379,93 @@
}
/**
+ * Registers a new attribute description and returns the id allocated to it. The registration is
+ * persisted before it is published, and is withdrawn if it cannot be persisted: an entry must
+ * never be written with a token whose definition did not reach the storage, because nothing
+ * stores it afterwards and the entry cannot be decoded once the server is restarted.
+ * <p>
+ * Must be called with the exclusive lock held, which is what makes the id allocated here still
+ * the last element of the decode map when it has to be withdrawn. The lock is reentrant, so
+ * that holds only while the store stays out of this compressed schema: an implementation of
+ * {@link #storeAttribute(byte[], String, Iterable)} must re-enter neither the encode, the load
+ * nor the decode path of it, which its own javadoc says as well. That it did stay out is
+ * checked by {@link #withdraw(Mappings, List, int, Object)} rather than assumed, since removing
+ * an element this registration did not append is worse than the leak it withdraws.
+ */
+ private int registerAttribute(final Mappings mappings, final AttributeDescription ad) throws DirectoryException
+ {
+ final int id = mappings.adDecodeMap.size();
+ // Appended to the decode map first: storeAttribute() is free to persist the whole content of
+ // this compressed schema rather than the single element it is handed - DefaultCompressedSchema
+ // rewrites its file from getAllAttributes() - so the element being registered has to be part
+ // of it by then. The decode map is not what an encode reaches the id through, so nothing can
+ // yet write an entry carrying it.
+ mappings.adDecodeMap.add(ad);
+ boolean registered = false;
+ try
+ {
+ storeAttribute(encodeId(id), ad.getAttributeType().getNameOrOID(), ad.getOptions());
+ // Published only once persisted: the encode map is read without the lock, so an id another
+ // thread finds there can be carried by an entry a moment later and must never be withdrawn.
+ mappings.adEncodeMap.put(ad, id);
+ registered = true;
+ }
+ finally
+ {
+ if (!registered)
+ {
+ withdraw(mappings, mappings.adDecodeMap, id, ad);
+ }
+ }
+ return id;
+ }
+
+ /**
+ * Withdraws the element a failed registration appended, so that the next attempt allocates the
+ * id again and stores it. Removed by index, and by the index of the last element: every append
+ * is made under the exclusive lock, so this is still the element appended by the registration
+ * being withdrawn, and no other id shifts.
+ * <p>
+ * That the element is still there is checked rather than assumed, because the lock is
+ * reentrant: a store re-entering the encode or the load path appends to the same decode map,
+ * and one re-entering the decode path replaces the mappings altogether. Neither is allowed by
+ * the contract of {@link #storeAttribute(byte[], String, Iterable)}, and neither is withdrawn
+ * from here - removing an element this registration did not append shifts the ids of everything
+ * after it, and the entries already written carry them. The violation is reported instead, and
+ * the element is left where it is: the registration leaks, which is what this method exists to
+ * prevent, but no id already handed out starts decoding as something else.
+ * <p>
+ * Reported rather than thrown, because this runs while the failure of the store is on its way
+ * out: that failure is what the caller has to be told.
+ *
+ * @param mappings
+ * The mappings the registration appended to.
+ * @param decodeMap
+ * The decode map of {@code mappings} the element was appended to.
+ * @param appendedAt
+ * The index the element was appended at, which is the id allocated to it.
+ * @param appended
+ * The element that was appended.
+ */
+ private void withdraw(final Mappings mappings, final List<?> decodeMap, final int appendedAt,
+ final Object appended)
+ {
+ if (mappings == this.mappings
+ && decodeMap.size() == appendedAt + 1
+ && decodeMap.get(appendedAt) == appended)
+ {
+ decodeMap.remove(appendedAt);
+ return;
+ }
+ logger.error(LocalizableMessage.raw(
+ "The registration of the compressed schema id %s could not be withdrawn after its store failed, "
+ + "because the store re-entered the compressed schema it was called from: the id is now taken "
+ + "by a definition that was never persisted. This is a defect of %s, whose store must re-enter "
+ + "neither the encode, the load nor the decode path of the compressed schema.",
+ appendedAt, getClass().getName()));
+ }
+
+ /**
* Encodes the provided set of object classes to a byte array. If the same set
* had been previously encoded, then the cached value will be used. Otherwise,
* a new value will be created.
@@ -354,10 +509,7 @@
id = mappings.ocEncodeMap.get(objectClasses);
if (id == null)
{
- id = mappings.ocDecodeMap.size();
- mappings.ocDecodeMap.add(objectClasses);
- mappings.ocEncodeMap.put(objectClasses, id);
- storeObjectClasses(encodeId(id), objectClasses.values());
+ id = registerObjectClasses(mappings, objectClasses);
}
return id;
}
@@ -368,6 +520,36 @@
}
/**
+ * Registers a new object class set and returns the id allocated to it, persisting the
+ * registration before publishing it and withdrawing it if it cannot be persisted, exactly as
+ * {@link #registerAttribute(Mappings, AttributeDescription)} does.
+ * <p>
+ * Must be called with the exclusive lock held, and under the same constraint on what
+ * {@link #storeObjectClasses(byte[], Collection)} may re-enter.
+ */
+ private int registerObjectClasses(final Mappings mappings, final Map<ObjectClass, String> objectClasses)
+ throws DirectoryException
+ {
+ final int id = mappings.ocDecodeMap.size();
+ mappings.ocDecodeMap.add(objectClasses);
+ boolean registered = false;
+ try
+ {
+ storeObjectClasses(encodeId(id), objectClasses.values());
+ mappings.ocEncodeMap.put(objectClasses, id);
+ registered = true;
+ }
+ finally
+ {
+ if (!registered)
+ {
+ withdraw(mappings, mappings.ocDecodeMap, id, objectClasses);
+ }
+ }
+ return id;
+ }
+
+ /**
* Returns a view of the encoded attributes in this compressed schema which can be used for saving
* the entire content to disk.
* <p>
@@ -385,19 +567,43 @@
return new Iterator<Entry<byte[], Entry<String, Iterable<String>>>>()
{
private int id;
- private List<AttributeDescription> adDecodeMap = getMappings().adDecodeMap;
+ private final List<AttributeDescription> adDecodeMap = getMappings().adDecodeMap;
@Override
public boolean hasNext()
{
- return id < adDecodeMap.size();
+ // Skips the gaps: a decode map padded with null for the ids missing from the
+ // compressed schema it was loaded from is still saved, and the ids around a gap are
+ // preserved by the token each element is written with. Looked up through
+ // decodeMapGet(), because withdrawing a registration shortens the decode map and a
+ // CopyOnWriteArrayList reads its array separately for size() and for get(). In tree
+ // this iteration runs under the exclusive lock - the only caller of save() is a store
+ // - but the class is extensible and a subclass can reach here from anywhere.
+ while (id < adDecodeMap.size())
+ {
+ if (decodeMapGet(adDecodeMap, id) != null)
+ {
+ return true;
+ }
+ id++;
+ }
+ return false;
}
@Override
public Entry<byte[], Entry<String, Iterable<String>>> next()
{
+ if (!hasNext())
+ {
+ throw new NoSuchElementException();
+ }
final byte[] encodedAttribute = encodeId(id);
- final AttributeDescription ad = adDecodeMap.get(id++);
+ final AttributeDescription ad = decodeMapGet(adDecodeMap, id++);
+ if (ad == null)
+ {
+ // The decode map was shortened between hasNext() and here.
+ throw new NoSuchElementException();
+ }
return new SimpleImmutableEntry<byte[], Entry<String, Iterable<String>>>(
encodedAttribute,
new SimpleImmutableEntry<String, Iterable<String>>(
@@ -437,14 +643,32 @@
@Override
public boolean hasNext()
{
- return id < ocDecodeMap.size();
+ // Skips the gaps, and looks the elements up the same way, as in getAllAttributes().
+ while (id < ocDecodeMap.size())
+ {
+ if (decodeMapGet(ocDecodeMap, id) != null)
+ {
+ return true;
+ }
+ id++;
+ }
+ return false;
}
@Override
public Entry<byte[], Collection<String>> next()
{
+ if (!hasNext())
+ {
+ throw new NoSuchElementException();
+ }
final byte[] encodedObjectClasses = encodeId(id);
- final Map<ObjectClass, String> ocMap = ocDecodeMap.get(id++);
+ final Map<ObjectClass, String> ocMap = decodeMapGet(ocDecodeMap, id++);
+ if (ocMap == null)
+ {
+ // The decode map was shortened between hasNext() and here.
+ throw new NoSuchElementException();
+ }
return new SimpleImmutableEntry<>(encodedObjectClasses, ocMap.values());
}
@@ -631,6 +855,17 @@
* assume that this method is not being called by other threads. Note that
* this method is not thread-safe with respect to
* {@link #storeObjectClasses(byte[], Collection)}.
+ * <p>
+ * Called with the exclusive lock of this compressed schema held, and that lock is reentrant, so
+ * an implementation must re-enter neither the encode, the load nor the decode path of the
+ * compressed schema it belongs to. The registration being persisted has already been appended
+ * to the decode map - so that an implementation persisting the whole content rather than the
+ * element it is handed, as {@code DefaultCompressedSchema} does, has it - and is withdrawn from
+ * there if this method throws. Encoding or loading appends to the same decode map, and the
+ * withdrawal would take back whatever was appended last; decoding rebuilds the mappings when the
+ * schema has changed, and the withdrawal would then have to take the element out of a map that
+ * has already been replaced. Neither is withdrawn: the violation is reported and the
+ * registration is left behind, holding an id no definition was persisted under.
*
* @param encodedAttribute
* The encoded attribute description.
@@ -654,6 +889,17 @@
* can assume that this method is not being called by other threads. Note that
* this method is not thread-safe with respect to
* {@link #storeAttribute(byte[], String, Iterable)}.
+ * <p>
+ * Called with the exclusive lock of this compressed schema held, and that lock is reentrant, so
+ * an implementation must re-enter neither the encode, the load nor the decode path of the
+ * compressed schema it belongs to. The registration being persisted has already been appended
+ * to the decode map - so that an implementation persisting the whole content rather than the
+ * element it is handed, as {@code DefaultCompressedSchema} does, has it - and is withdrawn from
+ * there if this method throws. Encoding or loading appends to the same decode map, and the
+ * withdrawal would take back whatever was appended last; decoding rebuilds the mappings when the
+ * schema has changed, and the withdrawal would then have to take the element out of a map that
+ * has already been replaced. Neither is withdrawn: the violation is reported and the
+ * registration is left behind, holding an id no definition was persisted under.
*
* @param encodedObjectClasses
* The encoded object classes.
@@ -686,12 +932,89 @@
return id - 1; // Subtract 1 to compensate for old behavior.
}
- private int decodeId(final ByteSequenceReader reader)
+ /**
+ * Reads the encoded schema element ID at the current position, reporting a record the ID cannot
+ * be read from rather than letting the read out of the decode path as an unchecked exception -
+ * for the same reason the lookup the ID feeds does not: the callers of a {@code @PublicAPI}
+ * decode path are written for {@link DirectoryException}.
+ *
+ * @param reader
+ * The byte string reader positioned on an encoded schema element ID.
+ * @param unreadableToken
+ * The message reporting a token this decode path cannot read.
+ * @return The encoded schema element ID, as the storage holds it.
+ * @throws DirectoryException
+ * If the record holds no readable schema element ID at the current position.
+ */
+ private static byte[] readIdBytes(final ByteSequenceReader reader,
+ final LocalizableMessageDescriptor.Arg1<Object> unreadableToken) throws DirectoryException
{
- final int length = reader.readBERLength();
+ final int length;
+ try
+ {
+ length = reader.readBERLength();
+ }
+ catch (final IndexOutOfBoundsException e)
+ {
+ // Both of the conditions readBERLength() reports this way: the record ends inside the
+ // length itself, and a length header naming more than the four bytes a length is written in.
+ throw unreadable(unreadableToken,
+ "the record ends inside the length of the token, or that length names more than four bytes", e);
+ }
+ if (length < 0 || length > MAX_ID_BYTES)
+ {
+ // The length is composed from up to four bytes unsigned, so a corrupt record can name a
+ // negative count - 0xFFFFFFFF - or more bytes than an id is ever written in. The upper
+ // bound is what encodeId() emits, and it is not only a sanity check: decodeId() folds
+ // whatever it is handed, so a token padded with leading zeros decodes to the id its
+ // canonical token addresses, and a record carrying one would read as a live definition
+ // instead of being reported. Checked before the array is allocated, too - a length of
+ // 0x7FFFFFFF is a two gigabyte allocation no reader of a corrupt record should attempt.
+ throw unreadable(unreadableToken, "the token names " + (length & 0xFFFFFFFFL)
+ + " bytes, and an id is never encoded in more than " + MAX_ID_BYTES, null);
+ }
+ if (length > reader.remaining())
+ {
+ throw unreadable(unreadableToken,
+ "the token names " + length + " bytes and the record holds " + reader.remaining(), null);
+ }
final byte[] idBytes = new byte[length];
reader.readBytes(idBytes);
- return decodeId(idBytes);
+ return idBytes;
+ }
+
+ /**
+ * Returns the exception reporting a token this decode path cannot read, tracing what the read
+ * raised where it raised anything: the callers of this path convert an exception of their own
+ * into a message, and where the record went wrong is worth keeping for a corrupt store.
+ */
+ private static DirectoryException unreadable(
+ final LocalizableMessageDescriptor.Arg1<Object> unreadableToken, final String reason,
+ final RuntimeException cause)
+ {
+ if (cause != null)
+ {
+ logger.traceException(cause);
+ }
+ return new DirectoryException(DirectoryServer.getCoreConfigManager().getServerErrorResultCode(),
+ unreadableToken.get(reason), cause);
+ }
+
+ /**
+ * Names a token in a message as the storage holds it - the key a definition is written under -
+ * together with the id it decodes to. The id on its own is one less than what was read, so a
+ * token that no definition was ever written under is reported as a value appearing nowhere in
+ * the stored data: an all-zero token reads as the id -1.
+ *
+ * @param idBytes
+ * The encoded schema element ID, as it was read.
+ * @param id
+ * The schema element ID it decoded to.
+ * @return The token as a message should name it.
+ */
+ private static String tokenInMessage(final byte[] idBytes, final int id)
+ {
+ return "0x" + bytesToHexNoSpace(idBytes) + " (id " + id + ")";
}
/**
--
Gitblit v1.10.0