This is an automated email from the ASF dual-hosted git repository.
lgoldstein pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/mina-sshd.git
commit 3d280750c6dd817c63948d4c10a4ad991d901eb3
Author: Thomas Wolf <thomas.w...@paranor.ch>
AuthorDate: Tue Aug 4 21:04:59 2020 +0200
[SSHD-1050] Fix race conditions with early access to AuthFuture
Make ClientSessionImpl.authFuture volatile since it may be accessed by
different threads without synchronization.
Remove the initial dummy AuthFuture that served only to record an early
exception, and to provide a correct session state in waitFor() when KEX
was done but auth() not called yet.
Record exceptions from signalAuthFailure() in a list if there's no
AuthFuture yet. Introduce a lock for guarding the authFuture field and
this new list of exceptions together.
The access in updateCurrentSessionState() must remain unsynchronized
because it occurs while the futureLock is held. Synchronizing there
on errorLock might introduce a lock inversion between that access in
updateCurrentSessionState() (lock order futureLock-errorLock), but
the call to authService.auth() inside ClientSessionImpl.auth() has the
potential to also access the futureLock is there was a previous future,
which might give the lock order errorLock-futureLock.
Listeners futures are always called outside the future's lock, so even
if a listener calls auth() or signalAuthFailure(), this will
not cause a lock inversion.
---
.../sshd/client/future/InitialAuthFuture.java | 52 -----------
.../sshd/client/session/ClientSessionImpl.java | 103 +++++++++++++--------
2 files changed, 64 insertions(+), 91 deletions(-)
diff --git
a/sshd-core/src/main/java/org/apache/sshd/client/future/InitialAuthFuture.java
b/sshd-core/src/main/java/org/apache/sshd/client/future/InitialAuthFuture.java
deleted file mode 100644
index 375fdb4..0000000
---
a/sshd-core/src/main/java/org/apache/sshd/client/future/InitialAuthFuture.java
+++ /dev/null
@@ -1,52 +0,0 @@
-/*
- * 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.sshd.client.future;
-
-import org.apache.sshd.common.future.SshFutureListener;
-
-/**
- * A special future used until the authentication service replaces it with a
"real" one. This future serves as
- * a placeholder for any exceptions caught until authentication starts. It is
special in one other way - it has no
- * listeners attached to it and expects no authentication success or failure -
only exceptions.
- *
- * @author <a href="mailto:d...@mina.apache.org";>Apache MINA SSHD Project</a>
- */
-public class InitialAuthFuture extends DefaultAuthFuture {
- public InitialAuthFuture(Object id, Object lock) {
- super(id, lock);
- }
-
- @Override
- public AuthFuture addListener(SshFutureListener<AuthFuture> listener) {
- throw new UnsupportedOperationException("Not allowed to add listeners
to this future");
- }
-
- @Override
- protected void notifyListener(SshFutureListener<AuthFuture> l) {
- if (l != null) {
- throw new UnsupportedOperationException("No listeners expected for
this future");
- }
- }
-
- @Override
- public void setAuthed(boolean authed) {
- throw new UnsupportedOperationException("No authentication expected
for this future");
- }
-}
diff --git
a/sshd-core/src/main/java/org/apache/sshd/client/session/ClientSessionImpl.java
b/sshd-core/src/main/java/org/apache/sshd/client/session/ClientSessionImpl.java
index bc8a9bd..9bc68a9 100644
---
a/sshd-core/src/main/java/org/apache/sshd/client/session/ClientSessionImpl.java
+++
b/sshd-core/src/main/java/org/apache/sshd/client/session/ClientSessionImpl.java
@@ -19,11 +19,13 @@
package org.apache.sshd.client.session;
import java.io.IOException;
+import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
import java.util.Collections;
import java.util.EnumSet;
import java.util.HashMap;
+import java.util.Iterator;
import java.util.List;
import java.util.Map;
import java.util.Objects;
@@ -32,7 +34,6 @@ import java.util.concurrent.TimeUnit;
import org.apache.sshd.client.ClientFactoryManager;
import org.apache.sshd.client.future.AuthFuture;
-import org.apache.sshd.client.future.InitialAuthFuture;
import org.apache.sshd.common.Service;
import org.apache.sshd.common.ServiceFactory;
import org.apache.sshd.common.SshConstants;
@@ -50,7 +51,20 @@ import org.apache.sshd.common.util.buffer.Buffer;
* @author <a href="mailto:d...@mina.apache.org";>Apache MINA SSHD Project</a>
*/
public class ClientSessionImpl extends AbstractClientSession {
- private AuthFuture authFuture;
+
+ /**
+ * The authentication future created by the last call to {@link #auth()};
{@code null} before the first call to
+ * {@link #auth()}.
+ *
+ * Volatile because of unsynchronized access in {@link
#updateCurrentSessionState(Collection)}.
+ */
+ private volatile AuthFuture authFuture;
+
+ /** Records exceptions before there is an authFuture. */
+ private List<Throwable> earlyErrors = new ArrayList<>();
+
+ /** Guards setting an earlyError and the authFuture together. */
+ private final Object errorLock = new Object();
/**
* For clients to store their own metadata
@@ -85,8 +99,6 @@ public class ClientSessionImpl extends AbstractClientSession {
nextServiceFactory = null;
}
- authFuture = new InitialAuthFuture(ioSession.getRemoteAddress(),
futureLock);
-
signalSessionCreated(ioSession);
/*
@@ -121,63 +133,73 @@ public class ClientSessionImpl extends
AbstractClientSession {
}
ClientUserAuthService authService = getUserAuthService();
- synchronized (sessionLock) {
- String serviceName = nextServiceName();
- // SSHD-1050 - check if need to propagate any previously caught
exceptions
- Throwable caught = (authFuture instanceof InitialAuthFuture) ?
authFuture.getException() : null;
- authFuture = ValidateUtils.checkNotNull(
+ String serviceName = nextServiceName();
+ List<Throwable> errors;
+ AuthFuture future;
+ // Guard both getting early errors and setting authFuture
+ synchronized (errorLock) {
+ future = ValidateUtils.checkNotNull(
authService.auth(serviceName), "No auth future generated
by service=%s", serviceName);
- if (caught != null) {
- /*
- * Safe to do under session lock despite SSHD-916 since
- * the newly generated future has no associated listeners
- * attached to it yet
- */
- signalAuthFailure(authFuture, caught);
+ errors = earlyErrors;
+ earlyErrors = null;
+ authFuture = future;
+ }
+ if (errors != null && !errors.isEmpty()) {
+ Iterator<Throwable> iter = errors.iterator();
+ Throwable first = iter.next();
+ iter.forEachRemaining(t -> {
+ if (t != first && t != null) {
+ first.addSuppressed(t);
+ }
+ });
+ future.setException(first);
+ if (log.isDebugEnabled()) {
+ log.debug("auth({}) early exception type={}: {}", //$NON-NLS-1$
+ this, first.getClass().getSimpleName(),
+ first.getMessage());
}
-
- return authFuture;
+ // Or throw directly:
+ // throw new IOException(first.getMessage(), first);
}
+ return future;
}
@Override
public void exceptionCaught(Throwable t) {
- signalAuthFailure(authFuture, t);
+ signalAuthFailure(t);
super.exceptionCaught(t);
}
@Override
protected void preClose() {
- signalAuthFailure(authFuture, new SshException("Session is being
closed"));
+ signalAuthFailure(new SshException("Session is being closed"));
super.preClose();
}
@Override
protected void handleDisconnect(int code, String msg, String lang, Buffer
buffer) throws Exception {
- signalAuthFailure(authFuture, new SshException(code, msg));
+ signalAuthFailure(new SshException(code, msg));
super.handleDisconnect(code, msg, lang, buffer);
}
- protected void signalAuthFailure(AuthFuture future, Throwable t) {
- boolean signalled = false;
- if (future != null) {
- /*
- * SSHD-916 - do not synchronize on the session lock since the
result of setting an exception may be an
- * exchange of messages due to one of the registered listeners. If
this is the case then a deadlock will
- * occur since the session's message handling loop also tries to
lock the session lock - but on another
- * thread.
- */
- synchronized (future) {
- if (!future.isDone()) {
- future.setException(t);
- signalled = true;
+ protected void signalAuthFailure(Throwable t) {
+ AuthFuture future = authFuture;
+ if (future == null) {
+ synchronized (errorLock) {
+ if (earlyErrors != null) {
+ earlyErrors.add(t);
}
+ future = authFuture;
}
}
-
+ if (future != null) {
+ future.setException(t);
+ }
if (log.isDebugEnabled()) {
- log.debug("signalAuthFailure({}) type={}, signalled={}: {}",
- this, t.getClass().getSimpleName(), signalled,
t.getMessage());
+ boolean signalled = future != null && t == future.getException();
+ log.debug("signalAuthFailure({}) type={}, signalled={}: {}", this,
+ t.getClass().getSimpleName(), Boolean.valueOf(signalled),
+ t.getMessage());
}
}
@@ -319,8 +341,11 @@ public class ClientSessionImpl extends
AbstractClientSession {
if (isAuthenticated()) { // authFuture.isSuccess()
state.add(ClientSessionEvent.AUTHED);
}
- if (KexState.DONE.equals(kexState.get()) && authFuture.isFailure()) {
- state.add(ClientSessionEvent.WAIT_AUTH);
+ if (KexState.DONE.equals(kexState.get())) {
+ AuthFuture future = authFuture;
+ if (future == null || future.isFailure()) {
+ state.add(ClientSessionEvent.WAIT_AUTH);
+ }
}
return state;
