arpitjain099 commented on PR #407:
URL: https://github.com/apache/commons-net/pull/407#issuecomment-5285392846
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.
--
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]
