kpumuk commented on PR #3414:
URL: https://github.com/apache/thrift/pull/3414#issuecomment-4339750874
There are two remaining edge cases that look unsafe.
### 1. Nested forward typedefs still lose the `Box`
Minimal IDL:
```thrift
typedef FutureUnion ForwardAlias
typedef ForwardAlias NestedForwardAlias
struct Wrapper {
1: optional NestedForwardAlias value
}
union FutureUnion {
1: i32 n
}
```
The Rust generator emits aliases like:
```rust
pub type ForwardAlias = Box<FutureUnion>;
pub type NestedForwardAlias = ForwardAlias;
```
But the new special-case union read path reads the resolved union type
directly and assigns it into the field temp:
```rust
let mut f_1: Option<NestedForwardAlias> = None;
match FutureUnion::read_from_in_protocol(i_prot) {
Ok(val) => { f_1 = Some(val); },
Err(thrift::Error::Protocol(ref e))
if e.kind == ProtocolErrorKind::UnknownUnionVariant => {}
Err(e) => return Err(e),
}
```
That is a generated-code compile error: `val` is `FutureUnion`, while
`NestedForwardAlias` is `Box<FutureUnion>`.
```text
error[E0308]: mismatched types
expected `Box<FutureUnion>`, found `FutureUnion`
```
The boxing decision needs to preserve boxing through the typedef chain, not
only inspect the outer field typedef.
### 2. Multiple unknown union fields are treated as ignorable
The union reader currently maps `received_field_count == 0` to
`UnknownUnionVariant` before rejecting multi-field unions. That also matches a
union payload with two unknown fields. The parent struct reader then catches
`UnknownUnionVariant` and silently leaves the optional union field as `None`.
Regression test shape:
```rust
let mut write_buf = Vec::new();
{
let cursor = Cursor::new(&mut write_buf);
let mut prot = TBinaryOutputProtocol::new(cursor, false);
prot.write_struct_begin(&TStructIdentifier::new("Wrapper")).unwrap();
prot.write_field_begin(&TFieldIdentifier::new("u", TType::Struct,
1)).unwrap();
prot.write_struct_begin(&TStructIdentifier::new("U")).unwrap();
prot.write_field_begin(&TFieldIdentifier::new("future_a", TType::I32,
99)).unwrap();
prot.write_i32(1).unwrap();
prot.write_field_end().unwrap();
prot.write_field_begin(&TFieldIdentifier::new("future_b", TType::I32,
100)).unwrap();
prot.write_i32(2).unwrap();
prot.write_field_end().unwrap();
prot.write_field_stop().unwrap();
prot.write_struct_end().unwrap();
prot.write_field_end().unwrap();
prot.write_field_stop().unwrap();
prot.write_struct_end().unwrap();
}
let read_cursor = Cursor::new(write_buf);
let mut rprot = TBinaryInputProtocol::new(read_cursor, false);
let err = Wrapper::read_from_in_protocol(&mut rprot)
.expect_err("multi-field union should stay invalid");
```
A union with multiple fields is invalid even if none of those fields are
known locally. The checks should preserve that:
```text
total_field_count == 0 => EmptyUnion
total_field_count > 1 => InvalidData
total_field_count == 1 && received_count == 0 => UnknownUnionVariant
```
--
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]
