[
https://issues.apache.org/jira/browse/AMQ-7466?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17283644#comment-17283644
]
Radek Kraus commented on AMQ-7466:
----------------------------------
Attached AMQ log files and received exceptions (tcp transport, vm transport).
> 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
> Assignee: Jean-Baptiste Onofré
> Priority: Major
> Attachments: tcp-transport-amq-log.txt, tcp-transport-exception.txt,
> vm-transport-amq-log.txt, vm-transport-exception.txt
>
>
> 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)
