hubcio commented on code in PR #3836:
URL: https://github.com/apache/iggy/pull/3836#discussion_r3756080058
##########
core/binary_protocol/src/consensus/header.rs:
##########
@@ -304,9 +303,88 @@ const _: () = {
assert!(
offset_of!(RequestHeader, user_id) == offset_of!(RequestHeader,
session) + size_of::<u64>()
);
- assert!(offset_of!(RequestHeader, reserved) + size_of::<[u8; 52]>() ==
HEADER_SIZE);
+ assert!(offset_of!(RequestHeader, reserved) + size_of::<[u8; 60]>() ==
HEADER_SIZE);
+};
+
+/// A [`RequestHeader`] AFTER the receiving node resolved its target.
+///
+/// The server-internal shape a client request travels in between shards
+/// (and follower to primary), never on the client wire. `group` carries the
+/// resolved consensus group so the owner shard can route, park, and fence
+/// WITHOUT re-decoding the payload -- clients no longer send any namespace
+/// (it is derived: plane from `operation`, partition group from the body),
+/// so this is where the derivation result lives for the internal hop.
+///
+/// Layout: identical to [`RequestHeader`] with `group` claiming the first
+/// eight reserved bytes, so the in-place `transmute_header` rewrite stays a
+/// same-size copy.
+#[repr(C)]
+#[derive(Debug, Clone, Copy, CheckedBitPattern, NoUninit)]
+pub struct RoutedRequestHeader {
+ pub checksum: u128,
+ pub checksum_body: u128,
+ pub cluster: u128,
+ pub size: u32,
+ pub view: u32,
+ pub release: u32,
+ pub command: Command2,
+ pub replica: u8,
+ pub reserved_frame: [u8; 66],
+
+ pub client: u128,
+ pub request_checksum: u128,
+ pub timestamp: u64,
+ pub request: u64,
+ pub operation: Operation,
+ pub operation_padding: [u8; 7],
+ pub session: u64,
+ pub user_id: u32,
+ /// Same offset and meaning as the leading 52 bytes of
+ /// `RequestHeader::reserved` -- this region CARRIES DATA (the
+ /// non-replicated op code range), so `group` must not displace it.
+ pub reserved: [u8; 52],
+ /// The resolved consensus group id (see `binary_protocol::namespace`),
+ /// claiming the TAIL of the client header's reserved area.
+ pub group: u64,
+}
+const _: () = {
+ assert!(size_of::<RoutedRequestHeader>() == HEADER_SIZE);
+ // Every field shared with `RequestHeader` sits at the same offset --
+ // including the data-bearing prefix of `reserved` (the non-replicated
+ // code range) -- so promotion preserves everything a client sent.
+ assert!(offset_of!(RoutedRequestHeader, client) ==
offset_of!(RequestHeader, client));
+ assert!(offset_of!(RoutedRequestHeader, session) ==
offset_of!(RequestHeader, session));
+ assert!(offset_of!(RoutedRequestHeader, user_id) ==
offset_of!(RequestHeader, user_id));
+ assert!(offset_of!(RoutedRequestHeader, reserved) ==
offset_of!(RequestHeader, reserved));
+ assert!(offset_of!(RoutedRequestHeader, group) + size_of::<u64>() ==
HEADER_SIZE);
};
+impl Default for RoutedRequestHeader {
+ fn default() -> Self {
+ Self {
+ checksum: 0,
+ checksum_body: 0,
+ cluster: 0,
+ size: 0,
+ view: 0,
+ release: 0,
+ command: Command2::Request,
Review Comment:
every other header's `Default` sets `command: Command2::Reserved` so a
half-filled header fails `validate()`; this one sets `Request`, so a
fully-defaulted `RoutedRequestHeader` passes validate carrying `client = 0` and
`operation = Reserved`. set it to `Reserved` too. the only builder that omits
`command` (the prepare template near line 2314) never validates, so it's safe.
note this is hygiene, not the security fix - the peer path reinterprets wire
bytes and never goes through `Default`.
##########
core/binary_protocol/src/consensus/header.rs:
##########
@@ -304,9 +303,88 @@ const _: () = {
assert!(
offset_of!(RequestHeader, user_id) == offset_of!(RequestHeader,
session) + size_of::<u64>()
);
- assert!(offset_of!(RequestHeader, reserved) + size_of::<[u8; 52]>() ==
HEADER_SIZE);
+ assert!(offset_of!(RequestHeader, reserved) + size_of::<[u8; 60]>() ==
HEADER_SIZE);
+};
+
+/// A [`RequestHeader`] AFTER the receiving node resolved its target.
+///
+/// The server-internal shape a client request travels in between shards
+/// (and follower to primary), never on the client wire. `group` carries the
+/// resolved consensus group so the owner shard can route, park, and fence
+/// WITHOUT re-decoding the payload -- clients no longer send any namespace
+/// (it is derived: plane from `operation`, partition group from the body),
+/// so this is where the derivation result lives for the internal hop.
+///
+/// Layout: identical to [`RequestHeader`] with `group` claiming the first
Review Comment:
this says `group` claims the first eight reserved bytes, but the field doc
just below and the actual layout put it at the tail (248..256, the last 8). an
SDK author porting from this doc places `group` at 196 and shifts every field
after it. point the struct doc at the tail.
##########
core/binary_protocol/src/consensus/header.rs:
##########
@@ -2389,11 +2539,7 @@ mod tests {
use std::mem::offset_of;
assert_eq!(size_of::<ReplyHeader>(), HEADER_SIZE);
assert_eq!(
- offset_of!(ReplyHeader, status),
- offset_of!(ReplyHeader, namespace) + size_of::<u64>()
- );
- assert_eq!(
- offset_of!(ReplyHeader, reserved) + size_of::<[u8; 28]>(),
+ offset_of!(ReplyHeader, reserved) + size_of::<[u8; 36]>(),
Review Comment:
this test lost its status-offset assertion. it used to pin `status` relative
to `namespace`; with `namespace` gone that assert was removed instead of
re-anchored, so both remaining lines just restate the compile-time `const _`
block and a test named `..._status_offset_..._pinned` pins no status offset.
the reply funnel reads `status` at 216 and four SDKs hardcode it. add
`assert_eq!(offset_of!(ReplyHeader, status), 216);`.
##########
core/metadata/src/impls/metadata.rs:
##########
@@ -1893,7 +1896,7 @@ where
}
let request = build_register_request_message(consensus, client_id,
user_id);
- // Wire path runs `RequestHeader::validate` at network boundary;
+ // Wire path runs `RoutedRequestHeader::validate` at network boundary;
// in-process skips it. debug_assert pins drift.
debug_assert!(
Review Comment:
this debug_assert used to prove the register builder emits a header the wire
path would accept, but `request` is now `Message<RoutedRequestHeader>` and its
`validate()` only checks the command byte, so it no longer pins `client != 0` /
`session == 0` / `request == 0`. the comment above was updated to name
`RoutedRequestHeader::validate`, which is exactly the validate that lost the
rules. the logout builder assert has the same downgrade. restoring the field
rules on `RoutedRequestHeader::validate` makes both real again.
##########
core/server_common/src/consensus_message.rs:
##########
@@ -606,7 +606,9 @@ where
match command {
Command2::Prepare =>
Ok(Self::Prepare(value.try_into_typed::<PrepareHeader>()?)),
- Command2::Request =>
Ok(Self::Request(value.try_into_typed::<RequestHeader>()?)),
+ Command2::Request => Ok(Self::Request(
Review Comment:
this decodes every peer-wire `Command2::Request` as `RoutedRequestHeader`,
whose `validate()` only checks the command byte - the old `RequestHeader`
decode here rejected `client == 0`. a peer can now forge a request with `client
= 0` and a metadata op that reaches `check_request` and trips the hard
`assert!(client_id != 0)` in `client_table.rs`, aborting the metadata primary
in a release build. replica auth is off by default, so any host that can reach
the replica port does it, and the elected successor is equally killable. the
same weakened validate also lets `operation == Reserved` through, which replays
a client's cached register reply. fix: give `RoutedRequestHeader::validate` the
same field rules as `RequestHeader::validate` (share one
`validate_request_fields`), or reject `Command2::Request` in
`MessageBag::try_from` since nothing puts one on the replica wire.
##########
core/binary_protocol/src/version.rs:
##########
@@ -63,7 +63,7 @@
//! `ClientVersionInfo` is the leading bytes of the login-register request
//! *body*, which itself rides inside a 256-byte VSR `RequestHeader` (see
//! `consensus::header`): `command` = `Command2::Request`, `operation` =
-//! `Operation::Register`, `namespace` = `METADATA_CONSENSUS_NAMESPACE`,
+//! `Operation::Register`, `namespace` = `METADATA_GROUP`,
Review Comment:
this still tells foreign SDK authors the login header carries `namespace =
METADATA_GROUP`, but the client header no longer has that field. an author
following it writes `1<<63` into what is now `session` and the register is
rejected. drop the `namespace` clause - the client sends no group.
##########
core/binary_protocol/src/consensus/header.rs:
##########
@@ -325,11 +403,84 @@ impl Default for RequestHeader {
request: 0,
operation: Operation::Reserved,
operation_padding: [0; 7],
- namespace: 0,
session: 0,
user_id: 0,
- reserved: [0; 52],
+ reserved: [0; 60],
+ }
+ }
+}
+
+impl RoutedRequestHeader {
+ /// Promote a client-wire [`RequestHeader`] to the server-internal routed
+ /// shape, stamping the resolved consensus `group`. Every shared field is
+ /// copied verbatim; this is the ONLY sanctioned crossing between the two
+ /// layouts (transmute-based reads across them would alias `group` with
+ /// reserved bytes).
+ #[must_use]
+ pub const fn from_request(header: &RequestHeader, group: u64) -> Self {
Review Comment:
`from_request` is the only sanctioned crossing between the two 256-byte
layouts and has no test - nothing in the test module names it or
`RoutedRequestHeader`. add one pinning that `reserved[0..52]` survives and
`group` lands at bytes 248..256, so a later reshuffle of the reserved carve
can't silently move the non-replicated op code that lives in `reserved[0..4]`.
##########
core/server-ng/src/dispatch.rs:
##########
@@ -861,6 +863,13 @@ async fn handle_client_request<B, MJ, S, SB>(
return;
}
};
+ // Promote to the server-internal routed shape at the boundary: the
+ // client wire carries no group (it is derived -- plane from `operation`,
+ // partition target from the payload), so it starts unset here and the
+ // resolution sites below stamp it before anything routes on it.
+ let request = request.transmute_header(|header, new_header: &mut
RoutedRequestHeader| {
Review Comment:
this rebuilds the whole 256-byte header on every client request, including
ping and poll, to stamp `group = 0` - and nothing reads `group` before the
metadata and partition paths overwrite it a few lines down. an in-place retype
that zeroes only the 8 group bytes is byte-for-byte equivalent and lets
`from_request` go away. best done after the validate fix, and keep the terminal
`validate` inside the retype.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
