Skip to content

Commit

Permalink
Compare thin pointers to find the connection
Browse files Browse the repository at this point in the history
Connection is a trait, so "*const Connection" is a fat pointer (128
bits, 64 bits pointing to the instance, 64 bits pointing to the vtable).

For some reason (a bug?), in Router::remove(), the connection passed as
argument has a vtable different from the (same) connection stored in the
"connections" field.

This has the unexpected consequence that for some x and y:

    println!("{:p} == {:p} ? {}", x, y, ptr::eq(x, y));

prints:

    0x7f87a8c3a618 == 0x7f87a8c3a618 ? false

Thus, the connection to remove was not found, leading to the panic
"Removing an unknown connection".

Instead, compare only the data part, by casting the fat pointers to thin
pointers.

See <#61 (comment)>

Thanks to mbrubeck on IRC freenode/##rust, who helped a lot to
understand the issue.
  • Loading branch information
rom1v committed Mar 6, 2018
1 parent 235ce78 commit c36fa4d
Show file tree
Hide file tree
Showing 2 changed files with 11 additions and 2 deletions.
9 changes: 9 additions & 0 deletions relay-rust/src/relay/binary.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,3 +35,12 @@ pub fn to_string(data: &[u8]) -> String {
}
s
}

// only compare the data part for fat pointers (ignore the vtable part)
// for some (buggy) reason, the vtable part may be different even if the data reference the same
// object
// See <https://github.com/Genymobile/gnirehtet/issues/61#issuecomment-370933770>
pub fn ptr_data_eq<T: ?Sized>(lhs: *const T, rhs: *const T) -> bool {
// cast to thin pointers to ignore the vtable part
lhs as *const () == rhs as *const ()
}
4 changes: 2 additions & 2 deletions relay-rust/src/relay/router.rs
Original file line number Diff line number Diff line change
Expand Up @@ -150,8 +150,8 @@ impl Router {
let index = self.connections
.iter()
.position(|item| {
// compare pointers to find the connection to remove
ptr::eq(connection, item.as_ptr())
// compare (thin) pointers to find the connection to remove
binary::ptr_data_eq(connection, item.as_ptr())
})
.expect("Removing an unknown connection");
debug!(
Expand Down

0 comments on commit c36fa4d

Please sign in to comment.