From 4cc0f1d12996df636b52aff130c5c26257d8aa5b Mon Sep 17 00:00:00 2001 From: Paul Schaub Date: Sun, 30 Aug 2020 23:08:26 +0200 Subject: [PATCH 1/8] Bump pgpainless version to 0.1.0 --- smack-openpgp/build.gradle | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/smack-openpgp/build.gradle b/smack-openpgp/build.gradle index 23d15a366..e083a2df6 100644 --- a/smack-openpgp/build.gradle +++ b/smack-openpgp/build.gradle @@ -8,7 +8,7 @@ dependencies { compile project(':smack-extensions') compile project(':smack-experimental') - api 'org.pgpainless:pgpainless-core:0.0.1-alpha11' + api 'org.pgpainless:pgpainless-core:0.1.0' testImplementation "org.bouncycastle:bcprov-jdk15on:${bouncyCastleVersion}" From b7824f008d27d1f19ce8aa09cc1d6b4ebf0f3c2c Mon Sep 17 00:00:00 2001 From: Florian Schmaus Date: Thu, 17 Sep 2020 12:31:35 +0200 Subject: [PATCH 2/8] Introduce and use XmlStringBuilder.text() Smack currently does unnecessary escaping of XML text, where it escapes e.g. '"' to '"'. This bloats the stanza size, especially if JSON payloads are involved. Fixes SMACK-892 (although there are probably still places where XmlStringBuilder.escape() is used when XmlStringBuild.text() could have been used). --- .../java/org/jivesoftware/smack/packet/Message.java | 2 +- .../smack/packet/StandardExtensionElement.java | 6 ++++-- .../jivesoftware/smack/util/XmlStringBuilder.java | 9 ++++++++- .../org/jivesoftware/smack/packet/MessageTest.java | 13 +++++++++++++ .../json/packet/AbstractJsonPacketExtension.java | 4 ++-- .../smackx/chat_markers/MarkableExtensionTest.java | 2 +- 6 files changed, 29 insertions(+), 7 deletions(-) diff --git a/smack-core/src/main/java/org/jivesoftware/smack/packet/Message.java b/smack-core/src/main/java/org/jivesoftware/smack/packet/Message.java index a85f7c5b6..61e43552b 100644 --- a/smack-core/src/main/java/org/jivesoftware/smack/packet/Message.java +++ b/smack-core/src/main/java/org/jivesoftware/smack/packet/Message.java @@ -590,7 +590,7 @@ public final class Message extends MessageOrPresence public XmlStringBuilder toXML(XmlEnvironment enclosingXmlEnvironment) { XmlStringBuilder xml = new XmlStringBuilder(this, enclosingXmlEnvironment); xml.rightAngleBracket(); - xml.escape(message); + xml.text(message); xml.closeElement(getElementName()); return xml; } diff --git a/smack-core/src/main/java/org/jivesoftware/smack/packet/StandardExtensionElement.java b/smack-core/src/main/java/org/jivesoftware/smack/packet/StandardExtensionElement.java index fd7229b84..d78ce1845 100644 --- a/smack-core/src/main/java/org/jivesoftware/smack/packet/StandardExtensionElement.java +++ b/smack-core/src/main/java/org/jivesoftware/smack/packet/StandardExtensionElement.java @@ -1,6 +1,6 @@ /** * - * Copyright 2015-2019 Florian Schmaus. + * Copyright 2015-2020 Florian Schmaus. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -142,7 +142,9 @@ public final class StandardExtensionElement implements ExtensionElement { } xml.rightAngleBracket(); - xml.optEscape(text); + if (text != null) { + xml.text(text); + } if (elements != null) { for (Map.Entry entry : elements.entrySet()) { diff --git a/smack-core/src/main/java/org/jivesoftware/smack/util/XmlStringBuilder.java b/smack-core/src/main/java/org/jivesoftware/smack/util/XmlStringBuilder.java index c483db913..f32e8dc7c 100644 --- a/smack-core/src/main/java/org/jivesoftware/smack/util/XmlStringBuilder.java +++ b/smack-core/src/main/java/org/jivesoftware/smack/util/XmlStringBuilder.java @@ -1,6 +1,6 @@ /** * - * Copyright 2014-2019 Florian Schmaus + * Copyright 2014-2020 Florian Schmaus * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -457,6 +457,13 @@ public class XmlStringBuilder implements Appendable, CharSequence, Element { return this; } + public XmlStringBuilder text(CharSequence text) { + assert text != null; + CharSequence escapedText = StringUtils.escapeForXmlText(text); + sb.append(escapedText); + return this; + } + public XmlStringBuilder escape(String text) { assert text != null; sb.append(StringUtils.escapeForXml(text)); diff --git a/smack-core/src/test/java/org/jivesoftware/smack/packet/MessageTest.java b/smack-core/src/test/java/org/jivesoftware/smack/packet/MessageTest.java index 4c45be4fe..cba14e74f 100644 --- a/smack-core/src/test/java/org/jivesoftware/smack/packet/MessageTest.java +++ b/smack-core/src/test/java/org/jivesoftware/smack/packet/MessageTest.java @@ -206,4 +206,17 @@ public class MessageTest { assertXmlSimilar(control, message.toXML(StreamOpen.CLIENT_NAMESPACE).toString()); } + + /** + * Tests that only required characters are XML escaped in body. + * + * @see SMACK-892 + */ + @Test + public void escapeInBodyTest() { + String theFive = "\"'<>&"; + Message.Body body = new Message.Body(null, theFive); + + assertEquals("\"'<>&", body.toXML().toString()); + } } diff --git a/smack-experimental/src/main/java/org/jivesoftware/smackx/json/packet/AbstractJsonPacketExtension.java b/smack-experimental/src/main/java/org/jivesoftware/smackx/json/packet/AbstractJsonPacketExtension.java index 4d395debf..bcdc3f858 100644 --- a/smack-experimental/src/main/java/org/jivesoftware/smackx/json/packet/AbstractJsonPacketExtension.java +++ b/smack-experimental/src/main/java/org/jivesoftware/smackx/json/packet/AbstractJsonPacketExtension.java @@ -1,6 +1,6 @@ /** * - * Copyright © 2014 Florian Schmaus + * Copyright © 2014-2020 Florian Schmaus * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -35,7 +35,7 @@ public abstract class AbstractJsonPacketExtension implements ExtensionElement { public final XmlStringBuilder toXML(org.jivesoftware.smack.packet.XmlEnvironment enclosingNamespace) { XmlStringBuilder xml = new XmlStringBuilder(this); xml.rightAngleBracket(); - xml.append(json); + xml.text(json); xml.closeElement(this); return xml; } diff --git a/smack-experimental/src/test/java/org/jivesoftware/smackx/chat_markers/MarkableExtensionTest.java b/smack-experimental/src/test/java/org/jivesoftware/smackx/chat_markers/MarkableExtensionTest.java index 2b08e1c1b..ff4198006 100644 --- a/smack-experimental/src/test/java/org/jivesoftware/smackx/chat_markers/MarkableExtensionTest.java +++ b/smack-experimental/src/test/java/org/jivesoftware/smackx/chat_markers/MarkableExtensionTest.java @@ -33,7 +33,7 @@ import org.jxmpp.jid.impl.JidCreate; public class MarkableExtensionTest { String markableMessageStanza = "" - + "My lord, dispatch; read o'er these articles." + + "My lord, dispatch; read o'er these articles." + "" + ""; String markableExtension = ""; From 488d01796a5e64e9b62aecf58699daf1c3858f7c Mon Sep 17 00:00:00 2001 From: Florian Schmaus Date: Thu, 17 Sep 2020 21:07:35 +0200 Subject: [PATCH 3/8] [tcp] Fix TlsState by aborting the channel selected callback Instead of breaking in case the SSLEngine signals NEED_WRAP, which leads to an endless loop while holding the channelSelectedCallbackLock, we have to return, so that the asynchronously invoked callback can aquire it, and do its work. --- .../smack/tcp/XmppTcpTransportModule.java | 20 +++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/smack-tcp/src/main/java/org/jivesoftware/smack/tcp/XmppTcpTransportModule.java b/smack-tcp/src/main/java/org/jivesoftware/smack/tcp/XmppTcpTransportModule.java index 946eb4a1a..965ddb192 100644 --- a/smack-tcp/src/main/java/org/jivesoftware/smack/tcp/XmppTcpTransportModule.java +++ b/smack-tcp/src/main/java/org/jivesoftware/smack/tcp/XmppTcpTransportModule.java @@ -1066,18 +1066,25 @@ public class XmppTcpTransportModule extends ModularXmppClientToServerConnectionM SSLEngineResult.HandshakeStatus handshakeStatus = handleHandshakeStatus(result); switch (handshakeStatus) { case NEED_TASK: - // A delegated task is asynchronously running. Signal that there is pending input data and - // cycle again through the smack reactor. + // A delegated task is asynchronously running. Take care of the remaining accumulatedData. addAsPendingInputData(accumulatedData); - break; + // Return here, as the async task created by handleHandshakeStatus will continue calling the + // cannelSelectedCallback. + return null; case NEED_UNWRAP: continue; case NEED_WRAP: // NEED_WRAP means that the SSLEngine needs to send data, probably without consuming data. // We exploit here the fact that the channelSelectedCallback is single threaded and that the // input processing is after the output processing. + addAsPendingInputData(accumulatedData); + // Note that it is ok that we the provided argument for pending input filter data to channel + // selected callback is false, as setPendingInputFilterData() will have set the internal state + // boolean accordingly. connectionInternal.asyncGo(() -> callChannelSelectedCallback(false, true)); - break; + // Do not break here, but instead return and let the asynchronously invoked + // callChannelSelectedCallback() do its work. + return null; default: break; } @@ -1109,8 +1116,13 @@ public class XmppTcpTransportModule extends ModularXmppClientToServerConnectionM } private void addAsPendingInputData(ByteBuffer byteBuffer) { + // TODO: Why doeesn't simply + // pendingInputData = byteBuffer; + // work? pendingInputData = ByteBuffer.allocate(byteBuffer.remaining()); pendingInputData.put(byteBuffer).flip(); + + pendingInputFilterData = pendingInputData.hasRemaining(); } private SSLEngineResult.HandshakeStatus handleHandshakeStatus(SSLEngineResult sslEngineResult) { From 525ee09ea15d7843e20d2d376f343cd5fe8ab76a Mon Sep 17 00:00:00 2001 From: Florian Schmaus Date: Thu, 17 Sep 2020 23:04:55 +0200 Subject: [PATCH 4/8] [tcp] Do not send SM ack after we send a Do net put an ack to the queue if it has already been shutdown. Some servers, like ejabberd, like to request an ack even after we have send a stream close (and hance the queue was shutdown). If we would not check here, then the ack would dangle around in the queue, and be send on the next re-connection attempt even before the stream open. See the following trace of the MUC bookmarks integration test. The fact that it is a MUC test does not matter, but this test does disconnect the connection and reconnect it. Not how the server, ejabberd in this case, requests an SM ack by sending an even though we already send the : 22:22:05 SENT (4): Nick-P2VXD7 22:22:05 RECV (4): 22:22:05 SENT (4): 22:22:05 RECV (4): Nick-P2VXD7
22:22:05 RECV (4): 22:22:05 SENT (4): 22:22:05 RECV (4): 22:22:05 RECV (4): 22:22:05 XMPPConnection closed (XMPPTCPConnection[sinttest-7in7j-4@salem.geekplace.eu/1415073683806426185213090] (4)) 22:22:05 SENT (4): 22:22:05 SENT (4): 22:22:05 RECV (4): ?xml version='1.0'?> 22:22:05 XMPPConnection closed due to an exception (XMPPTCPConnection[sinttest-7in7j-4@salem.geekplace.eu/1415073683806426185213090] (4)) org.jivesoftware.smack.XMPPException$StreamErrorException: invalid-xml You can read more about the meaning of this stream error at http://xmpp.org/rfcs/rfc6120.html#streams-error-conditions at org.jivesoftware.smack.tcp.XMPPTCPConnection$PacketReader.parsePackets(XMPPTCPConnection.java:981) at org.jivesoftware.smack.tcp.XMPPTCPConnection$PacketReader.access$700(XMPPTCPConnection.java:913) at org.jivesoftware.smack.tcp.XMPPTCPConnection$PacketReader$1.run(XMPPTCPConnection.java:936) at java.base/java.lang.Thread.run(Thread.java:834) --- .../util/ArrayBlockingQueueWithShutdown.java | 24 +++++++++++++++++++ .../smack/tcp/XMPPTCPConnection.java | 7 +++++- 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/smack-core/src/main/java/org/jivesoftware/smack/util/ArrayBlockingQueueWithShutdown.java b/smack-core/src/main/java/org/jivesoftware/smack/util/ArrayBlockingQueueWithShutdown.java index 01938c43c..da61c4724 100644 --- a/smack-core/src/main/java/org/jivesoftware/smack/util/ArrayBlockingQueueWithShutdown.java +++ b/smack-core/src/main/java/org/jivesoftware/smack/util/ArrayBlockingQueueWithShutdown.java @@ -293,6 +293,30 @@ public class ArrayBlockingQueueWithShutdown extends AbstractQueue implemen } } + /** + * Put if the queue has not been shutdown yet. + * + * @param e the element to put into the queue. + * @return true if the element has been put into the queue, false if the queue was shutdown. + * @throws InterruptedException if the calling thread was interrupted. + * @since 4.4 + */ + public boolean putIfNotShutdown(E e) throws InterruptedException { + checkNotNull(e); + lock.lockInterruptibly(); + + try { + if (isShutdown) { + return false; + } + + putInternal(e, true); + return true; + } finally { + lock.unlock(); + } + } + public void putAll(Collection elements) throws InterruptedException { checkNotNull(elements); lock.lockInterruptibly(); diff --git a/smack-tcp/src/main/java/org/jivesoftware/smack/tcp/XMPPTCPConnection.java b/smack-tcp/src/main/java/org/jivesoftware/smack/tcp/XMPPTCPConnection.java index 2d283d802..aeb9d109b 100644 --- a/smack-tcp/src/main/java/org/jivesoftware/smack/tcp/XMPPTCPConnection.java +++ b/smack-tcp/src/main/java/org/jivesoftware/smack/tcp/XMPPTCPConnection.java @@ -1592,7 +1592,12 @@ public class XMPPTCPConnection extends AbstractXMPPConnection { } private void sendSmAcknowledgementInternal() throws NotConnectedException, InterruptedException { - packetWriter.sendStreamElement(new AckAnswer(clientHandledStanzasCount)); + AckAnswer ackAnswer = new AckAnswer(clientHandledStanzasCount); + // Do net put an ack to the queue if it has already been shutdown. Some servers, like ejabberd, like to request + // an ack even after we have send a stream close (and hance the queue was shutdown). If we would not check here, + // then the ack would dangle around in the queue, and be send on the next re-connection attempt even before the + // stream open. + packetWriter.queue.putIfNotShutdown(ackAnswer); } /** From 08fc0ba0b4ecb33e882a9f3c722f5425e8772f0d Mon Sep 17 00:00:00 2001 From: Florian Schmaus Date: Sun, 20 Sep 2020 13:06:02 +0200 Subject: [PATCH 5/8] [tcp] Improve pendingWriteInterestAfterRead code comment --- .../org/jivesoftware/smack/tcp/XmppTcpTransportModule.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/smack-tcp/src/main/java/org/jivesoftware/smack/tcp/XmppTcpTransportModule.java b/smack-tcp/src/main/java/org/jivesoftware/smack/tcp/XmppTcpTransportModule.java index 965ddb192..c944d5440 100644 --- a/smack-tcp/src/main/java/org/jivesoftware/smack/tcp/XmppTcpTransportModule.java +++ b/smack-tcp/src/main/java/org/jivesoftware/smack/tcp/XmppTcpTransportModule.java @@ -498,9 +498,9 @@ public class XmppTcpTransportModule extends ModularXmppClientToServerConnectionM pendingInputFilterData = false; } - // We have successfully read something. It is now possible that a filter is now also able to write - // additional data (for example SSLEngine). if (pendingWriteInterestAfterRead) { + // We have successfully read something and someone announced a write interest after a read. It is + // now possible that a filter is now also able to write additional data (for example SSLEngine). pendingWriteInterestAfterRead = false; newInterestedOps |= SelectionKey.OP_WRITE; } From 4db7d787f7e4b6e525c9482d7088f673ff795026 Mon Sep 17 00:00:00 2001 From: Florian Schmaus Date: Sun, 20 Sep 2020 13:11:29 +0200 Subject: [PATCH 6/8] [tcp] Add code comment why we have to copy the ByteBuffer --- .../jivesoftware/smack/tcp/XmppTcpTransportModule.java | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/smack-tcp/src/main/java/org/jivesoftware/smack/tcp/XmppTcpTransportModule.java b/smack-tcp/src/main/java/org/jivesoftware/smack/tcp/XmppTcpTransportModule.java index c944d5440..96ce53341 100644 --- a/smack-tcp/src/main/java/org/jivesoftware/smack/tcp/XmppTcpTransportModule.java +++ b/smack-tcp/src/main/java/org/jivesoftware/smack/tcp/XmppTcpTransportModule.java @@ -1030,12 +1030,15 @@ public class XmppTcpTransportModule extends ModularXmppClientToServerConnectionM } } + @SuppressWarnings("ReferenceEquality") @Override public ByteBuffer input(ByteBuffer inputData) throws SSLException { ByteBuffer accumulatedData; if (pendingInputData == null) { accumulatedData = inputData; } else { + assert pendingInputData != inputData; + int accumulatedDataBytes = pendingInputData.remaining() + inputData.remaining(); accumulatedData = ByteBuffer.allocate(accumulatedDataBytes); accumulatedData.put(pendingInputData) @@ -1116,9 +1119,11 @@ public class XmppTcpTransportModule extends ModularXmppClientToServerConnectionM } private void addAsPendingInputData(ByteBuffer byteBuffer) { - // TODO: Why doeesn't simply + // Note that we can not simply write // pendingInputData = byteBuffer; - // work? + // we have to copy the provided byte buffer, because it is possible that this byteBuffer is re-used by some + // higher layer. That is, here 'byteBuffer' is typically 'incomingBuffer', which is a direct buffer only + // allocated once per connection for performance reasons and hence re-used for read() calls. pendingInputData = ByteBuffer.allocate(byteBuffer.remaining()); pendingInputData.put(byteBuffer).flip(); From 6837c305e86593cf968dabf4fcfd45abb1ce05ec Mon Sep 17 00:00:00 2001 From: Florian Schmaus Date: Sun, 20 Sep 2020 13:57:09 +0200 Subject: [PATCH 7/8] Smack 4.4.0-beta2 --- version | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/version b/version index 093319f00..5fd890b6c 100644 --- a/version +++ b/version @@ -1 +1 @@ -4.4.0-beta2-SNAPSHOT +4.4.0-beta2 From 02341f6330e9b53938fba3c04f30785cee393fd1 Mon Sep 17 00:00:00 2001 From: Florian Schmaus Date: Sun, 20 Sep 2020 14:10:53 +0200 Subject: [PATCH 8/8] Smack 4.4.0-beta3-SNAPSHOT --- version | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/version b/version index 5fd890b6c..2bc346d33 100644 --- a/version +++ b/version @@ -1 +1 @@ -4.4.0-beta2 +4.4.0-beta3-SNAPSHOT