Stream: git-wasmtime

Topic: wasmtime / PR #6444 fix: respect `fd_prestat_dir_name` `p...

view this post on Zulip Wasmtime GitHub notifications bot (May 24 2023 at 14:17):

rvolosatovs opened PR #6444 from rvolosatovs:fix/fd_prestat_dir_name to bytecodealliance:main:

Refs https://github.com/bytecodealliance/wasmtime/pull/6440#discussion_r1202998079

view this post on Zulip Wasmtime GitHub notifications bot (May 24 2023 at 14:45):

rvolosatovs updated PR #6444.

view this post on Zulip Wasmtime GitHub notifications bot (May 24 2023 at 14:56):

rvolosatovs updated PR #6444.

view this post on Zulip Wasmtime GitHub notifications bot (May 24 2023 at 14:57):

rvolosatovs updated PR #6444.

view this post on Zulip Wasmtime GitHub notifications bot (May 24 2023 at 15:35):

rvolosatovs updated PR #6444.

view this post on Zulip Wasmtime GitHub notifications bot (May 24 2023 at 16:00):

rvolosatovs has marked PR #6444 as ready for review.

view this post on Zulip Wasmtime GitHub notifications bot (May 24 2023 at 16:00):

rvolosatovs requested jameysharp for a review on PR #6444.

view this post on Zulip Wasmtime GitHub notifications bot (May 24 2023 at 16:00):

rvolosatovs requested wasmtime-core-reviewers for a review on PR #6444.

view this post on Zulip Wasmtime GitHub notifications bot (May 24 2023 at 16:25):

jameysharp submitted PR review:

I think this is right but I'd like @pchickey to take a look.

view this post on Zulip Wasmtime GitHub notifications bot (May 24 2023 at 16:39):

pchickey created PR review comment:

Can we also add a case where we pass a buffer which is 1 byte longer than required

view this post on Zulip Wasmtime GitHub notifications bot (May 24 2023 at 17:06):

rvolosatovs updated PR #6444.

view this post on Zulip Wasmtime GitHub notifications bot (May 24 2023 at 17:23):

pchickey created PR review comment:

    // Test that passing a larger than necessary buffer works correctly
    let r = wasi::fd_prestat_dir_name(pr_fd, vec![0; pr_name_len + 1].as_mut_ptr(), pr_name_len + 1);

view this post on Zulip Wasmtime GitHub notifications bot (May 24 2023 at 17:23):

pchickey created PR review comment:

I'd rather not risk this being UB in the case of an incorrect implementation

view this post on Zulip Wasmtime GitHub notifications bot (May 24 2023 at 17:28):

rvolosatovs updated PR #6444.

view this post on Zulip Wasmtime GitHub notifications bot (May 24 2023 at 17:30):

pchickey submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (May 24 2023 at 17:30):

pchickey has enabled auto merge for PR #6444.

view this post on Zulip Wasmtime GitHub notifications bot (May 24 2023 at 17:34):

alexcrichton created PR review comment:

I believe that this as-written is ok, but I'd recommend avoiding this pattern in the future as it's so easy to get wrong. This only works if the vec![...] allocate here lives longer than the function call which I think rustc's temporary rules satisfy here, but I at least personally think it'd be more clear to move the vec above this statement to ensure it lives across the statement.

view this post on Zulip Wasmtime GitHub notifications bot (May 25 2023 at 11:19):

rvolosatovs updated PR #6444.

view this post on Zulip Wasmtime GitHub notifications bot (May 25 2023 at 11:22):

rvolosatovs created PR review comment:

Thanks! Yes, good point, I would guess that miri should complain about this or at least I remember seeing it fail on a similar pattern in a different project before.

view this post on Zulip Wasmtime GitHub notifications bot (May 25 2023 at 11:33):

rvolosatovs updated PR #6444.

view this post on Zulip Wasmtime GitHub notifications bot (May 25 2023 at 15:15):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (May 25 2023 at 16:00):

alexcrichton merged PR #6444.

Last updated: Oct 23 2024 at 20:03 UTC