arpitjain099 commented on PR #407:
URL: https://github.com/apache/commons-net/pull/407#issuecomment-5286503028
Here it is, as a third case for `TFTPRequestPacketOptionBoundsTest`. It
fails on the PR branch and passes with the change I describe below.
```java
/** An option must be a NUL terminated name followed by a NUL terminated
value (RFC 2347). */
@Test
void testOptionNameWithoutValueIsRejected() throws Exception {
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);
// the option name is terminated, but no value field follows
final byte[] data = out.toByteArray();
final DatagramPacket packet = new DatagramPacket(data, data.length,
InetAddress.getLoopbackAddress(), 69);
assertThrows(TFTPPacketException.class, () ->
TFTPPacket.newTFTPPacket(packet));
}
```
On the branch this returns `{blksize=}` instead of throwing. After the name
loop stops on the NUL, `index` is incremented to `dataLen`, so the value loop's
`index < dataLen` guard is false immediately and the value reads as the empty
string.
Master does not get this right either, it throws
`ArrayIndexOutOfBoundsException` or `TFTPPacketException` depending on what is
in the buffer past the packet, so this is a question of what correct should be
rather than a regression. If you would rather accept a missing value as empty,
that is a defensible choice and I will drop it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
