Radek Kraus created AMQ-7466:
--------------------------------
Summary: JmsSecurityException is not propagated back to client
Key: AMQ-7466
URL: https://issues.apache.org/jira/browse/AMQ-7466
Project: ActiveMQ
Issue Type: Bug
Components: Broker
Affects Versions: 5.15.9
Reporter: Radek Kraus
It seems there is a "condition race", which caused, that
{{JmsSecurityException}} exception is not sent back to the client side.
I use a custom authentication plugin (based on {{SimpleAuthenticationPlugin}})
on broker side. My test usually works as expected I got
{{JmsSecurityException}}, when
user/password is incorrect and all is OK, when user/password is correct.
It is very hard to simulated it (and I have no test to reproduce it, because I
didn't find a way how write it). So I can only describe, what probably happens
in wrong scenario:
* I use vm transport (but I think it does not matter)
* and now we expect following threads timing (see simplified pieces of current
sources bellow):
** Thread 1 enters into {{TransportConnection.start()}} method (invoked from
{{TransportAcceptListener}} at {{TransportConnector}} by "taskRunnerFactory"
from "brokerService")
** Thread 1 successfully starts the transport, but it is suspended on the
beginning of finally section (before {{if}} condition, but outside of
synchronization section)
** Thread 2 enters into
{{TransportConnection.processAddConnection(ConnectionInfo)}} (triggered by
client on create session, send {{ConnectionInfo}} -
{{ActiveMQConnection.ensureConnectionInfoSent()}})
** Thread 2 goes into catch block (because {{SecurityException}} is thrown -
wrong user/password combination)
** Thread 2 enters into {{TransportConnection.delayedStop(int, String,
Throwable)}} method, *where status is set to {{PENDING_STOP}}*
** Thread 1 is back now and continues with processing if condition - {{if
(!status.compareAndSet(STARTING, STARTED))}}
** Thread 1 *stops the transport, because status is {{PENDING_STOP}}*
* {{JmsSecurityException}} is not sent back to the client side :(
{code:java}
public void start() throws Exception {
if (status.compareAndSet(NEW, STARTING)) {
try {
synchronized (this) {
...
}
} catch (Exception e) {
// Force clean up on an error starting up.
status.set(PENDING_STOP);
throw e;
} finally {
if (!status.compareAndSet(STARTING, STARTED)) {
LOG.debug("Calling the delayed stop() after start() {}", this);
stop();
}
}
}
}
{code}
{code:java}
public Response processAddConnection(ConnectionInfo info) throws Exception {
...
try {
broker.addConnection(context, info);
} catch (Exception e) {
synchronized (brokerConnectionStates) {
brokerConnectionStates.remove(info.getConnectionId());
}
unregisterConnectionState(info.getConnectionId());
LOG.warn("Failed to add Connection id={}, clientId={} due to {}",
info.getConnectionId(), clientId, e);
//AMQ-6561 - stop for all exceptions on addConnection
// close this down - in case the peer of this transport doesn't play nice
delayedStop(2000, "Failed with SecurityException: " +
e.getLocalizedMessage(), e);
throw e;
}
...
}
{code}
{code:java}
public void delayedStop(final int waitTime, final String reason, Throwable
cause) {
if (waitTime > 0) {
status.compareAndSet(STARTING, PENDING_STOP);
...
}
}
{code}
Unfortunately I can simulate it only in debugger, I am very sorry. But I think
it is a problem. Or I am completely wrong?
In a reality I have no such deep knowledge to prepare some fix, but I think,
that the solution can be to set status to {{STARTING}} inside of
synchronization section, something like this, but I am not really sure about
this solution:
{code:java}
public void start() throws Exception {
if (status.compareAndSet(NEW, STARTING)) {
boolean isStarted = false;
try {
synchronized (this) {
try {
...
} catch (Exception e) {
// Force clean up on an error starting up.
status.set(PENDING_STOP);
throw e;
} finally {
isStarted = status.compareAndSet(STARTING, STARTED);
}
}
} finally {
// stop() can be called from within the above block,
// but we want to be sure start() completes before
// stop() runs, so queue the stop until right now:
if (!isStarted) {
LOG.debug("Calling the delayed stop() after start() {}", this);
stop();
}
}
}
}
{code}
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
