cloudwego / cloudwego/pilota

read_struct_end does not restore last_read_field_id, corrupting field ids decoded after nested structs

Open Beginner friendly
#371 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Rust
Stars
167
Forks
25
PR merge metrics
No merged PRs in 30d

Description

## Bug Report

### Version

```
pilota 0.13.1
```

### Platform

```
Linux x86_64
```

### Crates

- `pilota` — `pilota/src/thrift/compact.rs`, affects both `TCompactInputProtocol` and `TAsyncCompactProtocol`

### Description

In pilota 0.13.1, the Thrift compact protocol reader does not restore the outer struct's field-id base when a nested struct ends. `read_struct_begin` pushes the current `last_read_field_id` onto a stack and resets it, but `read_struct_end` is a no-op (`Ok(())`): it neither pops the stack nor writes the popped value back — unlike the write side, whose `write_struct_end` pops and restores `last_write_field_id`. Since compact protocol encodes field ids as deltas against `last_read_field_id`, any field decoded after a nested struct gets a wrong id, which then triggers a decode error.

I tried this code (write `Outer { 1:"a"(string), 3:"b"(string), 5:[Inner{1:i32, 2:i32}], 6:"c"(string) }`, then read it back):

```rust
let mut trans = BytesMut::new();
let mut o = test_output_prot_bytesmut(&mut trans);
// outer: field1 string, field3 string, field5 list, field6 string
o.write_struct_begin(&TStructIdentifier { name: "Outer" }).unwrap();
o.write_field_begin(TType::Binary, 1).unwrap();
o.write_string("a").unwrap();
o.write_field_end().unwrap();
o.write_field_begin(TType::Binary, 3).unwrap();
o.write_string("b").unwrap();
o.write_field_end().unwrap();
o.write_field_begin(TType::List, 5).unwrap();
o.write_list_begin(TListIdentifier { element_type: TType::Struct, size: 1 }).unwrap();
o.write_struct_begin(&TStructIdentifier { name: "Inner" }).unwrap();
o.write_field_begin(TType::I32, 1).unwrap();
o.write_i32(1).unwrap();
o.write_field_end().unwrap();
o.write_field_begin(TType::I32, 2).unwrap();
o.write_i32(2).unwrap();
o.write_field_end().unwrap();
o.write_field_stop().unwrap();
o.write_struct_end().unwrap();
o.write_list_end().unwrap();
o.write_field_end().unwrap();
// regression point: field AFTER nested struct
o.write_field_begin(TType::Binary, 6).unwrap();
o.write_string("c").unwrap();
o.write_field_end().unwrap();
o.write_field_stop().unwrap();
o.write_struct_end().unwrap();

let mut bytes = Bytes::copy_from_slice(&trans[..]);
let mut input = test_input_prot_bytes(&mut bytes);
input.read_struct_begin().unwrap();
assert_eq!(input.read_field_begin().unwrap().id, Some(1));
assert_eq!(input.read_string().unwrap(), "a");
input.read_field_end().unwrap();
assert_eq!(input.read_field_begin().unwrap().id, Some(3));
assert_eq!(input.read_string().unwrap(), "b");
input.read_field_end().unwrap();
assert_eq!(input.read_field_begin().unwrap().id, Some(5));
input.read_list_begin().unwrap();
input.read_struct_begin().unwrap();
input.read_field_begin().unwrap();
input.read_i32().unwrap();
input.read_field_end().unwrap();
input.read_field_begin().unwrap();
input.read_i32().unwrap();
input.read_field_end().unwrap();
input.read_field_begin().unwrap(); // STOP
input.read_struct_end().unwrap();
input.read_list_end().unwrap();
input.read_field_end().unwrap();

let f = input.read_field_begin().unwrap(); // BUG: returns Some(3) on unpatched 0.13.1
assert_eq!(f.id, Some(6));
assert_eq!(input.read_string().unwrap(), "c");
```

I expected to see this happen: the field after the nested struct is decoded with its real id `6`, and decoding completes normally.

Instead, this happened: `read_field_begin` returns `Some(3)` — computed from the stale `last_read_field_id = 2` plus the compact delta `1` . Verified locally: the test above fails on unpatched pilota 0.13.1 with `left: Some(3), right: Some(6)`.

Contributor guide

Open the contributing guide

Research direction

Start in pilota/src/thrift/compact.rs at read_struct_begin and read_struct_end, then compare both TCompactInputProtocol and TAsyncCompactProtocol with the write-side struct handling. Reproduce the nested-struct round trip from the issue and add coverage for the field after the nested struct. Done means that field decodes as id 6 and both readers complete normally.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
api
Issue type
Bug
Difficulty
2/5
Estimated time
1-3 hours
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
82/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.