TFTPRequestPacket now throws TFTPPacketException instead of ArrayIndexOutOfBoundsException

[garydgregory]

TFTPRequestPacket now throws TFTPPacketException instead of ArrayIndexOutOfBoundsException

Before you push a pull request, review this list:

• Read the contribution guidelines for this project.
• Read the ASF Generative Tooling Guidance if you use Artificial Intelligence (AI).
• [ ] I used AI to create any part of, or all of, this pull request. Which AI tool was used to create this pull request, and to what extent did it contribute?
• Run a successful build using the default Maven goal with mvn; that's mvn on the command line by itself.
• Write unit tests that match behavioral changes, where the tests fail if the changes to the runtime are not applied. This may not always be possible, but it is a best practice.
• Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
• Each commit in the pull request should have a meaningful subject line and body. Note that a maintainer may squash commits during the merge process.

[arpitjain099]

Tested. Built master and the PR branch side by side and ran the same inputs through each.

The fix is correct. On the PR branch all three variants of the unterminated option value return TFTPPacketException, where master gives ArrayIndexOutOfBoundsException, "OK {blksize=1024}" and TFTPPacketException depending on what sits past getLength(). Well formed packets are unaffected: a terminated option still parses to {blksize=1024} with or without spare capacity, and a request with no options still parses to {}.

One behaviour change worth deciding before merge. A packet whose last option has a key and its NUL but no value field is now accepted with an empty value:

key + NUL, then nothing master: ArrayIndexOutOfBoundsException PR: OK {blksize=}
key + NUL, spare stale master: TFTPPacketException PR: OK {blksize=}

After the key loop stops on the NUL, index is incremented to dataLen, so the value loop's guard is false immediately and the value reads as empty. RFC 2347 terminates both name and value, so I would expect "Invalid option format" here. Master was not right either, so this is a choice about what correct looks like rather than a regression. The two tests on the branch do not cover it.

Minor: the new dataLen < 1 guard is followed by a read of data[1], which wants dataLen < 2. Unreachable through newTFTPPacket, which rejects lengths 0 through 2 first, so it is only about the constructor being self consistent.

[garydgregory]

Hello @arpitjain099

Thank you for your comment.

Would please provide a failing unit test for the expectation you mention?

[arpitjain099]

Here it is, as a third case for TFTPRequestPacketOptionBoundsTest. It fails on the PR branch and passes with the change I describe below.

    /** 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.

[garydgregory]

Hello @arpitjain099

I don't see where in RFC 2347, in the general syntax, an option value is not allowed to be of length 0. That said, for specific options, like blksize, an empty value doesn't make sense. This all tells me that the check should happen at the semantic level, for specific options, and not the syntax level. How do you read the RFC (and the code)?

[arpitjain099]

You are right, and I conflated two things. RFC 2347 does not forbid a zero-length value, and an option name followed by an empty value is well formed. My test case is a different input: the packet ends right after the name's NUL, so the value's terminating NUL is absent altogether. The RFC diagram terminates every field including the value, so that packet is truncated rather than carrying an empty value.

The reason I raised it is that the branch maps both to the same result:

                                          master     PR #407
A. blksize\0\0  (zero-length, terminated)  {blksize=}  {blksize=}
B. blksize\0    (no value, no terminator)  AIOOBE      {blksize=}

A is well formed and B is truncated, and after the fix they are indistinguishable to a caller.

That said, I think your framing is the right one. Whether blksize="" is acceptable is a question for blksize, not for the parser, and a semantic check per option gets the correct answer for both A and B without the parser having to care. Master was not doing anything useful with B either, it just happened to throw the wrong exception type.

So I would not hold the PR for this. If you want the distinction preserved it is cheap to keep, but it is a nicety rather than a defect, and my earlier comment overstated it.

[garydgregory]

@arpitjain099
I've merged the PR as it currently stands as it addresses the reported issue. Thank you for your help.