Project

General

Profile

Bug #4732

decodeControlParameters should not throw on unrecognized TLVs

Added by Davide Pesavento 7 months ago. Updated 5 months ago.

Status:
Closed
Priority:
Normal
Assignee:
-
Start date:
Due date:
% Done:

0%

Estimated time:

Description

The current behavior breaks all consumers every time the NFD management protocol adds a new field to the ControlParameters structure.

I think this regressed in commit c06427b310a07f3a96d5592bf136a070e5fea11d, that made the existing finishNestedTlvs default to skipCritical=false. That is indeed the correct v0.3 behavior for network-layer TLV elements, but the NFD management protocol is an application-layer protocol and the evolvability rules don't apply to it.

Please revert to the previous behavior of silently ignoring unknown TLVs nested in ControlParameters and ControlResponse.

History

#1 Updated by Davide Pesavento 7 months ago

Calling finishNestedTlvs(..., true) from decodeControlParameters() may be enough to fix, but I haven't tested.

Also, the API documentation of finishNestedTlvs() doesn't mention unrecognized critical elements as a reason for throwing.

#2 Updated by Jeff Thompson 6 months ago

Hi Davide. I pushed a changed to master. Can you give it a try? (Are you able to build from source?)
https://github.com/named-data/jndn/commit/fb547eacad3d11b1ceaa70f653bd0d910e211b2c

Using finishNestedTlvs(..., true) will only skip unrecognized elements and the end of the TLV after recognized ones. If there are unrecognized TLVs the come before a recognized optional element which is present, then it might not be picked up. (The TlvDecoder does not handle TLVs in random order.)

#3 Updated by Jeff Thompson 6 months ago

  • Status changed from New to Feedback

I updated decodeControlParameters to also skip intermediate optional unused TLVs such as LocalUri.
https://github.com/named-data/jndn/commit/dba986b3ddd8d86b88c9b2f43ca9dab9703fe4a8

I pushed a new minor release to Maven Central.
https://github.com/named-data/jndn/releases/tag/v0.20

Let me know if it fixes the problem.

#4 Updated by Jeff Thompson 5 months ago

Can we close this?

#5 Updated by Davide Pesavento 5 months ago

Jeff Thompson wrote:

Can we close this?

I haven't been able to test the new version yet, but the updated code looks fine now. Feel free to close.

#6 Updated by Jeff Thompson 5 months ago

  • Status changed from Feedback to Closed

Also available in: Atom PDF