[jira] [Commented] (AMQ-7466) JmsSecurityException is not propagated back to client

["Radek Kraus (Jira)" <ji...@apache.org>]

    [ 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)