From 93b1ecae5aff7a18e16556f749c8aba5806dc512 Mon Sep 17 00:00:00 2001
From: Fraser Tweedale <ftweedal@redhat.com>
Date: Wed, 29 Aug 2018 16:55:31 +1000
Subject: [PATCH 1/5] getTheSerialNumber: only return null if next range not
available
When cloning, if the master's current number range has been depleted
due to a previous UpdateNumberRange request,
Repository.getTheSerialNumber() returns null because the next serial
number is out of the current range, but the next range has not been
activated yet. NullPointerException ensues.
Update getTheSerialNumber() to return the next serial number even
when it exceeds the current number range, as long as there is a next
range. If there is no next range, return null (as before). It is
assumed that the next range is non-empty
Also do a couple of drive-by method extractions to improve
readability.
Part of: https://pagure.io/dogtagpki/issue/3055
(cherry picked from commit f1615df509053a8f474b82ea6a2fa0883ab06d09)
---
.../src/com/netscape/cmscore/dbs/Repository.java | 61 ++++++++++++++++------
1 file changed, 44 insertions(+), 17 deletions(-)
diff --git a/base/server/cmscore/src/com/netscape/cmscore/dbs/Repository.java b/base/server/cmscore/src/com/netscape/cmscore/dbs/Repository.java
index afe9013..c5120c4 100644
--- a/base/server/cmscore/src/com/netscape/cmscore/dbs/Repository.java
+++ b/base/server/cmscore/src/com/netscape/cmscore/dbs/Repository.java
@@ -317,7 +317,15 @@ public abstract class Repository implements IRepository {
}
/**
- * get the next serial number in cache
+ * Peek at the next serial number in cache (does not consume the
+ * number).
+ *
+ * The returned number is not necessarily the previously emitted
+ * serial number plus one, i.e. if we are going to roll into the
+ * next range. This method does not actually switch the range.
+ *
+ * Returns null if the next number exceeds the current range and
+ * there is not a next range.
*/
public BigInteger getTheSerialNumber() throws EBaseException {
@@ -327,7 +335,7 @@ public abstract class Repository implements IRepository {
BigInteger serial = mLastSerialNo.add(BigInteger.ONE);
if (mMaxSerialNo != null && serial.compareTo(mMaxSerialNo) > 0)
- return null;
+ return hasNextRange() ? mNextMinSerialNo : null;
else
return serial;
}
@@ -390,9 +398,13 @@ public abstract class Repository implements IRepository {
}
/**
- * Checks to see if range needs to be switched.
+ * Checks if the given number is in the current range.
+ * If it does not exceed the current range, return cleanly.
+ * If it exceeds the given range, and there is a next range, switch the range.
+ * If it exceeds the given range, and there is not a next range, throw EDBException.
*
- * @exception EBaseException thrown when next range is not allocated
+ * @exception EDBException thrown when range switch is needed
+ * but next range is not allocated
*/
protected void checkRange() throws EBaseException
{
@@ -413,7 +425,7 @@ public abstract class Repository implements IRepository {
if (mDB.getEnableSerialMgmt()) {
CMS.debug("Reached the end of the range. Attempting to move to next range");
- if ((mNextMinSerialNo == null) || (mNextMaxSerialNo == null)) {
+ if (!hasNextRange()) {
if (rangeLength != null && mCounter.compareTo(rangeLength) < 0) {
return;
} else {
@@ -421,18 +433,7 @@ public abstract class Repository implements IRepository {
mLastSerialNo.toString()));
}
}
- mMinSerialNo = mNextMinSerialNo;
- mMaxSerialNo = mNextMaxSerialNo;
- mLastSerialNo = mMinSerialNo;
- mNextMinSerialNo = null;
- mNextMaxSerialNo = null;
- mCounter = BigInteger.ZERO;
-
- // persist the changes
- mDB.setMinSerialConfig(mRepo, mMinSerialNo.toString(mRadix));
- mDB.setMaxSerialConfig(mRepo, mMaxSerialNo.toString(mRadix));
- mDB.setNextMinSerialConfig(mRepo, null);
- mDB.setNextMaxSerialConfig(mRepo, null);
+ switchToNextRange();
} else {
throw new EDBException(CMS.getUserMessage("CMS_DBS_LIMIT_REACHED",
mLastSerialNo.toString()));
@@ -441,6 +442,32 @@ public abstract class Repository implements IRepository {
}
/**
+ * Return true iff there is a next range ready to go.
+ */
+ private boolean hasNextRange() {
+ return (mNextMinSerialNo != null) && (mNextMaxSerialNo != null);
+ }
+
+ /**
+ * Switch to the next range and persist the changes.
+ */
+ private void switchToNextRange()
+ throws EBaseException {
+ mMinSerialNo = mNextMinSerialNo;
+ mMaxSerialNo = mNextMaxSerialNo;
+ mLastSerialNo = mMinSerialNo;
+ mNextMinSerialNo = null;
+ mNextMaxSerialNo = null;
+ mCounter = BigInteger.ZERO;
+
+ // persist the changes
+ mDB.setMinSerialConfig(mRepo, mMinSerialNo.toString(mRadix));
+ mDB.setMaxSerialConfig(mRepo, mMaxSerialNo.toString(mRadix));
+ mDB.setNextMinSerialConfig(mRepo, null);
+ mDB.setNextMaxSerialConfig(mRepo, null);
+ }
+
+ /**
* Checks to see if a new range is needed, or if we have reached the end of the
* current range, or if a range conflict has occurred.
*
--
1.8.3.1
From 35714763d32425f18a0ac8817aad96c8cfab589b Mon Sep 17 00:00:00 2001
From: Fraser Tweedale <ftweedal@redhat.com>
Date: Mon, 3 Sep 2018 15:55:35 +1000
Subject: [PATCH 2/5] Repository: handle depleted range in initCache()
Repository.initCache() does not handle the case where the current
range has been fully depleted, but the switch to the next range has
not occurred yet. This situation arises when the range has been
fully depleted by servicing UpdateNumberRange requests for clones.
Detect this situation and handle it by switching to the next range
(when available).
Part of: https://pagure.io/dogtagpki/issue/3055
(cherry picked from commit 2fb3611db5145dbdd5e7e14daaad1470691494f0)
---
.../src/com/netscape/cmscore/dbs/Repository.java | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
diff --git a/base/server/cmscore/src/com/netscape/cmscore/dbs/Repository.java b/base/server/cmscore/src/com/netscape/cmscore/dbs/Repository.java
index c5120c4..828217c 100644
--- a/base/server/cmscore/src/com/netscape/cmscore/dbs/Repository.java
+++ b/base/server/cmscore/src/com/netscape/cmscore/dbs/Repository.java
@@ -298,6 +298,25 @@ public abstract class Repository implements IRepository {
BigInteger theSerialNo = null;
theSerialNo = getLastSerialNumberInRange(mMinSerialNo, mMaxSerialNo);
+ if (theSerialNo == null) {
+ // This arises when range has been depleted by servicing
+ // UpdateNumberRange requests for clones. Attempt to
+ // move to next range.
+ CMS.debug(
+ "Repository: failed to get last serial number in range "
+ + mMinSerialNo + ".." + mMaxSerialNo);
+
+ if (hasNextRange()) {
+ CMS.debug("Repository: switching to next range.");
+ switchToNextRange();
+ CMS.debug("Repository: new range: " + mMinSerialNo + ".." + mMaxSerialNo);
+ // try again with updated range
+ theSerialNo = getLastSerialNumberInRange(mMinSerialNo, mMaxSerialNo);
+ } else {
+ CMS.debug("Repository: next range not available.");
+ }
+ }
+
if (theSerialNo != null) {
mLastSerialNo = new BigInteger(theSerialNo.toString());
--
1.8.3.1
From e1345a22c1d5236754fbeb93b0d3f8f2c447d918 Mon Sep 17 00:00:00 2001
From: Fraser Tweedale <ftweedal@redhat.com>
Date: Wed, 29 Aug 2018 17:31:34 +1000
Subject: [PATCH 3/5] rename method getTheSerialNumber -> peekNextSerialNumber
Rename Repository.getTheSerialNumber -> peekNextSerialNumber to more
accurately reflect what it does: peek at the next serial number
without actually consuming it.
Part of: https://pagure.io/dogtagpki/issue/3055
(cherry picked from commit 85e356580f64f87c0b01736b71dc3d385db0bcba)
---
base/ca/src/com/netscape/ca/CertificateAuthority.java | 2 +-
base/common/src/com/netscape/certsrv/dbs/repository/IRepository.java | 2 +-
.../cms/src/com/netscape/cms/servlet/csadmin/UpdateNumberRange.java | 2 +-
base/server/cmscore/src/com/netscape/cmscore/dbs/Repository.java | 2 +-
4 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/base/ca/src/com/netscape/ca/CertificateAuthority.java b/base/ca/src/com/netscape/ca/CertificateAuthority.java
index 0281db0..f414628 100644
--- a/base/ca/src/com/netscape/ca/CertificateAuthority.java
+++ b/base/ca/src/com/netscape/ca/CertificateAuthority.java
@@ -1077,7 +1077,7 @@ public class CertificateAuthority
public String getStartSerial() {
try {
BigInteger serial =
- mCertRepot.getTheSerialNumber();
+ mCertRepot.peekNextSerialNumber();
if (serial == null)
return "";
diff --git a/base/common/src/com/netscape/certsrv/dbs/repository/IRepository.java b/base/common/src/com/netscape/certsrv/dbs/repository/IRepository.java
index 39744ac..d0b6135 100644
--- a/base/common/src/com/netscape/certsrv/dbs/repository/IRepository.java
+++ b/base/common/src/com/netscape/certsrv/dbs/repository/IRepository.java
@@ -50,7 +50,7 @@ public interface IRepository {
* @return serial number
* @exception EBaseException failed to retrieve next serial number
*/
- public BigInteger getTheSerialNumber() throws EBaseException;
+ public BigInteger peekNextSerialNumber() throws EBaseException;
/**
* Set the maximum serial number.
diff --git a/base/server/cms/src/com/netscape/cms/servlet/csadmin/UpdateNumberRange.java b/base/server/cms/src/com/netscape/cms/servlet/csadmin/UpdateNumberRange.java
index 2586da2..e5b5168 100644
--- a/base/server/cms/src/com/netscape/cms/servlet/csadmin/UpdateNumberRange.java
+++ b/base/server/cms/src/com/netscape/cms/servlet/csadmin/UpdateNumberRange.java
@@ -187,7 +187,7 @@ public class UpdateNumberRange extends CMSServlet {
BigInteger decrement = new BigInteger(decrementStr, radix);
beginNum = endNum.subtract(decrement).add(oneNum);
- if (beginNum.compareTo(repo.getTheSerialNumber()) < 0) {
+ if (beginNum.compareTo(repo.peekNextSerialNumber()) < 0) {
String nextEndNumStr = cs.getString(nextEndConfig, "");
BigInteger endNum2 = new BigInteger(nextEndNumStr, radix);
CMS.debug("Transferring from the end of on-deck range");
diff --git a/base/server/cmscore/src/com/netscape/cmscore/dbs/Repository.java b/base/server/cmscore/src/com/netscape/cmscore/dbs/Repository.java
index 828217c..55068ea 100644
--- a/base/server/cmscore/src/com/netscape/cmscore/dbs/Repository.java
+++ b/base/server/cmscore/src/com/netscape/cmscore/dbs/Repository.java
@@ -346,7 +346,7 @@ public abstract class Repository implements IRepository {
* Returns null if the next number exceeds the current range and
* there is not a next range.
*/
- public BigInteger getTheSerialNumber() throws EBaseException {
+ public BigInteger peekNextSerialNumber() throws EBaseException {
CMS.debug("Repository:In getTheSerialNumber ");
if (mLastSerialNo == null)
--
1.8.3.1
From 26586f3e711f1e238d4692f801dd8de42b88fc53 Mon Sep 17 00:00:00 2001
From: Fraser Tweedale <ftweedal@redhat.com>
Date: Wed, 29 Aug 2018 21:42:40 +1000
Subject: [PATCH 4/5] checkRange: small refactor and add commentary
Add some commentary about the behaviour and proper usage of
Repository.checkRange(). Also perform a small refactor, avoiding
a redundant stringify and parse.
Part of: https://pagure.io/dogtagpki/issue/3055
(cherry picked from commit 5a606e83719272fb488047b28a9ca7d5ce2ea30b)
---
.../src/com/netscape/cmscore/dbs/Repository.java | 19 +++++++++++++++----
1 file changed, 15 insertions(+), 4 deletions(-)
diff --git a/base/server/cmscore/src/com/netscape/cmscore/dbs/Repository.java b/base/server/cmscore/src/com/netscape/cmscore/dbs/Repository.java
index 55068ea..9bc7e2a 100644
--- a/base/server/cmscore/src/com/netscape/cmscore/dbs/Repository.java
+++ b/base/server/cmscore/src/com/netscape/cmscore/dbs/Repository.java
@@ -406,14 +406,17 @@ public abstract class Repository implements IRepository {
throw new EBaseException("mLastSerialNo is null");
}
+ /* Advance the serial number. checkRange() will check if it exceeds
+ * the current range and, if so, rolls to the next range and resets
+ * mLastSerialNo to the start of the new range. Hence we return
+ * mLastSerialNo below, after the call to checkRange().
+ */
mLastSerialNo = mLastSerialNo.add(BigInteger.ONE);
checkRange();
- BigInteger retSerial = new BigInteger(mLastSerialNo.toString());
-
- CMS.debug("Repository: getNextSerialNumber: returning retSerial " + retSerial);
- return retSerial;
+ CMS.debug("Repository: getNextSerialNumber: returning " + mLastSerialNo);
+ return mLastSerialNo;
}
/**
@@ -422,6 +425,14 @@ public abstract class Repository implements IRepository {
* If it exceeds the given range, and there is a next range, switch the range.
* If it exceeds the given range, and there is not a next range, throw EDBException.
*
+ * Precondition: the serial number should already have been advanced.
+ * This method will detect that and switch to the next range, including
+ * resetting mLastSerialNo to the start of the new (now current) range.
+ *
+ * Postcondition: the caller should again read mLastSerialNo after
+ * calling checkRange(), in case checkRange switched the range and the
+ * new range is not adjacent to the current range.
+ *
* @exception EDBException thrown when range switch is needed
* but next range is not allocated
*/
--
1.8.3.1
From 57edb3ee50b3ae634ee31ed643e2bb3a891b80fa Mon Sep 17 00:00:00 2001
From: Fraser Tweedale <ftweedal@redhat.com>
Date: Wed, 29 Aug 2018 22:22:10 +1000
Subject: [PATCH 5/5] Add missing synchronisation for range management
Several methods in Repository (and CertificateRepository) need
synchronisation on the intrisic lock. Make these methods
synchronised.
Also take the lock in UpdateNumberRange so that no serial numbers
can be handed out in other threads between peekNextSerialNumber()
and set(Next)?MaxSerial(). Without this synchronisation, it is
possible that the master instance will use some of the serial
numbers it transfers to the clone.
Fixes: https://pagure.io/dogtagpki/issue/3055
(cherry picked from commit 851a0bdd79c12c627a04cfc376338c1727cd50d9)
---
.../cms/servlet/csadmin/UpdateNumberRange.java | 35 +++++++-----
.../cmscore/dbs/CertificateRepository.java | 62 ++++++++++------------
.../src/com/netscape/cmscore/dbs/Repository.java | 6 +--
3 files changed, 53 insertions(+), 50 deletions(-)
diff --git a/base/server/cms/src/com/netscape/cms/servlet/csadmin/UpdateNumberRange.java b/base/server/cms/src/com/netscape/cms/servlet/csadmin/UpdateNumberRange.java
index e5b5168..c2ff7ed 100644
--- a/base/server/cms/src/com/netscape/cms/servlet/csadmin/UpdateNumberRange.java
+++ b/base/server/cms/src/com/netscape/cms/servlet/csadmin/UpdateNumberRange.java
@@ -187,20 +187,27 @@ public class UpdateNumberRange extends CMSServlet {
BigInteger decrement = new BigInteger(decrementStr, radix);
beginNum = endNum.subtract(decrement).add(oneNum);
- if (beginNum.compareTo(repo.peekNextSerialNumber()) < 0) {
- String nextEndNumStr = cs.getString(nextEndConfig, "");
- BigInteger endNum2 = new BigInteger(nextEndNumStr, radix);
- CMS.debug("Transferring from the end of on-deck range");
- String newValStr = endNum2.subtract(decrement).toString(radix);
- repo.setNextMaxSerial(newValStr);
- cs.putString(nextEndConfig, newValStr);
- beginNum = endNum2.subtract(decrement).add(oneNum);
- endNum = endNum2;
- } else {
- CMS.debug("Transferring from the end of the current range");
- String newValStr = beginNum.subtract(oneNum).toString(radix);
- repo.setMaxSerial(newValStr);
- cs.putString(endNumConfig, newValStr);
+ /* We need to synchronise on repo because we peek the next
+ * serial number, then set the max serial of the current or
+ * next range. If we don't synchronize, we could end up
+ * using serial numbers that were transferred.
+ */
+ synchronized (repo) {
+ if (beginNum.compareTo(repo.peekNextSerialNumber()) < 0) {
+ String nextEndNumStr = cs.getString(nextEndConfig, "");
+ BigInteger endNum2 = new BigInteger(nextEndNumStr, radix);
+ CMS.debug("Transferring from the end of on-deck range");
+ String newValStr = endNum2.subtract(decrement).toString(radix);
+ repo.setNextMaxSerial(newValStr);
+ cs.putString(nextEndConfig, newValStr);
+ beginNum = endNum2.subtract(decrement).add(oneNum);
+ endNum = endNum2;
+ } else {
+ CMS.debug("Transferring from the end of the current range");
+ String newValStr = beginNum.subtract(oneNum).toString(radix);
+ repo.setMaxSerial(newValStr);
+ cs.putString(endNumConfig, newValStr);
+ }
}
if (beginNum == null) {
diff --git a/base/server/cmscore/src/com/netscape/cmscore/dbs/CertificateRepository.java b/base/server/cmscore/src/com/netscape/cmscore/dbs/CertificateRepository.java
index 367917f..94087c8 100644
--- a/base/server/cmscore/src/com/netscape/cmscore/dbs/CertificateRepository.java
+++ b/base/server/cmscore/src/com/netscape/cmscore/dbs/CertificateRepository.java
@@ -251,49 +251,45 @@ public class CertificateRepository extends Repository
return nextSerialNumber;
}
- private Object nextSerialNumberMonitor = new Object();
-
- public BigInteger getNextSerialNumber() throws
+ public synchronized BigInteger getNextSerialNumber() throws
EBaseException {
BigInteger nextSerialNumber = null;
BigInteger randomNumber = null;
- synchronized (nextSerialNumberMonitor) {
- super.initCacheIfNeeded();
- CMS.debug("CertificateRepository: getNextSerialNumber mEnableRandomSerialNumbers="+mEnableRandomSerialNumbers);
+ super.initCacheIfNeeded();
+ CMS.debug("CertificateRepository: getNextSerialNumber mEnableRandomSerialNumbers="+mEnableRandomSerialNumbers);
- if (mEnableRandomSerialNumbers) {
- int i = 0;
- do {
- if (i > 0) {
- CMS.debug("CertificateRepository: getNextSerialNumber regenerating serial number");
- }
- randomNumber = getRandomNumber();
- nextSerialNumber = getRandomSerialNumber(randomNumber);
- nextSerialNumber = checkSerialNumbers(randomNumber, nextSerialNumber);
- i++;
- } while (nextSerialNumber == null && i < mMaxCollisionRecoveryRegenerations);
-
- if (nextSerialNumber == null) {
- CMS.debug("CertificateRepository: in getNextSerialNumber nextSerialNumber is null");
- throw new EBaseException( "nextSerialNumber is null" );
+ if (mEnableRandomSerialNumbers) {
+ int i = 0;
+ do {
+ if (i > 0) {
+ CMS.debug("CertificateRepository: getNextSerialNumber regenerating serial number");
}
+ randomNumber = getRandomNumber();
+ nextSerialNumber = getRandomSerialNumber(randomNumber);
+ nextSerialNumber = checkSerialNumbers(randomNumber, nextSerialNumber);
+ i++;
+ } while (nextSerialNumber == null && i < mMaxCollisionRecoveryRegenerations);
- if (mCounter.compareTo(BigInteger.ZERO) >= 0 &&
- mMinSerialNo != null && mMaxSerialNo != null &&
- nextSerialNumber != null &&
- nextSerialNumber.compareTo(mMinSerialNo) >= 0 &&
- nextSerialNumber.compareTo(mMaxSerialNo) <= 0) {
- mCounter = mCounter.add(BigInteger.ONE);
- }
- CMS.debug("CertificateRepository: getNextSerialNumber nextSerialNumber="+
- nextSerialNumber+" mCounter="+mCounter);
+ if (nextSerialNumber == null) {
+ CMS.debug("CertificateRepository: in getNextSerialNumber nextSerialNumber is null");
+ throw new EBaseException( "nextSerialNumber is null" );
+ }
- super.checkRange();
- } else {
- nextSerialNumber = super.getNextSerialNumber();
+ if (mCounter.compareTo(BigInteger.ZERO) >= 0 &&
+ mMinSerialNo != null && mMaxSerialNo != null &&
+ nextSerialNumber != null &&
+ nextSerialNumber.compareTo(mMinSerialNo) >= 0 &&
+ nextSerialNumber.compareTo(mMaxSerialNo) <= 0) {
+ mCounter = mCounter.add(BigInteger.ONE);
}
+ CMS.debug("CertificateRepository: getNextSerialNumber nextSerialNumber="+
+ nextSerialNumber+" mCounter="+mCounter);
+
+ super.checkRange();
+ } else {
+ nextSerialNumber = super.getNextSerialNumber();
}
return nextSerialNumber;
diff --git a/base/server/cmscore/src/com/netscape/cmscore/dbs/Repository.java b/base/server/cmscore/src/com/netscape/cmscore/dbs/Repository.java
index 9bc7e2a..c31d376 100644
--- a/base/server/cmscore/src/com/netscape/cmscore/dbs/Repository.java
+++ b/base/server/cmscore/src/com/netscape/cmscore/dbs/Repository.java
@@ -185,7 +185,7 @@ public abstract class Repository implements IRepository {
* @param serial maximum number
* @exception EBaseException failed to set maximum serial number
*/
- public void setMaxSerial(String serial) throws EBaseException {
+ public synchronized void setMaxSerial(String serial) throws EBaseException {
BigInteger maxSerial = null;
CMS.debug("Repository:setMaxSerial " + serial);
@@ -211,7 +211,7 @@ public abstract class Repository implements IRepository {
* @param serial maximum number in next range
* @exception EBaseException failed to set maximum serial number in next range
*/
- public void setNextMaxSerial(String serial) throws EBaseException {
+ public synchronized void setNextMaxSerial(String serial) throws EBaseException {
BigInteger maxSerial = null;
CMS.debug("Repository:setNextMaxSerial " + serial);
@@ -346,7 +346,7 @@ public abstract class Repository implements IRepository {
* Returns null if the next number exceeds the current range and
* there is not a next range.
*/
- public BigInteger peekNextSerialNumber() throws EBaseException {
+ public synchronized BigInteger peekNextSerialNumber() throws EBaseException {
CMS.debug("Repository:In getTheSerialNumber ");
if (mLastSerialNo == null)
--
1.8.3.1