You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@activemq.apache.org by "Radek Kraus (Jira)" <ji...@apache.org> on 2021/02/12 12:18:00 UTC
[jira] [Updated] (AMQ-7466) JmsSecurityException is not propagated
back to client
[ https://issues.apache.org/jira/browse/AMQ-7466?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Radek Kraus updated AMQ-7466:
-----------------------------
Attachment: vm-transport-exception.txt
vm-transport-amq-log.txt
tcp-transport-exception.txt
tcp-transport-amq-log.txt
> 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)