This is an automated email from the ASF dual-hosted git repository.
garydgregory pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/commons-net.git
The following commit(s) were added to refs/heads/master by this push:
new ec2d274e TFTPRequestPacket now throws TFTPPacketException instead of
ArrayIndexOutOfBoundsException (#407)
ec2d274e is described below
commit ec2d274e310a5a4c934fc60669a0f06d64330efc
Author: Gary Gregory <[email protected]>
AuthorDate: Fri Aug 14 07:07:46 2026 -0400
TFTPRequestPacket now throws TFTPPacketException instead of
ArrayIndexOutOfBoundsException (#407)
* TFTPRequestPacket now throws TFTPPacketException instead of
ArrayIndexOutOfBoundsException.
* Refactor packet type checking
* Inline local variable declaration and initialization.
* Internal refactoring.
* Add org.apache.commons.net.tftp.TFTPPacketTest
---
.../org/apache/commons/net/tftp/TFTPAckPacket.java | 4 +-
.../apache/commons/net/tftp/TFTPDataPacket.java | 4 +-
.../apache/commons/net/tftp/TFTPErrorPacket.java | 4 +-
.../org/apache/commons/net/tftp/TFTPPacket.java | 8 ++-
.../apache/commons/net/tftp/TFTPRequestPacket.java | 73 ++++++++-------------
.../apache/commons/net/tftp/TFTPAckPacketTest.java | 9 ++-
.../commons/net/tftp/TFTPDataPacketTest.java | 9 ++-
.../commons/net/tftp/TFTPErrorPacketTest.java | 10 ++-
...{TFTPAckPacketTest.java => TFTPPacketTest.java} | 27 ++++----
.../net/tftp/TFTPReadRequestPacketTest.java | 9 ++-
.../tftp/TFTPRequestPacketOptionBoundsTest.java | 75 ++++++++++++++++++++++
.../net/tftp/TFTPWriteRequestPacketTest.java | 14 ++--
12 files changed, 169 insertions(+), 77 deletions(-)
diff --git a/src/main/java/org/apache/commons/net/tftp/TFTPAckPacket.java
b/src/main/java/org/apache/commons/net/tftp/TFTPAckPacket.java
index 2e1ca54c..6596f2e1 100644
--- a/src/main/java/org/apache/commons/net/tftp/TFTPAckPacket.java
+++ b/src/main/java/org/apache/commons/net/tftp/TFTPAckPacket.java
@@ -52,9 +52,7 @@ public final class TFTPAckPacket extends TFTPPacket {
data = datagram.getData();
- if (getType() != data[1]) {
- throw new TFTPPacketException("TFTP operator code does not match
type.");
- }
+ checkType(data);
this.blockNumber = (data[2] & 0xff) << 8 | data[3] & 0xff;
}
diff --git a/src/main/java/org/apache/commons/net/tftp/TFTPDataPacket.java
b/src/main/java/org/apache/commons/net/tftp/TFTPDataPacket.java
index 8f97dcb9..07acf827 100644
--- a/src/main/java/org/apache/commons/net/tftp/TFTPDataPacket.java
+++ b/src/main/java/org/apache/commons/net/tftp/TFTPDataPacket.java
@@ -66,9 +66,7 @@ public final class TFTPDataPacket extends TFTPPacket {
this.data = datagram.getData();
this.offset = 4;
- if (getType() != this.data[1]) {
- throw new TFTPPacketException("TFTP operator code does not match
type.");
- }
+ checkType(data);
this.blockNumber = (this.data[2] & 0xff) << 8 | this.data[3] & 0xff;
diff --git a/src/main/java/org/apache/commons/net/tftp/TFTPErrorPacket.java
b/src/main/java/org/apache/commons/net/tftp/TFTPErrorPacket.java
index c2586627..3b568c1c 100644
--- a/src/main/java/org/apache/commons/net/tftp/TFTPErrorPacket.java
+++ b/src/main/java/org/apache/commons/net/tftp/TFTPErrorPacket.java
@@ -92,9 +92,7 @@ public final class TFTPErrorPacket extends TFTPPacket {
data = datagram.getData();
length = datagram.getLength();
- if (getType() != data[1]) {
- throw new TFTPPacketException("TFTP operator code does not match
type.");
- }
+ checkType(data);
error = (data[2] & 0xff) << 8 | data[3] & 0xff;
diff --git a/src/main/java/org/apache/commons/net/tftp/TFTPPacket.java
b/src/main/java/org/apache/commons/net/tftp/TFTPPacket.java
index c930f427..2ad76be0 100644
--- a/src/main/java/org/apache/commons/net/tftp/TFTPPacket.java
+++ b/src/main/java/org/apache/commons/net/tftp/TFTPPacket.java
@@ -111,7 +111,7 @@ public abstract class TFTPPacket {
packet = new TFTPErrorPacket(datagram);
break;
default:
- throw new TFTPPacketException("Bad packet. Invalid TFTP operator
code.");
+ throw new TFTPPacketException("Bad packet. Invalid TFTP operator
code.");
}
return packet;
}
@@ -138,6 +138,12 @@ public abstract class TFTPPacket {
this.port = port;
}
+ void checkType(final byte[] data) throws TFTPPacketException {
+ if (getType() != data[1]) {
+ throw new TFTPPacketException("TFTP operator code does not match
type.");
+ }
+ }
+
/**
* Gets the address of the host where the packet is going to be sent or
where it came from.
*
diff --git a/src/main/java/org/apache/commons/net/tftp/TFTPRequestPacket.java
b/src/main/java/org/apache/commons/net/tftp/TFTPRequestPacket.java
index 9a7383b4..f3dec654 100644
--- a/src/main/java/org/apache/commons/net/tftp/TFTPRequestPacket.java
+++ b/src/main/java/org/apache/commons/net/tftp/TFTPRequestPacket.java
@@ -90,70 +90,57 @@ public abstract class TFTPRequestPacket extends TFTPPacket {
*/
TFTPRequestPacket(final int type, final DatagramPacket datagram) throws
TFTPPacketException {
super(type, datagram.getAddress(), datagram.getPort());
-
final byte[] data = datagram.getData();
-
- if (getType() != data[1]) {
- throw new TFTPPacketException("TFTP operator code does not match
type.");
- }
-
+ final int dataLen = datagram.getLength();
+ checkType(data);
final StringBuilder buffer = new StringBuilder();
-
int index = 2;
- final int length = datagram.getLength();
-
- while (index < length && data[index] != 0) {
+ while (isChar(data, index)) {
buffer.append((char) data[index]);
++index;
}
-
this.fileName = buffer.toString();
-
- if (index >= length) {
+ if (index >= dataLen) {
throw new TFTPPacketException("Bad file name and mode format.");
}
-
buffer.setLength(0);
++index; // need to advance beyond the end of string marker
- while (index < length && data[index] != 0) {
+ while (isChar(data, index)) {
buffer.append((char) data[index]);
++index;
}
-
final String modeString =
buffer.toString().toLowerCase(Locale.ENGLISH);
- final int modeStringsLength = modeStrings.length;
-
+ final int modeStringsLen = modeStrings.length;
int mode = 0;
int modeIndex;
- for (modeIndex = 0; modeIndex < modeStringsLength; modeIndex++) {
+ for (modeIndex = 0; modeIndex < modeStringsLen; modeIndex++) {
if (modeString.equals(modeStrings[modeIndex])) {
mode = modeIndex;
break;
}
}
-
this.mode = mode;
-
- if (modeIndex >= modeStringsLength) {
+ if (modeIndex >= modeStringsLen) {
throw new TFTPPacketException("Unrecognized TFTP transfer mode: "
+ modeString);
// May just want to default to binary mode instead of throwing
// exception.
- // _mode = TFTP.OCTET_MODE;
+ // mode = TFTP.OCTET_MODE;
}
-
++index;
- while (index < length) {
+ while (index < dataLen) {
int start = index;
- for (; data[index] != 0; ++index) {
- if (index >= length) {
+ while (isChar(data, index)) {
+ index++;
+ if (index >= dataLen) {
throw new TFTPPacketException("Invalid option format");
}
}
final String option = new String(data, start, index - start,
StandardCharsets.US_ASCII);
++index;
start = index;
- for (; data[index] != 0; ++index) {
- if (index >= length) {
+ while (isChar(data, index)) {
+ index++;
+ if (index >= dataLen) {
throw new TFTPPacketException("Invalid option format");
}
}
@@ -208,6 +195,10 @@ public abstract class TFTPRequestPacket extends TFTPPacket
{
}
}
+ private boolean isChar(final byte[] data, int index) {
+ return index < data.length && data[index] != 0;
+ }
+
/**
* Creates a UDP datagram containing all the TFTP request packet data in
the proper format. This is a method exposed to the programmer in case he wants
to
* implement his own TFTP client instead of using the {@link
org.apache.commons.net.tftp.TFTPClient} class. Under normal circumstances, you
should not have
@@ -217,28 +208,21 @@ public abstract class TFTPRequestPacket extends
TFTPPacket {
*/
@Override
public final DatagramPacket newDatagram() {
- final int fileLength;
- final int modeLength;
- final byte[] data;
-
- fileLength = fileName.length();
- modeLength = modeBytes[mode].length;
-
+ final int fileLength = fileName.length();
+ final int modeLength = modeBytes[mode].length;
int optionsLength = 0;
for (final Map.Entry<String, String> entry : options.entrySet()) {
optionsLength += entry.getKey().length() + 1 +
entry.getValue().length() + 1;
}
- data = new byte[fileLength + modeLength + 3 + optionsLength];
+ final byte[] data = new byte[fileLength + modeLength + 3 +
optionsLength];
data[0] = 0;
data[1] = (byte) type;
System.arraycopy(fileName.getBytes(Charset.defaultCharset()), 0, data,
2, fileLength);
data[fileLength + 2] = 0;
System.arraycopy(modeBytes[mode], 0, data, fileLength + 3, modeLength);
-
if (optionsLength > 0) {
handleOptions(data, fileLength, modeLength);
}
-
return new DatagramPacket(data, data.length, address, port);
}
@@ -252,25 +236,18 @@ public abstract class TFTPRequestPacket extends
TFTPPacket {
*/
@Override
final DatagramPacket newDatagram(final DatagramPacket datagram, final
byte[] data) {
- final int fileLength;
- final int modeLength;
-
- fileLength = fileName.length();
- modeLength = modeBytes[mode].length;
-
+ final int fileLength = fileName.length();
+ final int modeLength = modeBytes[mode].length;
data[0] = 0;
data[1] = (byte) type;
System.arraycopy(fileName.getBytes(Charset.defaultCharset()), 0, data,
2, fileLength);
data[fileLength + 2] = 0;
System.arraycopy(modeBytes[mode], 0, data, fileLength + 3, modeLength);
-
handleOptions(data, fileLength, modeLength);
-
datagram.setAddress(address);
datagram.setPort(port);
datagram.setData(data);
datagram.setLength(fileLength + modeLength + 3);
-
return datagram;
}
}
diff --git a/src/test/java/org/apache/commons/net/tftp/TFTPAckPacketTest.java
b/src/test/java/org/apache/commons/net/tftp/TFTPAckPacketTest.java
index b4eaedd3..f131d070 100644
--- a/src/test/java/org/apache/commons/net/tftp/TFTPAckPacketTest.java
+++ b/src/test/java/org/apache/commons/net/tftp/TFTPAckPacketTest.java
@@ -19,15 +19,22 @@ package org.apache.commons.net.tftp;
import static org.junit.jupiter.api.Assertions.assertNotNull;
+import java.net.DatagramPacket;
import java.net.InetAddress;
import java.net.UnknownHostException;
import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.function.Executable;
/**
* Tests {@link TFTPAckPacket}.
*/
-class TFTPAckPacketTest {
+class TFTPAckPacketTest extends TFTPPacketTest {
+
+ @Override
+ protected Executable getDatagramPacketCtor(final DatagramPacket packet) {
+ return () -> new TFTPAckPacket(packet);
+ }
@Test
void testNewDatagram() throws UnknownHostException {
diff --git a/src/test/java/org/apache/commons/net/tftp/TFTPDataPacketTest.java
b/src/test/java/org/apache/commons/net/tftp/TFTPDataPacketTest.java
index 76c758a3..12e777a0 100644
--- a/src/test/java/org/apache/commons/net/tftp/TFTPDataPacketTest.java
+++ b/src/test/java/org/apache/commons/net/tftp/TFTPDataPacketTest.java
@@ -19,15 +19,22 @@ package org.apache.commons.net.tftp;
import static org.junit.jupiter.api.Assertions.assertNotNull;
+import java.net.DatagramPacket;
import java.net.InetAddress;
import java.net.UnknownHostException;
import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.function.Executable;
/**
* Tests {@link TFTPDataPacket}.
*/
-class TFTPDataPacketTest {
+class TFTPDataPacketTest extends TFTPPacketTest {
+
+ @Override
+ protected Executable getDatagramPacketCtor(final DatagramPacket packet) {
+ return () -> new TFTPDataPacket(packet);
+ }
@Test
void testNewDatagram() throws UnknownHostException {
diff --git a/src/test/java/org/apache/commons/net/tftp/TFTPErrorPacketTest.java
b/src/test/java/org/apache/commons/net/tftp/TFTPErrorPacketTest.java
index 6183cb7d..6b339fc0 100644
--- a/src/test/java/org/apache/commons/net/tftp/TFTPErrorPacketTest.java
+++ b/src/test/java/org/apache/commons/net/tftp/TFTPErrorPacketTest.java
@@ -19,15 +19,22 @@ package org.apache.commons.net.tftp;
import static org.junit.jupiter.api.Assertions.assertNotNull;
+import java.net.DatagramPacket;
import java.net.InetAddress;
import java.net.UnknownHostException;
import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.function.Executable;
/**
* Tests {@link TFTPErrorPacket}.
*/
-class TFTPErrorPacketTest {
+class TFTPErrorPacketTest extends TFTPPacketTest {
+
+ @Override
+ protected Executable getDatagramPacketCtor(final DatagramPacket packet) {
+ return () -> new TFTPErrorPacket(packet);
+ }
@Test
void testNewDatagram() throws UnknownHostException {
@@ -38,4 +45,5 @@ class TFTPErrorPacketTest {
void testToString() throws UnknownHostException {
assertNotNull(new TFTPErrorPacket(InetAddress.getLocalHost(), 0, 0,
"").toString());
}
+
}
diff --git a/src/test/java/org/apache/commons/net/tftp/TFTPAckPacketTest.java
b/src/test/java/org/apache/commons/net/tftp/TFTPPacketTest.java
similarity index 50%
copy from src/test/java/org/apache/commons/net/tftp/TFTPAckPacketTest.java
copy to src/test/java/org/apache/commons/net/tftp/TFTPPacketTest.java
index b4eaedd3..4f52a5fb 100644
--- a/src/test/java/org/apache/commons/net/tftp/TFTPAckPacketTest.java
+++ b/src/test/java/org/apache/commons/net/tftp/TFTPPacketTest.java
@@ -17,25 +17,30 @@
package org.apache.commons.net.tftp;
-import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import java.net.DatagramPacket;
import java.net.InetAddress;
import java.net.UnknownHostException;
import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.function.Executable;
-/**
- * Tests {@link TFTPAckPacket}.
- */
-class TFTPAckPacketTest {
+abstract class TFTPPacketTest {
- @Test
- void testNewDatagram() throws UnknownHostException {
- assertNotNull(new TFTPAckPacket(InetAddress.getLocalHost(), 0,
0).newDatagram());
- }
+ protected abstract Executable getDatagramPacketCtor(DatagramPacket packet);
@Test
- void testToString() throws UnknownHostException {
- assertNotNull(new TFTPAckPacket(InetAddress.getLocalHost(), 0,
0).toString());
+ public void testConstructorBadType() throws UnknownHostException {
+ // Create a DatagramPacket with invalid TFTP packet type (not ACK)
+ final InetAddress address = InetAddress.getLocalHost();
+ final byte[] data = new byte[4];
+ data[0] = 0; // TFTP opcode 0 (invalid)
+ data[1] = 0; // TFTP opcode 0 (invalid)
+ data[2] = 0; // Block number high byte
+ data[3] = 1; // Block number low byte
+ final DatagramPacket packet = new DatagramPacket(data, data.length,
address, 69);
+ assertThrows(TFTPPacketException.class, () ->
TFTPPacket.newTFTPPacket(packet));
+ assertThrows(TFTPPacketException.class, getDatagramPacketCtor(packet));
}
}
diff --git
a/src/test/java/org/apache/commons/net/tftp/TFTPReadRequestPacketTest.java
b/src/test/java/org/apache/commons/net/tftp/TFTPReadRequestPacketTest.java
index c41e7264..93094574 100644
--- a/src/test/java/org/apache/commons/net/tftp/TFTPReadRequestPacketTest.java
+++ b/src/test/java/org/apache/commons/net/tftp/TFTPReadRequestPacketTest.java
@@ -19,15 +19,22 @@ package org.apache.commons.net.tftp;
import static org.junit.jupiter.api.Assertions.assertNotNull;
+import java.net.DatagramPacket;
import java.net.InetAddress;
import java.net.UnknownHostException;
import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.function.Executable;
/**
* Tests {@link TFTPReadRequestPacket}.
*/
-class TFTPReadRequestPacketTest {
+class TFTPReadRequestPacketTest extends TFTPPacketTest {
+
+ @Override
+ protected Executable getDatagramPacketCtor(final DatagramPacket packet) {
+ return () -> new TFTPReadRequestPacket(packet);
+ }
@Test
void testToString() throws UnknownHostException {
diff --git
a/src/test/java/org/apache/commons/net/tftp/TFTPRequestPacketOptionBoundsTest.java
b/src/test/java/org/apache/commons/net/tftp/TFTPRequestPacketOptionBoundsTest.java
new file mode 100644
index 00000000..8a57f089
--- /dev/null
+++
b/src/test/java/org/apache/commons/net/tftp/TFTPRequestPacketOptionBoundsTest.java
@@ -0,0 +1,75 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You 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
+ *
+ * https://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.apache.commons.net.tftp;
+
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertThrowsExactly;
+
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
+import java.net.DatagramPacket;
+import java.net.InetAddress;
+import java.nio.charset.StandardCharsets;
+import java.util.Arrays;
+
+import org.junit.jupiter.api.Test;
+
+/**
+ * Tests {@link TFTPRequestPacket}.
+ */
+class TFTPRequestPacketOptionBoundsTest {
+
+ /**
+ * RRQ for "f" in octet mode, one option whose value has no terminating
NUL.
+ */
+ private static byte[] newReadRequest() throws IOException {
+ final ByteArrayOutputStream out = new ByteArrayOutputStream();
+ out.write(0);
+ out.write(TFTPPacket.READ_REQUEST);
+ out.write("f".getBytes(StandardCharsets.US_ASCII));
+ out.write(0);
+ out.write("octet".getBytes(StandardCharsets.US_ASCII));
+ out.write(0);
+ out.write("blksize".getBytes(StandardCharsets.US_ASCII));
+ out.write(0);
+ out.write("1024".getBytes(StandardCharsets.US_ASCII));
+ return out.toByteArray();
+ }
+
+ private static String parse(final byte[] buf, final int len) throws
TFTPPacketException {
+ final DatagramPacket packet = new DatagramPacket(buf, len,
InetAddress.getLoopbackAddress(), 69);
+ return "OK " + ((TFTPRequestPacket)
TFTPPacket.newTFTPPacket(packet)).getOptions();
+ }
+
+ @Test
+ void testParseIsIndependentOfBytesBeyondGetLength() throws Exception {
+ final byte[] request = newReadRequest();
+ final byte[] zeroed = Arrays.copyOf(request, request.length + 16);
+ final byte[] stale = Arrays.copyOf(request, request.length + 16);
+ Arrays.fill(stale, request.length, stale.length, (byte) 'S');
+ assertThrowsExactly(TFTPPacketException.class, () -> parse(zeroed,
request.length));
+ assertThrowsExactly(TFTPPacketException.class, () -> parse(stale,
request.length));
+ }
+
+ @Test
+ void testUnterminatedOptionAtEndOfBufferThrowsDeclaredException() throws
Exception {
+ final byte[] request = newReadRequest();
+ final DatagramPacket packet = new DatagramPacket(request,
request.length, InetAddress.getLoopbackAddress(), 69);
+ assertThrows(TFTPPacketException.class, () ->
TFTPPacket.newTFTPPacket(packet));
+ }
+}
diff --git
a/src/test/java/org/apache/commons/net/tftp/TFTPWriteRequestPacketTest.java
b/src/test/java/org/apache/commons/net/tftp/TFTPWriteRequestPacketTest.java
index 50796719..014cb386 100644
--- a/src/test/java/org/apache/commons/net/tftp/TFTPWriteRequestPacketTest.java
+++ b/src/test/java/org/apache/commons/net/tftp/TFTPWriteRequestPacketTest.java
@@ -19,19 +19,25 @@ package org.apache.commons.net.tftp;
import static org.junit.jupiter.api.Assertions.assertNotNull;
+import java.net.DatagramPacket;
import java.net.InetAddress;
import java.net.UnknownHostException;
import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.function.Executable;
/**
- * Tests {@link TFTPReadRequestPacket}.
+ * Tests {@link TFTPWriteRequestPacket}.
*/
-class TFTPWriteRequestPacketTest {
+class TFTPWriteRequestPacketTest extends TFTPPacketTest {
+
+ @Override
+ protected Executable getDatagramPacketCtor(final DatagramPacket packet) {
+ return () -> new TFTPWriteRequestPacket(packet);
+ }
@Test
void testToString() throws UnknownHostException {
- assertNotNull(new TFTPReadRequestPacket(InetAddress.getLocalHost(), 0,
"", 0).toString());
+ assertNotNull(new TFTPWriteRequestPacket(InetAddress.getLocalHost(),
0, "", 0).toString());
}
-
}
