rdp: fix tx id handling

Tx ID handling did not take the required + 1 into account.

From a report:

        RDP can skip cleanup because its id convention does not match the
        generic Rust iterator. The generic iterator in applayer.rs returns
        tx.id() - 1, and cleanup trusts that id when calling StateTransactionFree
        in app-layer-parser.c. RDP registers that iterator in rdp.rs, but
        RdpTransaction::id() returns the stored id unchanged in rdp.rs, while
        free_tx also compares against the raw stored id in rdp.rs. For a single
        freeable RDP tx with stored id 1, the iterator returns C id 0; cleanup
        calls free_tx(0), nothing is removed, then has_next == false allows
        min_id to advance to total_txs in app-layer-parser.c. That leaves the
        tx live but now below min_id, so later cleanup will not revisit it.

This patch brings the handling in line with the other parsers.

Bug: #8717.
pull/15789/head
Victor Julien 1 month ago
parent 9e31a21aba
commit 9555e3add6

@ -146,7 +146,7 @@ impl RdpState {
let mut index = 0; let mut index = 0;
for ii in 0..len { for ii in 0..len {
let tx = &self.transactions[ii]; let tx = &self.transactions[ii];
if tx.id == tx_id { if tx.id == tx_id + 1 {
found = true; found = true;
index = ii; index = ii;
break; break;
@ -158,7 +158,7 @@ impl RdpState {
} }
fn get_tx(&self, tx_id: u64) -> Option<&RdpTransaction> { fn get_tx(&self, tx_id: u64) -> Option<&RdpTransaction> {
self.transactions.iter().find(|&tx| tx.id == tx_id) self.transactions.iter().find(|&tx| tx.id == tx_id + 1)
} }
fn new_tx(&mut self, item: RdpTransactionItem, direction: Direction) -> RdpTransaction { fn new_tx(&mut self, item: RdpTransactionItem, direction: Direction) -> RdpTransaction {
@ -721,7 +721,7 @@ mod tests {
state.transactions.push_back(tx0); state.transactions.push_back(tx0);
state.transactions.push_back(tx1); state.transactions.push_back(tx1);
state.transactions.push_back(tx2); state.transactions.push_back(tx2);
assert_eq!(Some(&state.transactions[1]), state.get_tx(2)); assert_eq!(Some(&state.transactions[1]), state.get_tx(1));
} }
#[test] #[test]
@ -742,11 +742,11 @@ mod tests {
state.transactions.push_back(tx0); state.transactions.push_back(tx0);
state.transactions.push_back(tx1); state.transactions.push_back(tx1);
state.transactions.push_back(tx2); state.transactions.push_back(tx2);
state.free_tx(1); state.free_tx(0);
assert_eq!(3, state.next_id); assert_eq!(3, state.next_id);
assert_eq!(2, state.transactions.len()); assert_eq!(2, state.transactions.len());
assert_eq!(2, state.transactions[0].id); assert_eq!(2, state.transactions[0].id);
assert_eq!(3, state.transactions[1].id); assert_eq!(3, state.transactions[1].id);
assert_eq!(None, state.get_tx(1)); assert_eq!(None, state.get_tx(0));
} }
} }

Loading…
Cancel
Save