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