Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Resource borrows escape scope #2

Closed
rvolosatovs opened this issue Feb 5, 2025 · 0 comments · Fixed by #4
Closed

Resource borrows escape scope #2

rvolosatovs opened this issue Feb 5, 2025 · 0 comments · Fixed by #4
Assignees
Labels
bug Something isn't working

Comments

@rvolosatovs
Copy link
Member

This appears to be bindgen bug, but not entirely sure that's the case.

On d8c91c3 (from #1 )

with the following patch applied:

diff --git a/crates/wasi/tests/all/p3/sockets.rs b/crates/wasi/tests/all/p3/sockets.rs
index 79ae6fd6e..668888323 100644
--- a/crates/wasi/tests/all/p3/sockets.rs
+++ b/crates/wasi/tests/all/p3/sockets.rs
@@ -14,7 +14,7 @@ async fn sockets_0_3_tcp_bind() -> anyhow::Result<()> {
     run(SOCKETS_0_3_TCP_BIND_COMPONENT).await
 }
 
-#[ignore = "needs `listen`, but fails earlier with `cannot remove owned resource while borrowed`, likely bug in calling host async func as guest sync"]
+//#[ignore = "needs `listen`, but fails earlier with `cannot remove owned resource while borrowed`, likely bug in calling host async func as guest sync"]
 #[test_log::test(tokio::test(flavor = "multi_thread"))]
 async fn sockets_0_3_tcp_connect() -> anyhow::Result<()> {
     run(SOCKETS_0_3_TCP_CONNECT_COMPONENT).await
$ cargo test -p wasmtime-wasi p3

fails with:

---- p3::sockets::sockets_0_3_tcp_connect stdout ----
Error: failed to call `wasi:cli/run#run`

Caused by:
    0: error while executing at wasm backtrace:
           0: 0x16f3 - sockets_0_3_tcp_connect-4e61c88e2a1b13aa.wasm!_ZN107_$LT$test_programs..p3..wasi..sockets..types..TcpSocket$u20$as$u20$test_programs..p3.._rt..WasmResource$GT$4drop17h766badd7db0279c0E
           1: 0x9177 - sockets_0_3_tcp_connect-4e61c88e2a1b13aa.wasm!_ZN83_$LT$test_programs..p3.._rt..Resource$LT$T$GT$$u20$as$u20$core..ops..drop..Drop$GT$4drop17h799cf665184cedfcE
           2: 0x90f2 - sockets_0_3_tcp_connect-4e61c88e2a1b13aa.wasm!_ZN4core3ptr111drop_in_place$LT$test_programs..p3.._rt..Resource$LT$test_programs..p3..wasi..sockets..types..TcpSocket$GT$$GT$17h5664e5c9cabc8f48E
           3: 0x8da8 - sockets_0_3_tcp_connect-4e61c88e2a1b13aa.wasm!_ZN4core3ptr71drop_in_place$LT$test_programs..p3..wasi..sockets..types..TcpSocket$GT$17hd5cd934b4a5064ccE
           4: 0x2d72 - sockets_0_3_tcp_connect-4e61c88e2a1b13aa.wasm!_ZN23sockets_0_3_tcp_connect23test_tcp_connect_unspec28_$u7b$$u7b$closure$u7d$$u7d$17he78ff0683392784dE
           5: 0x1aa4 - sockets_0_3_tcp_connect-4e61c88e2a1b13aa.wasm!_ZN104_$LT$sockets_0_3_tcp_connect..Component$u20$as$u20$test_programs..p3..exports..wasi..cli..run..Guest$GT$3run28_$u7b$$u7b$closure$u7d$$u7d$17hcf195b31a240fc06E
           6: 0x9d86 - sockets_0_3_tcp_connect-4e61c88e2a1b13aa.wasm!_ZN13test_programs2p37exports4wasi3cli3run16_export_run_cabi28_$u7b$$u7b$closure$u7d$$u7d$17hbb5fa4c167cb7ffeE
           7: 0x6f45 - sockets_0_3_tcp_connect-4e61c88e2a1b13aa.wasm!_ZN102_$LT$futures_util..future..future..map..Map$LT$Fut$C$F$GT$$u20$as$u20$core..future..future..Future$GT$4poll17h41cd575fb1fa83c3E
           8: 0xa2fa - sockets_0_3_tcp_connect-4e61c88e2a1b13aa.wasm!_ZN97_$LT$futures_util..future..future..Map$LT$Fut$C$F$GT$$u20$as$u20$core..future..future..Future$GT$4poll17h1a507c353f27c32eE
           9: 0x2a15b - sockets_0_3_tcp_connect-4e61c88e2a1b13aa.wasm!_ZN72_$LT$core..pin..Pin$LT$P$GT$$u20$as$u20$core..future..future..Future$GT$4poll17hc7487fa5323d29c5E
          10: 0x29b06 - sockets_0_3_tcp_connect-4e61c88e2a1b13aa.wasm!_ZN117_$LT$futures_util..stream..futures_unordered..FuturesUnordered$LT$Fut$GT$$u20$as$u20$futures_core..stream..Stream$GT$9poll_next17h683d5b4eec9585cfE
          11: 0x2a9e8 - sockets_0_3_tcp_connect-4e61c88e2a1b13aa.wasm!_ZN12futures_util6stream6stream9StreamExt15poll_next_unpin17h1aa5245cf32f814aE
          12: 0x28249 - sockets_0_3_tcp_connect-4e61c88e2a1b13aa.wasm!_ZN14wit_bindgen_rt13async_support4poll17h35ade1930e43cb9fE
          13: 0x77e6 - sockets_0_3_tcp_connect-4e61c88e2a1b13aa.wasm!_ZN14wit_bindgen_rt13async_support10first_poll17h63a2fd7fa33ef67bE
          14: 0x9c56 - sockets_0_3_tcp_connect-4e61c88e2a1b13aa.wasm!_ZN13test_programs2p37exports4wasi3cli3run16_export_run_cabi17h3ee5542402cf0b24E
          15: 0xa021 - sockets_0_3_tcp_connect-4e61c88e2a1b13aa.wasm![async]wasi:cli/run@0.3.0#run
       note: using the `WASMTIME_BACKTRACE_DETAILS=1` environment variable may show more debugging information
    1: cannot remove owned resource while borrowed


failures:
    p3::sockets::sockets_0_3_tcp_connect

The trap happens in

/// `0.0.0.0` / `::` is not a valid remote address in WASI.
async fn test_tcp_connect_unspec(family: IpAddressFamily) {
let addr = IpSocketAddress::new(IpAddress::new_unspecified(family), SOME_PORT);
let sock = TcpSocket::new(family);
assert!(matches!(
sock.connect(addr).await,
Err(ErrorCode::InvalidArgument)
));
}
, when the Rust guest is trying to drop the TcpSocket at the end of the scope.

The issue does not happen if connect is not called.

Interestingly, the host drop is never called

fn drop(&mut self, rep: Resource<TcpSocket>) -> wasmtime::Result<()> {
self.table()
.delete(rep)
.context("failed to delete socket resource from table")?;
Ok(())
}

The host connect implementation does not create borrows and each get_mut borrow of the store should be dropped at the end of the scope

fn connect(
mut store: StoreContextMut<'_, Self::TcpSocketData>,
mut socket: Resource<TcpSocket>,
remote_address: IpSocketAddress,
) -> impl Future<
Output = impl FnOnce(
StoreContextMut<'_, Self::TcpSocketData>,
) -> wasmtime::Result<Result<(), ErrorCode>>
+ 'static,
> + 'static {
let ctx = store.data().sockets();
let allowed = ctx.allowed_network_uses.tcp;
let socket_addr_check = ctx.socket_addr_check.clone();
let sock = store
.data_mut()
.table()
.get_mut(&mut socket)
.context("failed to get socket resource from table")
.map(|socket| {
let tcp_state = mem::replace(&mut socket.tcp_state, TcpState::Connecting);
if let TcpState::Default(sock) = tcp_state {
Some((sock, false, socket.family))
} else if let TcpState::Bound(sock) = tcp_state {
Some((sock, true, socket.family))
} else {
socket.tcp_state = tcp_state;
None
}
});
let remote_address = SocketAddr::from(remote_address);
async move {
let res = match sock {
Ok(sock)
if !allowed
|| !socket_addr_check(remote_address, SocketAddrUse::TcpConnect).await =>
{
if let Some((sock, bound, ..)) = sock {
Ok(Ok(Err((sock, bound, ErrorCode::AccessDenied))))
} else {
Ok(Err(ErrorCode::AccessDenied))
}
}
Ok(Some((sock, .., family))) => {
Ok(Ok(Ok(connect(sock, remote_address, family).await)))
}
Ok(None) => Ok(Err(ErrorCode::InvalidState)),
Err(err) => Err(err),
};
for_any(move |mut store: StoreContextMut<'_, Self::TcpSocketData>| {
let sock = res?;
let socket = store
.data_mut()
.table()
.get_mut(&mut socket)
.context("failed to get socket resource from table")?;
let sock = match sock {
Ok(sock) => sock,
Err(err) => return Ok(Err(err)),
};
ensure!(
matches!(socket.tcp_state, TcpState::Connecting),
"corrupted socket state"
);
match sock {
Ok(Ok(stream)) => {
socket.tcp_state = TcpState::Connected(stream);
Ok(Ok(()))
}
Ok(Err(err)) => {
socket.tcp_state = TcpState::Closed;
Ok(Err(err))
}
Err((sock, true, err)) => {
socket.tcp_state = TcpState::Bound(sock);
Ok(Err(err))
}
Err((sock, false, err)) => {
socket.tcp_state = TcpState::Default(sock);
Ok(Err(err))
}
}
})
}
}

  • Is it possible that the guest binding fails to drop the borrow of the TcpSocket once it's done with the method call?
  • Is this related to the fact that connect is implemented as async in the host, but is used as sync in the guest?

One potential scenario could be:

  1. Borrow of TcpSocket resource is created to call connect on it
  2. Borrow from (1.) is moved to the future, in which connect is polled
  3. The future (holding the borrow) is leaked, because the guest assumes the import to be sync
@rvolosatovs rvolosatovs added the bug Something isn't working label Feb 5, 2025
@rvolosatovs rvolosatovs moved this to Ready in Ship WASIp3 Feb 5, 2025
@rvolosatovs rvolosatovs moved this from Ready to Backlog in Ship WASIp3 Feb 5, 2025
@dicej dicej self-assigned this Feb 10, 2025
dicej added a commit that referenced this issue Feb 10, 2025
I missed this the first time around.  Thanks to Roman for catching this and
providing a test case.

Fixes #2.

Signed-off-by: Joel Dice <joel.dice@fermyon.com>
@dicej dicej closed this as completed in #4 Feb 11, 2025
@dicej dicej closed this as completed in 712d633 Feb 11, 2025
@github-project-automation github-project-automation bot moved this from Backlog to Done in Ship WASIp3 Feb 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants