google / google/emboss

Using $next in conditional fields generates incorrect offsets

Open
#206 4 comments 0 reactions 0 assignees View on GitHub
Dominant language
Python
Stars
92
Forks
30
PR merge metrics
No merged PRs in 30d

Description

I'm writing a definition for a protocol where the length field is dynamically sized. I've noted that all the fields after the conditional that pulls the extended length portion out are pointing at the wrong offset. It seems that $next is not doing what one would expect in the presence of a conditional.

Definition:
```
enum RfcommFrameType:
[maximum_bits: 8]
SET_ASYNC_BALANCED_MODE = 0x2f
UNNUMBERED_ACK = 0x63
DISCONNECT_MODE = 0x0f
DISCONNECT = 0xc3
UNNUMBERED_INFORMATION_WITH_HEADER_CHECK = 0xef
UNNUMBERED_INFORMATION_WITH_HEADER_CHECK_AND_POLL_FINAL = 0xff
-- RFCOMM extension. With Poll/Final bit set in UIH control byte, a credits
-- field is the first byte of the payload.

enum RfcommFixedChannel:
[maximum_bits: 8]
CONTROL = 0

enum RfcommCommandResponse:
-- Commands from the initiator and responses from the responder
-- have C/R = 0. Commands from the responder and responses from
-- the initiator have C/R = 1.
[maximum_bits: 1]
COMMAND = 0
RESPONSE = 1

enum RfcommDirection:
[maximum_bits: 1]
RESPONDER = 0
INITIATOR = 1

enum RfcommLengthExtended:
[maximum_bits: 1]
NORMAL = 1
EXTENDED = 0

struct RfcommFrame:
-- RFCOMM TS 07.10 frame
[requires: extended_address == true]
0 [+1] bits:
0 [+1] Flag extended_address
1 [+1] RfcommCommandResponse command_response
2 [+1] RfcommDirection direction
3 [+5] UInt channel

1 [+1] RfcommFrameType control

2 [+1] bits:
0 [+1] RfcommLengthExtended length_extended_flag
1 [+7] UInt length

if length_extended_flag == RfcommLengthExtended.EXTENDED:
$next [+1] UInt length_extended

if control == RfcommFrameType.UNNUMBERED_INFORMATION_WITH_HEADER_CHECK_AND_POLL_FINAL && channel != 0:
$next [+1] UInt credits
-- Credits field can appears as first byte of payload when Poll/Final bit is
-- set on UIH frames and channel is not control (0).

let combined_length = $present(length_extended) ? length * 256 + length_extended : length

# If present, credits is counted as part of payload.
let payload_length = $present(credits) ? combined_length - 1 : combined_length

$next [+payload_length] UInt:8[payload_length] payload

$next [+1] UInt fcs
-- Frame checksum. See: GSM 07.10 TS 101 369 V6.3.0
-- SABM, DISC, UA, DM frame types:
-- FCS should be calculated over address, control and length fields.
-- UIH frame type:
-- FCS should be calculated over address and control fields.
```

Everything after
```
if length_extended_flag == RfcommLengthExtended.EXTENDED:
$next [+1] UInt length_extended
```

is pointing one byte off, even if the `length_extended_flag != RfcommLengthExtended.EXTENDED`. I confirmed this with a `!has_length_extended()` check and looking at the `emboss_reserved_local_offset` variable in the generated code for each field.

For example one would expect `credits` to have offset `3` when `length_extended_flag != RfcommLengthExtended.EXTENDED`, but it actually has `4`.

On a related note: `IntrinsicSizeInBytes().Read()` reports one more byte than I would expect on this struct as well. Like it isn't taking into account the conditionals correctly.

There's some messy code that uses the generated stuff here for more context https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/246974

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.