[ibb] Use UInt16 for 'seq' and fix its handling

Fixes a off-by-one error when incrementing 'seq'. Thanks to Kim
Alvefur <zash@zash.se> for spotting this.
This commit is contained in:
Florian Schmaus 2020-12-03 08:53:19 +01:00
parent 02c9058c3d
commit a4bb5bfda8
17 changed files with 297 additions and 81 deletions

View File

@ -1,6 +1,6 @@
/**
*
* Copyright 2019 Florian Schmaus
* Copyright 2019-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.
@ -16,7 +16,9 @@
*/
package org.jivesoftware.smack.datatypes;
public abstract class Scalar extends java.lang.Number {
import org.jivesoftware.smack.util.DefaultCharSequence;
public abstract class Scalar extends java.lang.Number implements DefaultCharSequence {
/**
*
@ -83,4 +85,11 @@ public abstract class Scalar extends java.lang.Number {
public final String toString() {
return number.toString();
}
public abstract Scalar getMinValue();
public abstract Scalar getMaxValue();
public abstract Scalar incrementedByOne();
}

View File

@ -1,6 +1,6 @@
/**
*
* Copyright 2019 Florian Schmaus
* Copyright 2019-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.
@ -21,12 +21,18 @@ import org.jivesoftware.smack.util.NumberUtil;
/**
* A number representing an unsigned 16-bit integer. Can be used for values with the XML schema type "xs:unsingedShort".
*/
public final class UInt16 extends Scalar {
public final class UInt16 extends Scalar implements Comparable<UInt16> {
private static final long serialVersionUID = 1L;
private final int number;
public static final int MIN_VALUE_INT = 0;
public static final int MAX_VALUE_INT = (1 << 16) - 1;
public static final UInt16 MIN_VALUE = UInt16.from(MIN_VALUE_INT);
public static final UInt16 MAX_VALUE = UInt16.from(MAX_VALUE_INT);
private UInt16(int number) {
super(NumberUtil.requireUShort16(number));
this.number = number;
@ -54,4 +60,25 @@ public final class UInt16 extends Scalar {
return super.equals(other);
}
@Override
public int compareTo(UInt16 o) {
return Integer.compare(number, o.number);
}
@Override
public UInt16 getMinValue() {
return MIN_VALUE;
}
@Override
public UInt16 getMaxValue() {
return MAX_VALUE;
}
@Override
public UInt16 incrementedByOne() {
int incrementedValue = number < MAX_VALUE_INT ? number + 1 : 0;
return UInt16.from(incrementedValue);
}
}

View File

@ -1,6 +1,6 @@
/**
*
* Copyright 2019 Florian Schmaus
* Copyright 2019-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.
@ -27,6 +27,12 @@ public final class UInt32 extends Scalar {
private final long number;
public static final long MIN_VALUE_LONG = 0;
public static final long MAX_VALUE_LONG = (1L << 32) - 1;
public static final UInt32 MIN_VALUE = UInt32.from(MAX_VALUE_LONG);
public static final UInt32 MAX_VALUE = UInt32.from(MAX_VALUE_LONG);
private UInt32(long number) {
super(NumberUtil.requireUInt32(number));
this.number = number;
@ -55,4 +61,20 @@ public final class UInt32 extends Scalar {
return super.equals(other);
}
@Override
public UInt32 getMinValue() {
return MIN_VALUE;
}
@Override
public UInt32 getMaxValue() {
return MAX_VALUE;
}
@Override
public UInt32 incrementedByOne() {
long incrementedValue = number < MAX_VALUE_LONG ? number + 1 : 0;
return UInt32.from(incrementedValue);
}
}

View File

@ -55,4 +55,28 @@ public class SmackParsingException extends Exception {
super(uriSyntaxException);
}
}
public static class RequiredValueMissingException extends SmackParsingException {
/**
*
*/
private static final long serialVersionUID = 1L;
public RequiredValueMissingException(String message) {
super(message);
}
}
public static class RequiredAttributeMissingException extends RequiredValueMissingException {
/**
*
*/
private static final long serialVersionUID = 1L;
public RequiredAttributeMissingException(String attributeName) {
super("The required attribute '" + attributeName + "' is missing (or has the empty String as value)");
}
}
}

View File

@ -0,0 +1,36 @@
/**
*
* Copyright 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.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package org.jivesoftware.smack.util;
public interface DefaultCharSequence extends CharSequence {
@Override
default int length() {
return toString().length();
}
@Override
default char charAt(int index) {
return toString().charAt(index);
}
@Override
default CharSequence subSequence(int start, int end) {
return toString().subSequence(start, end);
}
}

View File

@ -29,6 +29,7 @@ import org.jivesoftware.smack.datatypes.UInt16;
import org.jivesoftware.smack.datatypes.UInt32;
import org.jivesoftware.smack.packet.XmlEnvironment;
import org.jivesoftware.smack.parsing.SmackParsingException;
import org.jivesoftware.smack.parsing.SmackParsingException.RequiredAttributeMissingException;
import org.jivesoftware.smack.parsing.SmackParsingException.SmackTextParseException;
import org.jivesoftware.smack.parsing.SmackParsingException.SmackUriSyntaxParsingException;
import org.jivesoftware.smack.xml.XmlPullParser;
@ -231,6 +232,14 @@ public class ParserUtils {
return UInt16.from(integer);
}
public static UInt16 getRequiredUInt16Attribute(XmlPullParser parser, String name) throws RequiredAttributeMissingException {
UInt16 uint16 = getUInt16Attribute(parser, name);
if (uint16 == null) {
throw new SmackParsingException.RequiredAttributeMissingException(name);
}
return uint16;
}
public static int getIntegerFromNextText(XmlPullParser parser) throws XmlPullParserException, IOException {
String intString = parser.nextText();
return Integer.valueOf(intString);

View File

@ -362,6 +362,20 @@ public class XmlStringBuilder implements Appendable, CharSequence, Element {
return this;
}
/**
* Same as {@link #optAttribute(String, CharSequence)}, but with a different method name. This method can be used if
* the provided attribute value argument type causes ambiguity in method overloading. For example if the type is a
* subclass of Number and CharSequence.
*
* @param name the name of the attribute.
* @param value the value of the attribute.
* @return a reference to this object.
* @since 4.5
*/
public XmlStringBuilder optAttributeCs(String name, CharSequence value) {
return optAttribute(name, value);
}
/**
* Add the given attribute if {@code value => 0}.
*

View File

@ -0,0 +1,30 @@
/**
*
* Copyright 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.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package org.jivesoftware.smack.datatypes;
import static org.junit.Assert.assertEquals;
import org.junit.jupiter.api.Test;
public class UInt16Test {
@Test
public void testMaxValue() {
assertEquals(65535, UInt16.MAX_VALUE_INT);
}
}

View File

@ -0,0 +1,30 @@
/**
*
* Copyright 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.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package org.jivesoftware.smack.datatypes;
import static org.junit.Assert.assertEquals;
import org.junit.jupiter.api.Test;
public class UInt32Test {
@Test
public void testMaxValue() {
assertEquals(4294967295L, UInt32.MAX_VALUE_LONG);
}
}

View File

@ -23,11 +23,14 @@ import java.net.SocketTimeoutException;
import java.util.concurrent.BlockingQueue;
import java.util.concurrent.LinkedBlockingQueue;
import java.util.concurrent.TimeUnit;
import java.util.logging.Level;
import java.util.logging.Logger;
import org.jivesoftware.smack.SmackException.NotConnectedException;
import org.jivesoftware.smack.SmackException.NotLoggedInException;
import org.jivesoftware.smack.StanzaListener;
import org.jivesoftware.smack.XMPPConnection;
import org.jivesoftware.smack.datatypes.UInt16;
import org.jivesoftware.smack.filter.AndFilter;
import org.jivesoftware.smack.filter.StanzaFilter;
import org.jivesoftware.smack.filter.StanzaTypeFilter;
@ -61,6 +64,10 @@ import org.jxmpp.jid.Jid;
*/
public class InBandBytestreamSession implements BytestreamSession {
private static final Logger LOGGER = Logger.getLogger(InBandBytestreamSession.class.getName());
static final String UNEXPECTED_IBB_SEQUENCE = "Unexpected IBB sequence";
/* XMPP connection */
private final XMPPConnection connection;
@ -261,7 +268,7 @@ public class InBandBytestreamSession implements BytestreamSession {
private int bufferPointer = -1;
/* data packet sequence (range from 0 to 65535) */
private long seq = -1;
private UInt16 expectedSeq = UInt16.MIN_VALUE;
/* flag to indicate if input stream is closed */
private boolean isClosed = false;
@ -383,21 +390,16 @@ public class InBandBytestreamSession implements BytestreamSession {
return false;
}
// handle sequence overflow
if (this.seq == 65535) {
this.seq = -1;
}
final UInt16 dataSeq = data.getSeq();
// check if data packets sequence is successor of last seen sequence
long seq = data.getSeq();
if (seq - 1 != this.seq) {
if (!expectedSeq.equals(dataSeq)) {
// packets out of order; close stream/session
InBandBytestreamSession.this.close();
throw new IOException("Packets out of sequence");
}
else {
this.seq = seq;
String message = UNEXPECTED_IBB_SEQUENCE + " " + dataSeq + " received, expected "
+ expectedSeq;
throw new IOException(message);
}
expectedSeq = dataSeq.incrementedByOne();
// set buffer to decoded data
buffer = data.getDecodedData();
@ -465,22 +467,41 @@ public class InBandBytestreamSession implements BytestreamSession {
protected StanzaListener getDataPacketListener() {
return new StanzaListener() {
private long lastSequence = -1;
private UInt16 expectedSequence = UInt16.MIN_VALUE;;
@Override
public void processStanza(Stanza packet) throws NotConnectedException, InterruptedException {
final Data dataIq = (Data) packet;
// get data packet extension
DataPacketExtension data = ((Data) packet).getDataPacketExtension();
DataPacketExtension data = dataIq.getDataPacketExtension();
final UInt16 seq = data.getSeq();
/*
* check if sequence was not used already (see XEP-0047 Section 2.2)
*/
if (data.getSeq() <= this.lastSequence) {
IQ unexpectedRequest = IQ.createErrorResponse((IQ) packet,
StanzaError.Condition.unexpected_request);
if (!expectedSequence.equals(seq)) {
String descriptiveEnTest = UNEXPECTED_IBB_SEQUENCE + " " + seq + " received, expected "
+ expectedSequence;
StanzaError stanzaError = StanzaError.getBuilder()
.setCondition(StanzaError.Condition.unexpected_request)
.setDescriptiveEnText(descriptiveEnTest)
.build();
IQ unexpectedRequest = IQ.createErrorResponse(dataIq, stanzaError);
connection.sendStanza(unexpectedRequest);
return;
try {
// TODO: It would be great if close would take a "close error reason" argument. Also there
// is the question if this is really a reason to close the stream. We could have some more
// tolerance regarding out-of-sequence stanzas arriving: Even though XMPP has the in-order
// guarantee, I could imagine that there are cases where stanzas are, for example,
// duplicated because of stream resumption.
close();
} catch (IOException e) {
LOGGER.log(Level.FINER, "Could not close session, because of IOException. Close reason: "
+ descriptiveEnTest);
}
return;
}
// check if encoded data is valid (see XEP-0047 Section 2.2)
@ -492,19 +513,14 @@ public class InBandBytestreamSession implements BytestreamSession {
return;
}
expectedSequence = seq.incrementedByOne();
// data is valid; add to data queue
dataQueue.offer(data);
// confirm IQ
IQ confirmData = IQ.createResultIQ((IQ) packet);
connection.sendStanza(confirmData);
// set last seen sequence
this.lastSequence = data.getSeq();
if (this.lastSequence == 65535) {
this.lastSequence = -1;
}
}
};
@ -618,7 +634,7 @@ public class InBandBytestreamSession implements BytestreamSession {
protected int bufferPointer = 0;
/* data packet sequence (range from 0 to 65535) */
protected long seq = 0;
protected UInt16 seq = UInt16.from(0);
/* flag to indicate if output stream is closed */
protected boolean isClosed = false;
@ -756,7 +772,7 @@ public class InBandBytestreamSession implements BytestreamSession {
bufferPointer = 0;
// increment sequence, considering sequence overflow
this.seq = this.seq + 1 == 65535 ? 0 : this.seq + 1;
seq = seq.incrementedByOne();
}

View File

@ -18,8 +18,10 @@ package org.jivesoftware.smackx.bytestreams.ibb.packet;
import javax.xml.namespace.QName;
import org.jivesoftware.smack.datatypes.UInt16;
import org.jivesoftware.smack.packet.ExtensionElement;
import org.jivesoftware.smack.packet.IQ.IQChildElementXmlStringBuilder;
import org.jivesoftware.smack.util.Objects;
import org.jivesoftware.smack.util.XmlStringBuilder;
import org.jivesoftware.smack.util.stringencoder.Base64;
@ -47,7 +49,7 @@ public class DataPacketExtension implements ExtensionElement {
private final String sessionID;
/* sequence of this packet in regard to the other data packets */
private final long seq;
private final UInt16 seq;
/* the data contained in this packet */
private final String data;
@ -60,19 +62,28 @@ public class DataPacketExtension implements ExtensionElement {
* @param sessionID unique session ID identifying this In-Band Bytestream
* @param seq sequence of this stanza in regard to the other data packets
* @param data the base64 encoded data contained in this packet
* @throws IllegalArgumentException if seq is not within the range [0, 65535].
*/
public DataPacketExtension(String sessionID, long seq, String data) {
public DataPacketExtension(String sessionID, int seq, String data) {
this(sessionID, UInt16.from(seq), data);
}
/**
* Creates a new In-Band Bytestream data packet.
*
* @param sessionID unique session ID identifying this In-Band Bytestream
* @param seq sequence of this stanza in regard to the other data packets
* @param data the base64 encoded data contained in this packet
*/
public DataPacketExtension(String sessionID, UInt16 seq, String data) {
if (sessionID == null || "".equals(sessionID)) {
throw new IllegalArgumentException("Session ID must not be null or empty");
}
if (seq < 0 || seq > 65535) {
throw new IllegalArgumentException("Sequence must not be between 0 and 65535");
}
if (data == null) {
throw new IllegalArgumentException("Data must not be null");
}
this.sessionID = sessionID;
this.seq = seq;
this.seq = Objects.requireNonNull(seq);
this.data = data;
}
@ -90,7 +101,7 @@ public class DataPacketExtension implements ExtensionElement {
*
* @return the sequence of this stanza in regard to the other data packets.
*/
public long getSeq() {
public UInt16 getSeq() {
return seq;
}
@ -148,7 +159,7 @@ public class DataPacketExtension implements ExtensionElement {
}
protected IQChildElementXmlStringBuilder getIQChildElementBuilder(IQChildElementXmlStringBuilder xml) {
xml.attribute("seq", Long.toString(seq));
xml.attribute("seq", seq);
xml.attribute("sid", sessionID);
xml.rightAngleBracket();
xml.append(data);

View File

@ -18,8 +18,11 @@ package org.jivesoftware.smackx.bytestreams.ibb.provider;
import java.io.IOException;
import org.jivesoftware.smack.datatypes.UInt16;
import org.jivesoftware.smack.packet.XmlEnvironment;
import org.jivesoftware.smack.parsing.SmackParsingException;
import org.jivesoftware.smack.parsing.SmackParsingException.RequiredAttributeMissingException;
import org.jivesoftware.smack.util.ParserUtils;
import org.jivesoftware.smack.xml.XmlPullParser;
import org.jivesoftware.smack.xml.XmlPullParserException;
@ -51,9 +54,9 @@ public class DataPacketProvider {
@Override
public DataPacketExtension parse(XmlPullParser parser,
int initialDepth, XmlEnvironment xmlEnvironment) throws XmlPullParserException,
IOException {
IOException, RequiredAttributeMissingException {
String sessionID = parser.getAttributeValue("", "sid");
long seq = Long.parseLong(parser.getAttributeValue("", "seq"));
UInt16 seq = ParserUtils.getRequiredUInt16Attribute(parser, "seq");
String data = parser.nextText();
return new DataPacketExtension(sessionID, seq, data);
}

View File

@ -1,6 +1,6 @@
/**
*
* Copyright 2019 Florian Schmaus
* Copyright 2019-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.
@ -83,8 +83,8 @@ public class MediaElement implements FormFieldChildElement {
@Override
public XmlStringBuilder toXML(XmlEnvironment xmlEnvironment) {
XmlStringBuilder xml = new XmlStringBuilder(this, xmlEnvironment);
xml.optAttribute("height", height)
.optAttribute("width", width)
xml.optAttributeCs("height", height)
.optAttributeCs("width", width)
.rightAngleBracket();
xml.append(uris);

View File

@ -401,8 +401,8 @@ public abstract class ValidateElement implements FormFieldChildElement {
@Override
public XmlStringBuilder toXML(XmlEnvironment enclosingXmlEnvironment) {
XmlStringBuilder buf = new XmlStringBuilder(this, enclosingXmlEnvironment);
buf.optAttribute("min", getMin());
buf.optAttribute("max", getMax());
buf.optAttributeCs("min", getMin());
buf.optAttributeCs("max", getMax());
buf.closeEmptyElement();
return buf;
}

View File

@ -17,8 +17,8 @@
package org.jivesoftware.smackx.bytestreams.ibb;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.junit.jupiter.api.Assertions.fail;
import java.io.IOException;
import java.io.InputStream;
@ -102,7 +102,7 @@ public class InBandBytestreamSessionMessageTest extends SmackTestSuite {
public void verify(Message request, IQ response) {
DataPacketExtension dpe = request.getExtension(
DataPacketExtension.class);
assertEquals(lastSeq++, dpe.getSeq());
assertEquals(lastSeq++, dpe.getSeq().longValue());
}
};
@ -275,16 +275,13 @@ public class InBandBytestreamSessionMessageTest extends SmackTestSuite {
listener.processStanza(dataMessage);
// read until exception is thrown
try {
inputStream.read();
fail("exception should be thrown");
}
catch (IOException e) {
assertTrue(e.getMessage().contains("Packets out of sequence"));
}
IOException ioException = assertThrows(IOException.class, () ->
inputStream.read()
);
String ioExceptionMessage = ioException.getMessage();
assertTrue(ioExceptionMessage.startsWith(InBandBytestreamSession.UNEXPECTED_IBB_SEQUENCE));
protocol.verifyAll();
}
/**

View File

@ -17,6 +17,7 @@
package org.jivesoftware.smackx.bytestreams.ibb;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.junit.jupiter.api.Assertions.fail;
@ -29,6 +30,7 @@ import org.jivesoftware.smack.SmackException;
import org.jivesoftware.smack.StanzaListener;
import org.jivesoftware.smack.XMPPConnection;
import org.jivesoftware.smack.XMPPException;
import org.jivesoftware.smack.datatypes.UInt16;
import org.jivesoftware.smack.packet.IQ;
import org.jivesoftware.smack.packet.StanzaError;
import org.jivesoftware.smack.test.util.SmackTestSuite;
@ -96,11 +98,11 @@ public class InBandBytestreamSessionTest extends SmackTestSuite {
incrementingSequence = new Verification<Data, IQ>() {
long lastSeq = 0;
int lastSeq = 0;
@Override
public void verify(Data request, IQ response) {
assertEquals(lastSeq++, request.getDataPacketExtension().getSeq());
assertEquals(lastSeq++, request.getDataPacketExtension().getSeq().intValue());
}
};
@ -266,9 +268,9 @@ public class InBandBytestreamSessionTest extends SmackTestSuite {
@Override
public void verify(Data request, IQ response) {
byte[] decodedData = request.getDataPacketExtension().getDecodedData();
int seq = (int) request.getDataPacketExtension().getSeq();
UInt16 seq = request.getDataPacketExtension().getSeq();
for (int i = 0; i < decodedData.length; i++) {
assertEquals(controlData[(seq * blockSize) + i], decodedData[i]);
assertEquals(controlData[(seq.intValue() * blockSize) + i], decodedData[i]);
}
}
@ -441,15 +443,6 @@ public class InBandBytestreamSessionTest extends SmackTestSuite {
*/
@Test
public void shouldSendCloseRequestIfInvalidSequenceReceived() throws Exception {
IQ resultIQ = IBBPacketUtils.createResultIQ(initiatorJID, targetJID);
// confirm data packet with invalid sequence
protocol.addResponse(resultIQ);
// confirm close request
protocol.addResponse(resultIQ, Verification.requestTypeSET,
Verification.correspondingSenderReceiver);
// get IBB sessions data packet listener
InBandBytestreamSession session = new InBandBytestreamSession(connection, initBytestream,
initiatorJID);
@ -465,16 +458,11 @@ public class InBandBytestreamSessionTest extends SmackTestSuite {
listener.processStanza(data);
// read until exception is thrown
try {
inputStream.read();
fail("exception should be thrown");
}
catch (IOException e) {
assertTrue(e.getMessage().contains("Packets out of sequence"));
}
protocol.verifyAll();
IOException ioException = assertThrows(IOException.class, () ->
inputStream.read()
);
String ioExceptionMessage = ioException.getMessage();
assertTrue(ioExceptionMessage.equals("Stream is closed"));
}
/**

View File

@ -75,7 +75,7 @@ public class DataPacketExtensionTest extends SmackTestSuite {
public void shouldSetAllFieldsCorrectly() {
DataPacketExtension data = new DataPacketExtension("sessionID", 0, "data");
assertEquals("sessionID", data.getSessionID());
assertEquals(0, data.getSeq());
assertEquals(0, data.getSeq().intValue());
assertEquals("data", data.getData());
}