From cbd9f63a9d4ba003c60e53b0adc2d1a56c6d3560 Mon Sep 17 00:00:00 2001 From: Justin Bertram Date: Thu, 15 Dec 2022 09:31:49 -0600 Subject: [PATCH] ARTEMIS-4115 ArrayIndexOutOfBoundsException when dup cache size is 0 Allow setting id-cache-size to 0 from broker.xml and ensure the broker handles this gracefully. Previously you could only set the cache size to 0 via broker properties or programmatically and it would throw an ArrayIndexOutOfBoundsException when adding an item to the cache. --- .../impl/FileConfigurationParser.java | 2 +- .../postoffice/impl/DuplicateIDCaches.java | 13 ++- .../postoffice/impl/NoOpDuplicateIDCache.java | 77 ++++++++++++++++++ .../impl/FileConfigurationParserTest.java | 12 +++ .../integration/DuplicateDetectionTest.java | 79 ++++++++++++++++++- .../persistence/DuplicateCacheTest.java | 18 +++++ 6 files changed, 194 insertions(+), 7 deletions(-) create mode 100644 artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/NoOpDuplicateIDCache.java diff --git a/artemis-server/src/main/java/org/apache/activemq/artemis/core/deployers/impl/FileConfigurationParser.java b/artemis-server/src/main/java/org/apache/activemq/artemis/core/deployers/impl/FileConfigurationParser.java index f41b770290..efe2b10cbc 100644 --- a/artemis-server/src/main/java/org/apache/activemq/artemis/core/deployers/impl/FileConfigurationParser.java +++ b/artemis-server/src/main/java/org/apache/activemq/artemis/core/deployers/impl/FileConfigurationParser.java @@ -421,7 +421,7 @@ public final class FileConfigurationParser extends XMLConfigurationUtil { config.setAddressQueueScanPeriod(getLong(e, "address-queue-scan-period", config.getAddressQueueScanPeriod(), Validators.MINUS_ONE_OR_GT_ZERO)); - config.setIDCacheSize(getInteger(e, "id-cache-size", config.getIDCacheSize(), Validators.GT_ZERO)); + config.setIDCacheSize(getInteger(e, "id-cache-size", config.getIDCacheSize(), Validators.GE_ZERO)); config.setPersistIDCache(getBoolean(e, "persist-id-cache", config.isPersistIDCache())); diff --git a/artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/DuplicateIDCaches.java b/artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/DuplicateIDCaches.java index 79b55e8145..2f0e538ee5 100644 --- a/artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/DuplicateIDCaches.java +++ b/artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/DuplicateIDCaches.java @@ -29,11 +29,18 @@ public final class DuplicateIDCaches { public static DuplicateIDCache persistent(final SimpleString address, final int size, final StorageManager storageManager) { - return new PersistentDuplicateIDCache(address, size, storageManager); + if (size == 0) { + return new NoOpDuplicateIDCache(); + } else { + return new PersistentDuplicateIDCache(address, size, storageManager); + } } public static DuplicateIDCache inMemory(final SimpleString address, final int size) { - return new InMemoryDuplicateIDCache(address, size); + if (size == 0) { + return new NoOpDuplicateIDCache(); + } else { + return new InMemoryDuplicateIDCache(address, size); + } } - } diff --git a/artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/NoOpDuplicateIDCache.java b/artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/NoOpDuplicateIDCache.java new file mode 100644 index 0000000000..998074ed05 --- /dev/null +++ b/artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/NoOpDuplicateIDCache.java @@ -0,0 +1,77 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.activemq.artemis.core.postoffice.impl; + +import java.util.Collections; +import java.util.List; + +import org.apache.activemq.artemis.api.core.Pair; +import org.apache.activemq.artemis.core.postoffice.DuplicateIDCache; +import org.apache.activemq.artemis.core.transaction.Transaction; + +public final class NoOpDuplicateIDCache implements DuplicateIDCache { + + @Override + public boolean contains(byte[] duplicateID) { + return false; + } + + @Override + public boolean atomicVerify(byte[] duplID, Transaction tx) throws Exception { + return false; + } + + @Override + public void addToCache(byte[] duplicateID) throws Exception { + + } + + @Override + public void addToCache(byte[] duplicateID, Transaction tx) throws Exception { + + } + + @Override + public void addToCache(byte[] duplicateID, Transaction tx, boolean instantAdd) throws Exception { + + } + + @Override + public void deleteFromCache(byte[] duplicateID) throws Exception { + + } + + @Override + public void load(List> ids) throws Exception { + + } + + @Override + public void load(Transaction tx, byte[] duplID) { + + } + + @Override + public void clear() throws Exception { + + } + + @Override + public List> getMap() { + return Collections.emptyList(); + } +} diff --git a/artemis-server/src/test/java/org/apache/activemq/artemis/core/config/impl/FileConfigurationParserTest.java b/artemis-server/src/test/java/org/apache/activemq/artemis/core/config/impl/FileConfigurationParserTest.java index e38f67ffbe..88a1000cf2 100644 --- a/artemis-server/src/test/java/org/apache/activemq/artemis/core/config/impl/FileConfigurationParserTest.java +++ b/artemis-server/src/test/java/org/apache/activemq/artemis/core/config/impl/FileConfigurationParserTest.java @@ -121,6 +121,18 @@ public class FileConfigurationParserTest extends ActiveMQTestBase { Assert.assertEquals(333, config.getClusterConfigurations().get(0).getRetryInterval()); } + @Test + public void testParsingZeroIDCacheSize() throws Exception { + FileConfigurationParser parser = new FileConfigurationParser(); + + String configStr = firstPart + "0" + lastPart; + ByteArrayInputStream input = new ByteArrayInputStream(configStr.getBytes(StandardCharsets.UTF_8)); + + Configuration config = parser.parseMainConfig(input); + + Assert.assertEquals(0, config.getIDCacheSize()); + } + @Test public void testWildcardConfiguration() throws Exception { FileConfigurationParser parser = new FileConfigurationParser(); diff --git a/tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/DuplicateDetectionTest.java b/tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/DuplicateDetectionTest.java index 94b5d6cce0..5f09dc6f95 100644 --- a/tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/DuplicateDetectionTest.java +++ b/tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/DuplicateDetectionTest.java @@ -19,7 +19,7 @@ package org.apache.activemq.artemis.tests.integration; import javax.transaction.xa.XAException; import javax.transaction.xa.XAResource; import javax.transaction.xa.Xid; - +import java.lang.invoke.MethodHandles; import java.util.Arrays; import java.util.Collection; @@ -37,7 +37,6 @@ import org.apache.activemq.artemis.api.core.client.ClientSession; import org.apache.activemq.artemis.api.core.client.ClientSessionFactory; import org.apache.activemq.artemis.api.core.client.ServerLocator; import org.apache.activemq.artemis.core.config.Configuration; - import org.apache.activemq.artemis.core.postoffice.impl.PostOfficeImpl; import org.apache.activemq.artemis.core.server.ActiveMQServer; import org.apache.activemq.artemis.core.transaction.impl.XidImpl; @@ -51,7 +50,6 @@ import org.junit.runner.RunWith; import org.junit.runners.Parameterized; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import java.lang.invoke.MethodHandles; @RunWith(Parameterized.class) public class DuplicateDetectionTest extends ActiveMQTestBase { @@ -136,6 +134,81 @@ public class DuplicateDetectionTest extends ActiveMQTestBase { Assert.assertNull(message2); } + @Test + public void testDisabledDuplicateDetection() throws Exception { + server.stop(); + + config = createDefaultInVMConfig().setIDCacheSize(0); + + server = createServer(config); + + server.start(); + + sf = createSessionFactory(locator); + + ClientSession session = sf.createSession(false, true, true); + + session.start(); + + final SimpleString queueName = new SimpleString("DuplicateDetectionTestQueue"); + + session.createQueue(new QueueConfiguration(queueName).setDurable(false)); + + ClientProducer producer = session.createProducer(queueName); + + ClientConsumer consumer = session.createConsumer(queueName); + + ClientMessage message = createMessage(session, 0); + producer.send(message); + ClientMessage message2 = consumer.receive(1000); + Assert.assertEquals(0, message2.getObjectProperty(propKey)); + + message = createMessage(session, 1); + SimpleString dupID = new SimpleString("abcdefg"); + message.putBytesProperty(Message.HDR_DUPLICATE_DETECTION_ID, dupID.getData()); + producer.send(message); + message2 = consumer.receive(1000); + Assert.assertEquals(1, message2.getObjectProperty(propKey)); + + message = createMessage(session, 2); + SimpleString dupID1 = new SimpleString("abcdefg1"); + message.putBytesProperty(Message.HDR_DUPLICATE_DETECTION_ID, dupID1.getData()); + producer.send(message); + message2 = consumer.receive(1000); + Assert.assertEquals(2, message2.getObjectProperty(propKey)); + + message = createMessage(session, 3); + message.putBytesProperty(Message.HDR_DUPLICATE_DETECTION_ID, dupID.getData()); + producer.send(message); + message2 = consumer.receiveImmediate(); + Assert.assertEquals(3, message2.getObjectProperty(propKey)); + + message = createMessage(session, 4); + message.putBytesProperty(Message.HDR_DUPLICATE_DETECTION_ID, dupID.getData()); + producer.send(message); + message2 = consumer.receiveImmediate(); + Assert.assertEquals(4, message2.getObjectProperty(propKey)); + + message = createMessage(session, 5); + SimpleString dupID2 = new SimpleString("hijklmnop"); + message.putBytesProperty(Message.HDR_DUPLICATE_DETECTION_ID, dupID2.getData()); + producer.send(message); + message2 = consumer.receive(1000); + Assert.assertEquals(5, message2.getObjectProperty(propKey)); + + message = createMessage(session, 6); + message.putBytesProperty(Message.HDR_DUPLICATE_DETECTION_ID, dupID2.getData()); + producer.send(message); + message2 = consumer.receiveImmediate(); + Assert.assertEquals(6, message2.getObjectProperty(propKey)); + + message = createMessage(session, 7); + message.putBytesProperty(Message.HDR_DUPLICATE_DETECTION_ID, dupID.getData()); + producer.send(message); + message2 = consumer.receiveImmediate(); + Assert.assertEquals(7, message2.getObjectProperty(propKey)); + } + @Test public void testDuplicateIDCacheMemoryRetentionForNonTemporaryQueues() throws Exception { testDuplicateIDCacheMemoryRetention(false); diff --git a/tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/persistence/DuplicateCacheTest.java b/tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/persistence/DuplicateCacheTest.java index 39fe106999..d14d316429 100644 --- a/tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/persistence/DuplicateCacheTest.java +++ b/tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/persistence/DuplicateCacheTest.java @@ -128,4 +128,22 @@ public class DuplicateCacheTest extends StorageManagerTestBase { cache.clear(); } + + @Test + public void testDisabledPersistentCache() throws Exception { + createStorage(); + DuplicateIDCache cache = DuplicateIDCaches.persistent(new SimpleString("test"), 0, journal); + byte[] bytes = RandomUtil.randomBytes(); + // Previously this would throw an ArrayIndexOutOfBoundsException + cache.addToCache(bytes); + } + + @Test + public void testDisabledInMemoryCache() throws Exception { + createStorage(); + DuplicateIDCache cache = DuplicateIDCaches.inMemory(new SimpleString("test"), 0); + byte[] bytes = RandomUtil.randomBytes(); + // Previously this would throw an ArrayIndexOutOfBoundsException + cache.addToCache(bytes); + } }