isisd: consume leftover bytes after FAD sub-sub-TLV loop

[TristanInSec]

The Flex-Algorithm sub-sub-TLV loop condition is while (subsubtlvs_len > 2), so when 1 or 2 bytes remain after the last iteration, they are not consumed. The stream position then falls behind the declared subtlv length, desynchronizing subsequent subtlv parsing in the outer loop.

Skip any leftover bytes after the while loop exits.

Signed-off-by: Tristan Madani tristan@live.fr

[greptile-apps]

Greptile Summary

This PR adds a post-loop stream_forward_getp to consume 1–2 leftover bytes that the while (subsubtlvs_len > 2) condition cannot drain, fixing a stream-position desync in the FAD sub-sub-TLV parser. The fix is correct for the normal loop-exit path but introduces a double-advance bug on the error-break path.

• When the inner length-validation check (line 5168) fires, the error handler already consumes all remaining bytes via stream_forward_getp(s, subsubtlvs_len - 2) and then breaks — but subsubtlvs_len is never decremented before the break, so the new if (subsubtlvs_len > 0) guard is true and a second stream_forward_getp(s, subsubtlvs_len) advances the stream far past the sub-TLV boundary. Changing the guard to subsubtlvs_len > 0 && subsubtlvs_len <= 2 limits the skip to the intended normal-exit case.

Confidence Score: 3/5

Not safe to merge as-is: the fix introduces a double-advance on the existing error-break path that can desync stream parsing.

The intended fix (draining 1–2 leftover bytes) is correct, but the guard condition subsubtlvs_len > 0 also fires when the loop exits via break after the invalid-length error handler, causing a second skip of up to subsubtlvs_len bytes and worsening the very desync it aims to prevent.

isisd/isis_tlvs.c — specifically the post-loop skip guard and its interaction with the error-break at line 5173.

Important Files Changed

Filename	Overview
isisd/isis_tlvs.c	Adds a post-loop skip for leftover sub-sub-TLV bytes, but the guard condition > 0 also triggers on the error-break path where bytes were already consumed, causing a double-advance.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["subsubtlvs_len = length - 4"] --> B{"subsubtlvs_len > 2?"}
    B -- Yes --> C["read type + len (2 bytes)"]
    C --> D{"subsubtlv_len >\nsubsubtlvs_len - 2?"}
    D -- Yes --> E["stream_forward_getp(s, subsubtlvs_len - 2)\nlog error"]
    E --> F["break\n(subsubtlvs_len unchanged, still > 2)"]
    D -- No --> G["process sub-sub-TLV\nstream_forward_getp for payload"]
    G --> H["subsubtlvs_len -= 2 + subsubtlv_len"]
    H --> B
    B -- No --> I{"subsubtlvs_len > 0?"}
    F --> I
    I -- "No (== 0)" --> K["done ✓"]
    I -- "Yes (1 or 2)\nnormal exit" --> J["stream_forward_getp(s, subsubtlvs_len) ✓"]
    I -- "Yes (> 2)\nerror-break path - BUG" --> L["stream_forward_getp(s, subsubtlvs_len)\ndouble-advance! ✗"]
    J --> K
    L --> M["stream desync - worse than original bug"]

Loading

Prompt To Fix All With AI

This is a comment left during a code review.
Path: isisd/isis_tlvs.c
Line: 5251-5252

Comment:
**Double-advance on error-break path**

When the inner validation check at line 5168 fires (`subsubtlv_len > subsubtlvs_len - 2`), the error handler reads 2 bytes (type + length) and then calls `stream_forward_getp(s, subsubtlvs_len - 2)`, consuming all remaining bytes of this sub-TLV. It then `break`s out of the while loop with `subsubtlvs_len` still holding its pre-break value (always > 2). The new guard `subsubtlvs_len > 0` is true, so `stream_forward_getp(s, subsubtlvs_len)` is called a second time, advancing the stream far past the sub-TLV boundary and causing exactly the kind of desync the fix intends to cure.

The skip should only apply when the loop exited normally (i.e. `subsubtlvs_len` drained to ≤ 2), not when it `break`ed early:

```suggestion
			if (subsubtlvs_len > 0 && subsubtlvs_len <= 2)
				stream_forward_getp(s, subsubtlvs_len);
```

How can I resolve this? If you propose a fix, please make it concise.

_{Reviews (1): Last reviewed commit: "isisd: consume leftover bytes after FAD ..." | Re-trigger Greptile}

[greptile-apps]

Double-advance on error-break path

When the inner validation check at line 5168 fires (subsubtlv_len > subsubtlvs_len - 2), the error handler reads 2 bytes (type + length) and then calls stream_forward_getp(s, subsubtlvs_len - 2), consuming all remaining bytes of this sub-TLV. It then breaks out of the while loop with subsubtlvs_len still holding its pre-break value (always > 2). The new guard subsubtlvs_len > 0 is true, so stream_forward_getp(s, subsubtlvs_len) is called a second time, advancing the stream far past the sub-TLV boundary and causing exactly the kind of desync the fix intends to cure.

The skip should only apply when the loop exited normally (i.e. subsubtlvs_len drained to ≤ 2), not when it breaked early:

Suggested change

	if (subsubtlvs_len > 0)
	stream_forward_getp(s, subsubtlvs_len);
	if (subsubtlvs_len > 0 && subsubtlvs_len <= 2)
	stream_forward_getp(s, subsubtlvs_len);

Prompt To Fix With AI

This is a comment left during a code review.
Path: isisd/isis_tlvs.c
Line: 5251-5252

Comment:
**Double-advance on error-break path**

When the inner validation check at line 5168 fires (`subsubtlv_len > subsubtlvs_len - 2`), the error handler reads 2 bytes (type + length) and then calls `stream_forward_getp(s, subsubtlvs_len - 2)`, consuming all remaining bytes of this sub-TLV. It then `break`s out of the while loop with `subsubtlvs_len` still holding its pre-break value (always > 2). The new guard `subsubtlvs_len > 0` is true, so `stream_forward_getp(s, subsubtlvs_len)` is called a second time, advancing the stream far past the sub-TLV boundary and causing exactly the kind of desync the fix intends to cure.

The skip should only apply when the loop exited normally (i.e. `subsubtlvs_len` drained to ≤ 2), not when it `break`ed early:

```suggestion
			if (subsubtlvs_len > 0 && subsubtlvs_len <= 2)
				stream_forward_getp(s, subsubtlvs_len);
```

How can I resolve this? If you propose a fix, please make it concise.

[mjstapp]

this comment looks valid to me

[TristanInSec]

Valid finding. Fixed the guard to only fire on normal loop exit (1-2 leftover bytes), not on the error-break path where bytes were already consumed.

[mjstapp]

same commit issue here - please squash into the first, properly-formatted commit

[mjstapp]

CI:rerun

[donaldsharp]

@Mergifyio backport stable/10.6 stable/10.5 stable/10.4 stable/10.3 stable/10.2 stable/10.1 stable/10.0

[mergify]

backport stable/10.6 stable/10.5 stable/10.4 stable/10.3 stable/10.2 stable/10.1 stable/10.0

✅ Backports have been created

Details

• #21642 isisd: consume leftover bytes after FAD sub-sub-TLV loop (backport #21544) has been created for branch stable/10.6
• #21643 isisd: consume leftover bytes after FAD sub-sub-TLV loop (backport #21544) has been created for branch stable/10.5
• #21644 isisd: consume leftover bytes after FAD sub-sub-TLV loop (backport #21544) has been created for branch stable/10.4
• #21645 isisd: consume leftover bytes after FAD sub-sub-TLV loop (backport #21544) has been created for branch stable/10.3
• #21646 isisd: consume leftover bytes after FAD sub-sub-TLV loop (backport #21544) has been created for branch stable/10.2
• #21647 isisd: consume leftover bytes after FAD sub-sub-TLV loop (backport #21544) has been created for branch stable/10.1
• #21648 isisd: consume leftover bytes after FAD sub-sub-TLV loop (backport #21544) has been created for branch stable/10.0