From c7c10dfc36437fdf724f3ae6515cfe9478b7408d Mon Sep 17 00:00:00 2001 From: Robert Varga Date: Tue, 28 Mar 2017 17:29:16 +0200 Subject: [PATCH] BUG-7901: fix unsynchronized transaction access The evidence in the issue indicates that we have a race condition where a batch of modifications is being submitted (triggered by a write/delete and exceeded batch count) and we are observing a concurrent modification. After auditing the CDS side of this, while we can improve safety a bit, the scenario has to involve multi-threaded access by the client. Looking at TransactionChainManager, it seems to be heavily synchronized, but there is a hole in the paths which are using getTransactionSafely(). While the access to the TransactionChain, it returns the transaction (which really is an alias to the wTx field). That throws of static analysis, as we end up the object pointed to by wTx without holding the lock -- hence concurrent threads will end up touching the transaction without any guards, leading to a violation of Transaction contract (which requires external synchronization). This patch reworks getTransactionSafely() so it only ensures the field is initialized as appropriate and makes its callers access the wTx field with the proper lock held. Bug: 7500 Bug: 7901 Bug: 8060 Change-Id: I058feb55f91a40ccda6d69ee87cd1a20f792afba Signed-off-by: Robert Varga (cherry picked from commit 6dbbd412a88dafdb7d7acd824516850b473b145f) --- .../impl/device/TransactionChainManager.java | 76 ++++++++----------- 1 file changed, 33 insertions(+), 43 deletions(-) diff --git a/openflowplugin-impl/src/main/java/org/opendaylight/openflowplugin/impl/device/TransactionChainManager.java b/openflowplugin-impl/src/main/java/org/opendaylight/openflowplugin/impl/device/TransactionChainManager.java index da4bdae04e..aded9b26d9 100644 --- a/openflowplugin-impl/src/main/java/org/opendaylight/openflowplugin/impl/device/TransactionChainManager.java +++ b/openflowplugin-impl/src/main/java/org/opendaylight/openflowplugin/impl/device/TransactionChainManager.java @@ -64,7 +64,7 @@ class TransactionChainManager implements TransactionChainListener, AutoCloseable @GuardedBy("txLock") private ListenableFuture lastSubmittedFuture; - private boolean initCommit; + private volatile boolean initCommit; @GuardedBy("txLock") private TransactionChainManagerStatus transactionChainManagerStatus = TransactionChainManagerStatus.SLEEPING; @@ -73,7 +73,6 @@ class TransactionChainManager implements TransactionChainListener, AutoCloseable @Nonnull final DeviceInfo deviceInfo) { this.dataBroker = dataBroker; this.nodeId = deviceInfo.getNodeInstanceIdentifier().getKey().getId().getValue(); - this.transactionChainManagerStatus = TransactionChainManagerStatus.SLEEPING; this.lastSubmittedFuture = Futures.immediateFuture(null); } @@ -103,7 +102,7 @@ class TransactionChainManager implements TransactionChainListener, AutoCloseable LOG.debug("activateTransactionManager for node {} transaction submit is set to {}", this.nodeId, submitIsEnabled); } synchronized (txLock) { - if (TransactionChainManagerStatus.SLEEPING.equals(transactionChainManagerStatus)) { + if (TransactionChainManagerStatus.SLEEPING == transactionChainManagerStatus) { Preconditions.checkState(txChainFactory == null, "TxChainFactory survive last close."); Preconditions.checkState(wTx == null, "We have some unexpected WriteTransaction."); this.transactionChainManagerStatus = TransactionChainManagerStatus.WORKING; @@ -126,7 +125,7 @@ class TransactionChainManager implements TransactionChainListener, AutoCloseable } final ListenableFuture future; synchronized (txLock) { - if (TransactionChainManagerStatus.WORKING.equals(transactionChainManagerStatus)) { + if (TransactionChainManagerStatus.WORKING == transactionChainManagerStatus) { transactionChainManagerStatus = TransactionChainManagerStatus.SLEEPING; future = txChainShuttingDown(); Preconditions.checkState(wTx == null, "We have some unexpected WriteTransaction."); @@ -168,15 +167,16 @@ class TransactionChainManager implements TransactionChainListener, AutoCloseable } return true; } - Preconditions.checkState(TransactionChainManagerStatus.WORKING.equals(transactionChainManagerStatus), + Preconditions.checkState(TransactionChainManagerStatus.WORKING == transactionChainManagerStatus, "we have here Uncompleted Transaction for node {} and we are not MASTER", this.nodeId); final CheckedFuture submitFuture = wTx.submit(); + lastSubmittedFuture = submitFuture; + wTx = null; + Futures.addCallback(submitFuture, new FutureCallback() { @Override public void onSuccess(final Void result) { - if (initCommit) { - initCommit = false; - } + initCommit = false; } @Override @@ -192,27 +192,24 @@ class TransactionChainManager implements TransactionChainListener, AutoCloseable } } if (initCommit) { - wTx = null; Optional.ofNullable(lifecycleService).ifPresent(LifecycleService::closeConnection); } } }); - lastSubmittedFuture = submitFuture; - wTx = null; } return true; } void addDeleteOperationTotTxChain(final LogicalDatastoreType store, final InstanceIdentifier path){ - final WriteTransaction writeTx = getTransactionSafely(); - if (Objects.nonNull(writeTx)) { - writeTx.delete(store, path); - } else { - if (LOG.isDebugEnabled()) { + synchronized (txLock) { + ensureTransaction(); + if (wTx == null) { LOG.debug("WriteTx is null for node {}. Delete {} was not realized.", this.nodeId, path); + throw new TransactionChainClosedException(CANNOT_WRITE_INTO_TRANSACTION); } - throw new TransactionChainClosedException(CANNOT_WRITE_INTO_TRANSACTION); + + wTx.delete(store, path); } } @@ -220,23 +217,26 @@ class TransactionChainManager implements TransactionChainListener, AutoCloseable final InstanceIdentifier path, final T data, final boolean createParents){ - final WriteTransaction writeTx = getTransactionSafely(); - if (Objects.nonNull(writeTx)) { - writeTx.put(store, path, data, createParents); - } else { - if (LOG.isDebugEnabled()) { + synchronized (txLock) { + ensureTransaction(); + if (wTx == null) { LOG.debug("WriteTx is null for node {}. Write data for {} was not realized.", this.nodeId, path); + throw new TransactionChainClosedException(CANNOT_WRITE_INTO_TRANSACTION); } - throw new TransactionChainClosedException(CANNOT_WRITE_INTO_TRANSACTION); + + wTx.put(store, path, data, createParents); } } @Override public void onTransactionChainFailed(final TransactionChain chain, final AsyncTransaction transaction, final Throwable cause) { - if (transactionChainManagerStatus.equals(TransactionChainManagerStatus.WORKING)) { - LOG.warn("Transaction chain failed, recreating chain due to ", cause); - recreateTxChain(); + synchronized (txLock) { + if (TransactionChainManagerStatus.WORKING == transactionChainManagerStatus) { + LOG.warn("Transaction chain failed, recreating chain due to ", cause); + createTxChain(); + wTx = null; + } } } @@ -245,21 +245,13 @@ class TransactionChainManager implements TransactionChainListener, AutoCloseable // NOOP } - private void recreateTxChain() { - synchronized (txLock) { - createTxChain(); - wTx = null; - } - } - + @GuardedBy("txLock") @Nullable - private WriteTransaction getTransactionSafely() { - synchronized (txLock) { - if (wTx == null && TransactionChainManagerStatus.WORKING.equals(transactionChainManagerStatus)) { - Optional.ofNullable(txChainFactory).ifPresent(bindingTransactionChain -> wTx = txChainFactory.newWriteOnlyTransaction()); - } - } - return wTx; + private void ensureTransaction() { + if (wTx == null && TransactionChainManagerStatus.WORKING == transactionChainManagerStatus + && txChainFactory != null) { + wTx = txChainFactory.newWriteOnlyTransaction(); + } } @VisibleForTesting @@ -274,12 +266,10 @@ class TransactionChainManager implements TransactionChainListener, AutoCloseable if (LOG.isDebugEnabled()) { LOG.debug("TxManager is going SHUTTING_DOWN for node {}", this.nodeId); } - ListenableFuture future; synchronized (txLock) { this.transactionChainManagerStatus = TransactionChainManagerStatus.SHUTTING_DOWN; - future = txChainShuttingDown(); + return txChainShuttingDown(); } - return future; } @GuardedBy("txLock") -- 2.36.6