tomaswolf commented on a change in pull request #156:
URL: https://github.com/apache/mina-sshd/pull/156#discussion_r464964829
##########
File path:
sshd-core/src/main/java/org/apache/sshd/client/session/ClientSessionImpl.java
##########
@@ -124,8 +123,19 @@ public AuthFuture auth() throws IOException {
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;
Review comment:
Hmmm. Here we're protected by the sessionLock, but signalAuthFailure
synchronizes on the future. Three points here:
1. I don't think signalAuthFailure needs to synchronize at all. It could
just do
```
if (future != null) {
future.setException(t);
signalled = t == future.getException();
}
```
Or am I missing something?
2. There still might be concurrency issues here. If a concurrent
signalAuthFailure sets the exception on the initial authFuture after line 127
has been executed but before the assignment in line 128, the exception would
still be lost. At the very least this would need to be
```
AuthFuture previous = authFuture;
authFuture = ValidateUtils.checkNotNull(
authService.auth(serviceName), "No auth future generated by service=%s",
serviceName);
Throwable caught = (previous instanceof InitialAuthFuture) ?
previous.getException() : null;
if (caught != null) {
...
```
But even that won't help yet since a concurrent signalAuthFailure might
still get the value of authFuture before the assignment (thus getting previous)
but execute the setException() after our getException().
3. Since several threads may access authFuture, it should be at least
volatile. The synch on sessionLock does protect changing the field, but there's
numerous unsynchronized accesses to it. So what's the purpose of synching here?
In summary, I don't think this is correct. Part of the problem is the
additional indirection through that InitialAuthFuture, and missing synch on
authFuture.
I'll try to come up with something later.
##########
File path:
sshd-core/src/main/java/org/apache/sshd/client/session/ClientSessionImpl.java
##########
@@ -124,8 +123,19 @@ public AuthFuture auth() throws IOException {
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(
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);
Review comment:
I wonder... how about throwing the exception right away? (Wrapped in an
IOException.)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
