[
https://issues.apache.org/jira/browse/SSHD-1050?focusedWorklogId=466179&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-466179
]
ASF GitHub Bot logged work on SSHD-1050:
----------------------------------------
Author: ASF GitHub Bot
Created on: 04/Aug/20 11:36
Start Date: 04/Aug/20 11:36
Worklog Time Spent: 10m
Work Description: 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]
Issue Time Tracking
-------------------
Worklog Id: (was: 466179)
Time Spent: 20m (was: 10m)
> Race condition between early exceptions and AuthFuture
> ------------------------------------------------------
>
> Key: SSHD-1050
> URL: https://issues.apache.org/jira/browse/SSHD-1050
> Project: MINA SSHD
> Issue Type: Bug
> Affects Versions: 2.4.0, 2.6.0
> Reporter: Thomas Wolf
> Assignee: Lyor Goldstein
> Priority: Major
> Time Spent: 20m
> Remaining Estimate: 0h
>
> It appears that sometimes exceptions that occur early in connection setup are
> not reported on the AuthFuture. When that happens, AuthFuture.verify(timeout)
> will spend the whole timeout waiting and then report a timeout. The earlier
> exception is lost and nowhere to be seen.
> I stumbled over this when analyzing [Eclipse bug
> 565394|https://bugs.eclipse.org/bugs/show_bug.cgi?id=565394].
> It's not easy to reproduce, but with the below client test I can manage to
> make the test fail from time to time if run repeatedly. (That test uses a
> large preamble before the server identification to provoke an early
> exception. Using publickey auth instead of password auth increases the
> chances to get a failure. Running in a debugger increases the chances even
> more. It's clearly timing-related.)
> {code:java}
> private String longPreamble() {
> StringBuilder b = new StringBuilder();
> for (int i = 0; i < 250; i++) {
> b.append('a');
> }
> String line = b.toString();
> b = new StringBuilder(line);
> int limit = CoreModuleProperties.MAX_IDENTIFICATION_SIZE.get(sshd)
> .orElse(Integer.valueOf(16 * 1024)).intValue();
> limit = limit / 250 + 1;
> for (int i = 0; i < limit; i++) {
> b.append(CoreModuleProperties.SERVER_EXTRA_IDENT_LINES_SEPARATOR)
> .append(line);
> }
> return b.toString();
> }
> @Test
> public void testAuthGetsNotifiedOnLongPreamble() throws Exception {
> CoreModuleProperties.SERVER_EXTRA_IDENTIFICATION_LINES.set(sshd,
> longPreamble());
> sshd.setPasswordAuthenticator(RejectAllPasswordAuthenticator.INSTANCE);
>
> sshd.setKeyboardInteractiveAuthenticator(KeyboardInteractiveAuthenticator.NONE);
> client.setUserAuthFactories(
> Collections.singletonList(UserAuthPublicKeyFactory.INSTANCE));
> client.start();
> try (ClientSession session =
> client.connect(getCurrentTestName(), TEST_LOCALHOST, port)
> .verify(CONNECT_TIMEOUT).getSession()) {
> KeyPairProvider keys = createTestHostKeyProvider();
> session.addPublicKeyIdentity(
> keys.loadKey(session,
> CommonTestSupportUtils.DEFAULT_TEST_HOST_KEY_TYPE));
> // This auth should fail because the server sends too many lines before
> // the server identification. However, the auth must not time out!
> There's
> // an exception raised early on when the identification is read, but
> // frequently this exception gets not reported on the auth future that
> // we are waiting on here, and then we wait for the whole timeout.
> //
> // There's a race condition somewhere. The test succeeds frequently, and
> is
> // hard to make fail, but if run enough times it will usually fail.
> Running
> // this in a debugger increases the chances of it failing.
> Throwable e = assertThrows(Throwable.class,
> () -> session.auth().verify(AUTH_TIMEOUT));
> assertFalse(e.getMessage().contains("timeout"));
> assertFalse(session.isAuthenticated());
> } finally {
> client.stop();
> }
> }
> {code}
> Perhaps the connect future should be fulfilled only once the initial KEX is
> done? OpenSSH's ConnectTimeout does include the time until after the initial
> KEX.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
