acf/can: widen Can_GetCanPayloadLength return type to uint16_t

[SoundMatt]

Problem

Avtp_Can_GetCanPayloadLength (src/avtp/acf/Can.c) and
Avtp_CanBrief_GetCanPayloadLength (src/avtp/acf/CanBrief.c)
truncated mod 256 for any frame larger than ~64 quadlets:

uint8_t Avtp_Can_GetCanPayloadLength(const Avtp_Can_t* const pdu)
{
    uint8_t acf_msg_length = Avtp_Can_GetAcfMsgLength(pdu) * 4;
    uint8_t acf_pad_length = Avtp_Can_GetPad(pdu);
    return acf_msg_length - AVTP_CAN_HEADER_LEN - acf_pad_length;
}

AcfMsgLength is a 9-bit field counted in quadlets (4-byte units),
so the byte-count can be up to 2044. Storing both the intermediate
acf_msg_length and the return value in uint8_t wraps mod 256
once the frame exceeds 256 bytes. Downstream callers then read the
wrong number of payload bytes — sometimes underflowing because the
truncated value is smaller than AVTP_CAN_HEADER_LEN + pad_length,
sometimes mid-frame, depending on the actual size.

Evidence the receiving side already uses uint16_t

The IsValid helpers in this file already use uint16_t for the
same quadlet-derived computation. And in examples/acf-can/acf-can-common.c:

uint16_t can_payload_length = Avtp_Can_GetCanPayloadLength((Avtp_Can_t*)acf_pdu);

The example code already assigns the result to a uint16_t local,
suggesting someone noticed the issue but only fixed the receiving
side. This PR fixes the source.

Fix

Widen both functions' return types and the local intermediate
acf_msg_length to uint16_t. No behaviour change for frames
≤ 256 bytes; correct behaviour for larger frames.

Notes

• 4 files touched: 2 headers (include/avtp/acf/Can.h,
  include/avtp/acf/CanBrief.h), 2 sources (src/avtp/acf/Can.c,
  src/avtp/acf/CanBrief.c). The 4 changes are mechanically
  symmetric.
• I noticed several other audit-worthy issues in this codebase
  (unbounded memcpy in setters, VSS string-array iteration without
  buffer-size tracking) — happy to follow up with separate PRs once
  this lands and after maintainer guidance on the API-shape
  questions (e.g., whether setters should grow a bufferSize
  parameter to mirror the existing IsValid(pdu, bufferSize)
  helpers).

[Copilot]

Pull request overview

This PR fixes an integer-width truncation bug in the ACF CAN / CAN Brief helpers where payload lengths could wrap modulo 256 for frames larger than 256 bytes, by widening the computed byte-length and return types to uint16_t.

Changes:

• Widen Avtp_Can_GetCanPayloadLength return type and intermediate acf_msg_length to uint16_t.
• Widen Avtp_CanBrief_GetCanPayloadLength return type and intermediate acf_msg_length to uint16_t.
• Update the corresponding public header prototypes to match.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.

File	Description
src/avtp/acf/Can.c	Widen CAN payload-length computation/return type to avoid modulo-256 truncation.
src/avtp/acf/CanBrief.c	Widen CAN Brief payload-length computation/return type to avoid modulo-256 truncation.
include/avtp/acf/Can.h	Update exported CAN API prototype to return uint16_t.
include/avtp/acf/CanBrief.h	Update exported CAN Brief API prototype to return uint16_t.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[nayakned]

Hi @SoundMatt, thanks for pointing this issue and with the other PRs.

Firstly, the lengths are indeed inconsistently used in the parts (Can.c, acf-can-common.c) of the CAN code in Open1722. We need to use a consistent length throughout -- either uint8_t or uint16_t.
The maximum payload that a correctly formed CAN frame supports is 8-bytes for classic CAN and 64-bytes for CAN-FD. So actually uint8_t should suffice as return type for Avtp_Can_GetCanPayloadLength(). I would argue that returning lengths beyond 64 is already an error, as it cannot be a valid CAN frame and thus a valid IEEE 1722 ACF-CAN message.
Returning uint16_t as done in this PR allows users to create invalid packets instead of warning them.
That being said, we see in several places, we have used uint16_t instead of uint8_t, e.g., Avtp_Can_CreateAcfMessage(), Avtp_Can_Finalize(), etc. I think a consistent usage of uint8_t is a better option.
We also see that the compiler options which would have flagged this issue are not selected in the Make system.

So my idea would be:

• Add compiler options for -Wconversion -Wsign-conversion to see how many more of such issues we have.
• Use uint8_t instead of uint16_t for CAN Payload lengths.
• Fix the other warnings that the compiler throws.

I would like to hear your ideas on this, especially if you have another opinion. We are also looking into the PR for VSS. The VSS part is anyways a non-standard extension for IEEE 1722 and is more an experiment. Nonetheless, we need to fix those.

[SoundMatt]

Hi @nayakned, thanks for the considered review and the broader
framing on CAN payload bounds.

You're right that classic CAN tops out at 8 bytes and CAN-FD at 64
— uint8_t is genuinely the semantically correct type for a valid
CAN payload. I'd been reasoning from the wire-encoding range (9-bit
AcfMsgLength × 4-byte quadlets = up to 2044 bytes) rather than the
protocol's actual valid range. Your framing is better: returning
uint16_t lets the function produce values that can't correspond
to any valid CAN frame, masking invalid-frame errors rather than
surfacing them.

That said, I think the bug we found isn't really the return type
— it's the silent truncation when the wire encodes an out-of-range
length. With uint8_t throughout, if a malformed PDU encodes
AcfMsgLength = 100 quadlets, the original code computes
(uint8_t)(100 * 4) = (uint8_t)400 = 144 and returns
144 - AVTP_CAN_HEADER_LEN - pad — a plausible-looking but wrong
byte count, silently. So uint8_t alone doesn't fully address it.

A few directions that address both concerns:

1. Keep uint8_t, add a bounds check inside this function.
   Reject (return 0 or similar) when the decoded length exceeds the
   max valid CAN-FD payload + header + pad. This is the smallest
   change that preserves your architectural intent and fixes the
   silent-truncation case.

2. Document that this function trusts IsValid() was called, and
   strengthen IsValid to check the CAN-FD bound. Lightest-touch
   fix; the question is whether you'd rather have the precondition
   or the per-call check.

3. Your broader -Wconversion -Wsign-conversion cleanup. Great
   idea — would catch this and almost certainly more like it. Happy
   to take that on as a separate PR (or PR series) once the shape of
   this one is settled, since it'll likely surface a pile of related
   findings that deserve their own discussion.

How would you like to proceed with this PR?

• (a) Close it; you take the diagnosis from here and shape the fix
  yourself.
• (b) I revise this PR to keep uint8_t and add a bounds-rejection
  inside the function — narrow scope, focused on the silent
  truncation.
• (c) Close this one, open a new PR combining the bounds-rejection
  with the -Wconversion build flag, and plan the follow-up series
  for whatever warnings that surfaces.

I lean (b) for keeping scope contained and addressing the actual
runtime hazard, then (c) as the natural follow-up. But happy to
defer to your call.

I went ahead and tried what felt like a reasonable middle ground
and force-pushed an updated version of this PR — happy to revise if
you'd prefer a different shape:

• Reverted the return-type change back to uint8_t (per your point).
• Inside Avtp_Can_GetCanPayloadLength and
  Avtp_CanBrief_GetCanPayloadLength, added bounds checks: return 0
  when the decoded byte-count would underflow header + pad, or
  when the resulting payload exceeds CAN-FD's 64-byte maximum.
• Calculation done in uint16_t internally to avoid the truncation;
  the bounds-rejected result is then narrowed to uint8_t for the
  return.

This preserves the uint8_t API while addressing the silent-
truncation hazard. If you'd rather have the bound check live in
IsValid() instead, or use a different sentinel than 0, or use a
different max (e.g. 8 for classic-CAN-only), happy to adjust.

The broader -Wconversion -Wsign-conversion cleanup you proposed
is a great idea — happy to take that on as a separate PR (or PR
series) once this one's settled, since it'll likely surface more
findings that deserve their own discussion.

For the VSS-over-AVTP PR (#129), good to know it's an experimental
extension. The bugs there (dead bounds check; uint16_t overflow
in the iteration counter) fire regardless of standard status, but
happy to revise the approach there too if you'd prefer a different
shape.

[adriaan-niess]

Hi @nayakned I think using uint16 for all ACF message lenghts makes sense. Even though CAN-FD frames are relatively small (<64Byte), it's possible to encode a bigger length in an ACF-CAN message and the application should be able to read this value.

It might even be dangerous to truncate the length value when iterating through a list of ACF messages, because someone could calculate the wrong start position of the next message.

For people to not shoot themself in the foot I think it's good to have headerLength + payloadLength + padding = totalLength and leave bounds check to the application. Why not leave the 64byte boundary check to the isValid function?

[nayakned]

Hi @adriaan-niess, Agreed about using uint16_t as far as ACF message length is concerned. In fact we have to use uint16_t since ACF message lengths are 9-bit.

The question is about the API to get CAN payload length and is specific to only the CAN v1 format. This API is mainly used at the receiver/listener side to extract payload data from an ACF message and remove the padding bytes so as to regenerate the original CAN message. A value of greater than 64 for the CAN payload length renders the CAN frame is invalid. To detect the start of the next ACF message, ACF message lengths will be used which remain 16-bit.

[nayakned]

For people to not shoot themself in the foot I think it's good to have payloadLength + padding = totalLength and leave bounds check to the application. Why not leave the 64byte boundary check to the isValid function?

This is the option 2 that @SoundMatt is suggesting.

[nayakned]

• Reverted the return-type change back to uint8_t (per your point).
• Inside Avtp_Can_GetCanPayloadLength and
  Avtp_CanBrief_GetCanPayloadLength, added bounds checks: return 0
  when the decoded byte-count would underflow header + pad, or
  when the resulting payload exceeds CAN-FD's 64-byte maximum.
• Calculation done in uint16_t internally to avoid the truncation;
  the bounds-rejected result is then narrowed to uint8_t for the
  return.

@SoundMatt
I am kind of tending towards your option 2 along with the suggestion from @adriaan-niess.
We can do the bounds check in IsValid function and if required also consider the individual length bounds for CAN classic or CAN-FD. That way the full responsibility of validity check is centralized in one API. If the application is confident of receiving correctly formed frames, it can skip this check and just use the other get/set APIs. Else it can make this validity check.

The GetCanPayloadLength() can remain as it is. I think returning 0 as length is also equivalent to a "silent" truncation. If the ACF-CAN message is malformed, better discard the encapsulated CAN frame rather than use it with a truncated payload (either at 0, 8, 64 or 255)...

Lets see if others have contradicting opinion.

[SebastianSchildt]

I would vote against bounds checking here, this will seriously affect performance on applications that are doing can bridging as it will be done all the time, even though „correct“ packages do not need it.

I think a silent truncation is much better here in case of a malformed packet, because even in case of maliciously crafted packets this does not pose a security risk.

[adriaan-niess]

Agree, better move bounds check etc. in isValid() function and keep the other getters/setters very efficient.

As @nayakned suggested, an application might just skip processing an ACF-CAN message if isValid() returns false.

[SebastianSchildt]

Same argument as for Can Brief: We should not waste cycles on the bounds check here.

[nayakned]

So let me summarize. We use this pull request and do the following changes.

• Add a comment to GetCanPayloadLength() that this assumes a check for malformed packets is done using IsValid() and that no validity check will be made.
• Extend the validity checks in isValid(). CAN payload lengths checked: 8 bytes for classic CAN, 64 bytes for CAN-FD. The fdf bit in the message can be used for that. fdf = 0 for classic CAN, 1 for CAN-FD.
• Extend unit tests to test the validity checks.
• Bonus: In acf-can-common.c

  Open1722/examples/acf-can/acf-can-common.c

  Line 321 in 4bf9931

  uint16_t can_payload_length = Avtp_Can_GetCanPayloadLength((Avtp_Can_t*)acf_pdu);

  we can call the IsValid function to check for malformed packets and ignore if we detect any.

Further changes including the compiler warning flags, we do in subsequent PRs.
@SoundMatt We would be happy to merge the PR with these changes. Do make sure you have read the contribution guidelines and signed off on all commits (as already done with the current commit) and rebased on the latest version of the main branch.

[nayakned]

Changes as requested in the conversational thread.

[SoundMatt]

Hi @nayakned, @adriaan-niess, @SebastianSchildt — thanks all of you
for the careful back-and-forth. The combined position makes sense:
keep the getters lean, centralise validity in IsValid, document
the precondition.

Just to make sure I have the plan right before I start coding:

1. Revert the bounds check in GetCanPayloadLength /
   CanBrief_GetCanPayloadLength. Compute the payload length
   straightforwardly (in uint16_t internally to avoid the
   original silent-truncation hazard, narrowed back to uint8_t
   for the return), and trust the caller to have invoked IsValid
   first. Add a doc-comment on the declaration that states the
   precondition explicitly.

2. Extend Avtp_Can_IsValid and Avtp_CanBrief_IsValid to
   enforce the CAN-payload-length invariant: ≤ 8 bytes for
   classic CAN (fdf == 0), ≤ 64 bytes for CAN-FD (fdf == 1).
   Both PDU types already expose an fdf field via
   Avtp_Can_GetFdf / Avtp_CanBrief_GetFdf, so the check shape
   matches across both.

3. Add unit tests in unit/test-can.c for the new IsValid
   invariants — a valid classic-CAN frame, a valid CAN-FD frame at
   the 64-byte boundary, and over-boundary frames for each that
   fail the check.

4. Bonus update to examples/acf-can/acf-can-common.c:321 to
   call IsValid before using the payload length, ignoring
   messages that fail.

5. Compiler-warnings cleanup is tracked separately in Improved compiler settings #131 — glad
   to see that's already moving; I won't duplicate work there.

One practical ask: I haven't yet got the existing cmocka unit/
suite building cleanly on macOS, so the test-additions piece (3)
is the part where I'd most appreciate any pointers. I'll work it
out from test-can.c and existing fixtures, and will ping if I
get stuck.

I've pushed (1), (2), and the bonus (4) as a follow-up commit on
top of the existing PR (rebased onto current main). (3) tests
i would like help on.

[nayakned]

One practical ask: I haven't yet got the existing cmocka unit/
suite building cleanly on macOS, so the test-additions piece (3)
is the part where I'd most appreciate any pointers.

We provide a devcontainer with the libcmocka library installed in it.
You would not require building it on your local machine.

[SoundMatt]

The CI failure is a single assertion in unit/test-can.c::can_is_valid,
line 131:

// Valid IEEE 1722 CAN Frame
Avtp_Can_Init((Avtp_Can_t*)pdu);
assert_int_equal(Avtp_Can_IsValid((Avtp_Can_t*)pdu, MAX_PDU_SIZE), 1);

After Avtp_Can_Init, AcfMsgLength == 0 (the init only sets the
ACF type byte). With the new IsValid check we agreed on in the
thread above — payload length ≤ 8 bytes for classic CAN, ≤ 64 for
CAN-FD, with the underflow guard msg_length_bytes >= header_and_pad
— a frame with AcfMsgLength == 0 is rejected because
0 < AVTP_CAN_HEADER_LEN (16). The test's comment calls this a
"Valid IEEE 1722 CAN Frame," but a frame with no message length
declared isn't valid by the IEEE 1722 ACF wire format either —
AcfMsgLength is required to cover at least the header.

The other three assertions in can_is_valid (lines 134, 140, 145)
all pass under the new check, including the AcfMsgLength == 6
case at line 140, where the payload size works out to exactly 8
bytes (24 − 16-byte header − 0 pad), within the classic-CAN limit.

Two reasonable paths from here, and I'd like your call on which
one you want:

Option A — revert the IsValid extension. Keep the previous
semantics (only buffer-fit + ACF-type check) and drop the
payload-length / underflow / FDF-aware checks. The
GetCanPayloadLength simplification still goes in. The test
suite stays untouched. Downside: the bound you specifically asked
for in this thread doesn't get enforced, and a malformed frame
could still feed GetCanPayloadLength an out-of-range
AcfMsgLength, producing a wrong but (uint8_t)-truncated
payload size — the original silent-truncation hazard returns,
just routed via "caller didn't check IsValid."

Option B — update the test fixture to match the agreed-on
stricter semantics. The first assertion's fixture (line 131) is
incorrect by the new (and arguably the original) IEEE 1722 wire-
format definition — it asserts that an unset PDU is valid, which
was true under the old looser IsValid but isn't true under the
spec-faithful one. Replace with a properly-formed fixture (e.g.
Avtp_Can_SetAcfMsgLength((Avtp_Can_t*)pdu, 4); for a header-
only frame, or use Avtp_Can_CreateAcfMessage to construct a
real one) and re-run.

I lean B, because:

1. The whole reason for the IsValid extension was the consensus
   between you, @adriaan-niess, and @SebastianSchildt that
   IsValid should be the central place where CAN-spec
   conformance is enforced. The test fixture pre-dates that
   consensus and was verifying the looser old semantics.
2. The test comment "Valid IEEE 1722 CAN Frame" is materially
   wrong under the spec: AcfMsgLength == 0 describes no header,
   no payload, no padding — there's nothing for the receiver to
   parse.
3. The fix is a one-line update to the test fixture, no behaviour
   change to anyone using the API correctly.
4. Project hygiene. A module should be able to stand alone:
   its public API should fail (or at least not silently corrupt)
   when called outside the prescribed flow. Right now, if a
   downstream user forgets to call IsValid and feeds a
   malformed frame into GetCanPayloadLength, they get a
   plausible-looking but wrong byte count back. That's the exact
   bug this PR set out to fix; reverting (Option A) brings it
   back, just relocated. Even granting the perf argument that the
   getter shouldn't re-validate, the module as a whole should
   enforce its invariants somewhere reachable, and IsValid plus
   a tested fixture pinning that contract is the cheapest way to
   document and lock in that contract.

But this is your codebase and your test suite, so if you'd rather
take Option A and address the bound elsewhere, that's a clean call
too. Let me know which direction you'd like and I'll push the
matching change as a follow-up commit on this branch.

[SebastianSchildt]

I would agree that B - change the test and replace it with one (or more) building an actually correct/useful frame is defintely the better option.
(opionally -in addition - maybe keep the old "empty" frame constructed now, but assert isValid should fail.

In any case some form of B. (And not reverting the code to make this bogus test pass)

[SoundMatt]

Pushed Option B as a third commit on this branch:

• Replaced the Avtp_Can_Init + assert IsValid==1 assertion at
  unit/test-can.c:131 with a properly-formed classic CAN frame
  constructed via Avtp_Can_CreateAcfMessage (8-byte payload,
  classic max).
• Kept the original empty-Init frame as a negative case asserting
  IsValid == 0 (per @SebastianSchildt's suggestion in the thread).
• Added three new assertions for the payload bounds the extended
  IsValid now enforces:
  • classic CAN with a 12-byte payload — rejected (> 8 byte limit)
  • CAN-FD with a 64-byte payload — accepted (at the FD limit)
  • CAN-FD with a 68-byte payload — rejected (> 64 byte limit)

CI should now pass. Ping me if you want any of the new fixtures
shaped differently.