TFTPRequestPacket now throws TFTPPacketException instead of ArrayIndexOutOfBoundsException

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;
     }

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;
 

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;
 

src/main/java/org/apache/commons/net/tftp/TFTPPacket.java

@@ -111,7 +111,7 @@ public static final TFTPPacket newTFTPPacket(final DatagramPacket datagram) thro
             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 static final TFTPPacket newTFTPPacket(final DatagramPacket datagram) thro
         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.
      *

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 @@ private void handleOptions(final byte[] data, final int fileLength, final int mo
         }
     }
 
+    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 @@ private void handleOptions(final byte[] data, final int fileLength, final int mo
      */
     @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 final DatagramPacket newDatagram() {
      */
     @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;
     }
 }

src/test/java/org/apache/commons/net/tftp/TFTPAckPacketTest.java

@@ -19,15 +19,22 @@
 
 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 {

src/test/java/org/apache/commons/net/tftp/TFTPDataPacketTest.java

@@ -19,15 +19,22 @@
 
 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 {

src/test/java/org/apache/commons/net/tftp/TFTPErrorPacketTest.java

@@ -19,15 +19,22 @@
 
 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 @@ void testNewDatagram() throws UnknownHostException {
     void testToString() throws UnknownHostException {
         assertNotNull(new TFTPErrorPacket(InetAddress.getLocalHost(), 0, 0, "").toString());
     }
+
 }

src/test/java/org/apache/commons/net/tftp/TFTPPacketTest.java

@@ -0,0 +1,46 @@
+/*
+ * 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 java.net.DatagramPacket;
+import java.net.InetAddress;
+import java.net.UnknownHostException;
+
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.function.Executable;
+
+abstract class TFTPPacketTest {
+
+    protected abstract Executable getDatagramPacketCtor(DatagramPacket packet);
+
+    @Test
+    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));
+    }
+}

src/test/java/org/apache/commons/net/tftp/TFTPReadRequestPacketTest.java

@@ -19,15 +19,22 @@
 
 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 {