[protonnegativo]

Summary

AlternateTimeOffsetIndicatorTLV did not handle short displayNames correctly, in either direction.

The TLV body is 15 octets of fixed fields (keyField, currentOffset, jumpSeconds, timeOfNextJump) followed by a PTPText displayName, which is 1 + N octets plus a pad octet when needed. So the smallest valid lengthField is 16.

• Decoding rejected valid TLVs. UnmarshalBinary required lengthField >= 20. An ATOI with an empty displayName (lengthField 16) or a one-character one (18) failed with expected TLV ... to have length of at least 20. Because readTLVs returns that error, the whole Announce failed in DecodePacket.
• The displayName was not bounded by the TLV. It was parsed from b[tlvHeadSize+15:], which is the rest of the packet. If the text length octet claimed more than the TLV holds, the name quietly took in the bytes of the next TLV. For example, an ATOI with lengthField 20 and length octet 8, followed by an ALTERNATE_RESPONSE_PORT TLV, decoded with DisplayName == "PTP\x00\x80\x01\x00\x02" and no error.
• Encoding dropped the length octet. With an empty displayName, MarshalBinaryTo wrote only the 15 fixed octets and left out the mandatory PTPText length octet (19 bytes instead of 20).

The fix:

• lowers the minimum length to 16;
• parses the displayName from b[tlvHeadSize+15 : tlvHeadSize+lengthField], which checkTLVLength already guarantees is in range;
• always marshals the displayName (an empty one is a single zero octet). The existing buffer-size check after MarshalBinary covers the extra octet, so tlvMinSize is unchanged.

Behaviour change worth knowing: an ATOI whose displayName length octet runs past its lengthField now returns an error instead of decoding with a garbage name, and lengthField 16-19 is now accepted.

Test Plan

Added three tests in ptp/protocol/tlvs_test.go:

• an Announce carrying an ATOI with an empty displayName (decode + Bytes round trip);
• a one-character displayName (decode + MarshalBinaryTo round trip, covers the pad octet);
• a displayName length octet that overruns the TLV (must error).

Without the change to tlvs.go:

$ go test ./ptp/protocol/ -run AlternateTimeOffset
--- FAIL: TestParseAnnounceWithAlternateTimeOffsetIndicatorEmptyDisplayName (0.00s)
        	Error:      	Received unexpected error:
--- FAIL: TestParseAlternateTimeOffsetIndicatorDisplayNameOverrun (0.00s)
        	Error:      	An error is expected but got nil.
--- FAIL: TestAlternateTimeOffsetIndicatorOneCharDisplayName (0.00s)
        	Error:      	Received unexpected error:
FAIL

With it:

$ go test ./ptp/protocol/ -run AlternateTimeOffset -v
--- PASS: TestParseAnnounceWithAlternateTimeOffsetIndicator (0.00s)
--- PASS: TestParseAnnounceWithAlternateTimeOffsetIndicatorEmptyDisplayName (0.00s)
--- PASS: TestParseAlternateTimeOffsetIndicatorDisplayNameOverrun (0.00s)
--- PASS: TestAlternateTimeOffsetIndicatorOneCharDisplayName (0.00s)
ok  	github.com/facebook/time/ptp/protocol

Also ran go test ./ptp/... (all ok), go vet ./ptp/protocol/, gofmt -l ptp/protocol (clean), and go test ./ptp/protocol -run '^$' -fuzz '^FuzzDecodePacket$' -fuzztime 30s (no failures).

[meta-cla]

Hi @protonnegativo!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks!

[meta-cla]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

[vvfedorenko]

Hi @protonnegativo! Could you please check why Fuzz test is failing on this PR?