Stream: git-wasmtime

Topic: wasmtime / PR #2528 Add safe Memory::read and Memory::wri...

view this post on Zulip Wasmtime GitHub notifications bot (Dec 20 2020 at 00:49):

theduke opened PR #2528 from issue-2484-safe-memory-access to main:

Introduce Memory::read and Memory::write methods for accessing memory contents safely.

Also adds a test and mentions the the new methods in the Memory docs.

Fixes #2484.

Currently most fallible methods use anyhow::Error.
I have instead added a new MemoryError, since recovering from such errors is possible and it's more expressive.
(future proofed with #[non_exhaustive]).

Could to change it to a anyhow::Error if that's the preferred strategy (or wrap the MemoryError with anyhow).

I also already added a MemoryError::Locked variant, which is not required yet, but apparently will be once threading support lands. Happy to remove it or slap on a #[doc(hidden)] .

view this post on Zulip Wasmtime GitHub notifications bot (Dec 20 2020 at 00:50):

theduke edited PR #2528 from issue-2484-safe-memory-access to main:

Introduce Memory::read and Memory::write methods for accessing memory contents safely.

Also adds a test and mentions the the new methods in the Memory docs.

Fixes #2484.

Currently most fallible methods use anyhow::Error.
I have instead added a new MemoryError, since recovering from such errors is possible and it's more expressive.
(future proofed with #[non_exhaustive]).

Could to change it to a anyhow::Error if that's the preferred strategy (or wrap the MemoryError with anyhow).

I also already added a MemoryError::Locked variant, which is not required yet, but apparently will be once threading support lands. Happy to remove it or slap on a #[doc(hidden)] to prevent confusion.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 20 2020 at 01:11):

theduke updated PR #2528 from issue-2484-safe-memory-access to main:

Introduce Memory::read and Memory::write methods for accessing memory contents safely.

Also adds a test and mentions the the new methods in the Memory docs.

Fixes #2484.

Currently most fallible methods use anyhow::Error.
I have instead added a new MemoryError, since recovering from such errors is possible and it's more expressive.
(future proofed with #[non_exhaustive]).

Could to change it to a anyhow::Error if that's the preferred strategy (or wrap the MemoryError with anyhow).

I also already added a MemoryError::Locked variant, which is not required yet, but apparently will be once threading support lands. Happy to remove it or slap on a #[doc(hidden)] to prevent confusion.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 04 2021 at 16:48):

alexcrichton submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 04 2021 at 16:48):

alexcrichton created PR Review Comment:

Could this documentation detail the error cases as well? It also might be good to mention that the size of the copy is the size of the buffer itself.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 04 2021 at 16:48):

alexcrichton submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 04 2021 at 16:48):

alexcrichton created PR Review Comment:

FWIW I don't think that we'll ever need this error because we'll basically be implementing memory.copy semantics which would never need an error for this.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 04 2021 at 16:48):

alexcrichton created PR Review Comment:

I think here this will want to use checked_add to avoid overflow.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 04 2021 at 19:19):

theduke submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 04 2021 at 19:19):

theduke created PR Review Comment:

In that case ... should the error just be a struct?

I don't see any other relevant error conditions.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 04 2021 at 19:22):

alexcrichton submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 04 2021 at 19:22):

alexcrichton created PR Review Comment:

That's true yeah, and I think we could go ahead and have it just be an opaque structure with a singular private field so we can add more to it later if we want.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 04 2021 at 19:46):

theduke updated PR #2528 from issue-2484-safe-memory-access to main:

Introduce Memory::read and Memory::write methods for accessing memory contents safely.

Also adds a test and mentions the the new methods in the Memory docs.

Fixes #2484.

Currently most fallible methods use anyhow::Error.
I have instead added a new MemoryError, since recovering from such errors is possible and it's more expressive.
(future proofed with #[non_exhaustive]).

Could to change it to a anyhow::Error if that's the preferred strategy (or wrap the MemoryError with anyhow).

I also already added a MemoryError::Locked variant, which is not required yet, but apparently will be once threading support lands. Happy to remove it or slap on a #[doc(hidden)] to prevent confusion.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 04 2021 at 19:47):

theduke submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 04 2021 at 19:47):

theduke created PR Review Comment:

Done.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 04 2021 at 19:47):

theduke created PR Review Comment:

Ah yeah, I'm too used to debug semantics for overflow checking...

Both read and write now use checked_add.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 04 2021 at 19:47):

theduke submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 04 2021 at 19:48):

theduke submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 04 2021 at 19:48):

theduke created PR Review Comment:

I expanded the docs for both read and write a bit.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 04 2021 at 19:51):

theduke updated PR #2528 from issue-2484-safe-memory-access to main:

Introduce Memory::read and Memory::write methods for accessing memory contents safely.

Also adds a test and mentions the the new methods in the Memory docs.

Fixes #2484.

Currently most fallible methods use anyhow::Error.
I have instead added a new MemoryError, since recovering from such errors is possible and it's more expressive.
(future proofed with #[non_exhaustive]).

Could to change it to a anyhow::Error if that's the preferred strategy (or wrap the MemoryError with anyhow).

I also already added a MemoryError::Locked variant, which is not required yet, but apparently will be once threading support lands. Happy to remove it or slap on a #[doc(hidden)] to prevent confusion.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 04 2021 at 20:04):

theduke updated PR #2528 from issue-2484-safe-memory-access to main:

Introduce Memory::read and Memory::write methods for accessing memory contents safely.

Also adds a test and mentions the the new methods in the Memory docs.

Fixes #2484.

Currently most fallible methods use anyhow::Error.
I have instead added a new MemoryError, since recovering from such errors is possible and it's more expressive.
(future proofed with #[non_exhaustive]).

Could to change it to a anyhow::Error if that's the preferred strategy (or wrap the MemoryError with anyhow).

I also already added a MemoryError::Locked variant, which is not required yet, but apparently will be once threading support lands. Happy to remove it or slap on a #[doc(hidden)] to prevent confusion.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 04 2021 at 20:24):

theduke updated PR #2528 from issue-2484-safe-memory-access to main:

Introduce Memory::read and Memory::write methods for accessing memory contents safely.

Also adds a test and mentions the the new methods in the Memory docs.

Fixes #2484.

Currently most fallible methods use anyhow::Error.
I have instead added a new MemoryError, since recovering from such errors is possible and it's more expressive.
(future proofed with #[non_exhaustive]).

Could to change it to a anyhow::Error if that's the preferred strategy (or wrap the MemoryError with anyhow).

I also already added a MemoryError::Locked variant, which is not required yet, but apparently will be once threading support lands. Happy to remove it or slap on a #[doc(hidden)] to prevent confusion.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 04 2021 at 20:30):

alexcrichton submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 04 2021 at 20:30):

alexcrichton created PR Review Comment:

I think it should be ok to drop the PartialEq here

view this post on Zulip Wasmtime GitHub notifications bot (Jan 04 2021 at 20:30):

alexcrichton submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 04 2021 at 20:30):

alexcrichton created PR Review Comment:

It's ok to remove this comment, I don't think it adds much over what the existence of the checked_add already implies.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 04 2021 at 20:30):

alexcrichton created PR Review Comment:

Although thinking about this again, another perhaps safer way to write these methods would be:

let src = self.data_unchecked().get(offset..offset + buffer.len())?;
buffer.copy_from_slice(src);
Ok(())

(and something similar below)

view this post on Zulip Wasmtime GitHub notifications bot (Jan 04 2021 at 20:31):

alexcrichton submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 04 2021 at 20:31):

alexcrichton created PR Review Comment:

Or actually to avoid having to do checked arithmetic:

let src = self.data_unchecked().get(offset..).get(..buffer.len())?;

view this post on Zulip Wasmtime GitHub notifications bot (Jan 26 2021 at 04:04):

theduke updated PR #2528 from issue-2484-safe-memory-access to main:

Introduce Memory::read and Memory::write methods for accessing memory contents safely.

Also adds a test and mentions the the new methods in the Memory docs.

Fixes #2484.

Currently most fallible methods use anyhow::Error.
I have instead added a new MemoryError, since recovering from such errors is possible and it's more expressive.
(future proofed with #[non_exhaustive]).

Could to change it to a anyhow::Error if that's the preferred strategy (or wrap the MemoryError with anyhow).

I also already added a MemoryError::Locked variant, which is not required yet, but apparently will be once threading support lands. Happy to remove it or slap on a #[doc(hidden)] to prevent confusion.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 26 2021 at 04:07):

theduke updated PR #2528 from issue-2484-safe-memory-access to main:

Introduce Memory::read and Memory::write methods for accessing memory contents safely.

Also adds a test and mentions the the new methods in the Memory docs.

Fixes #2484.

Currently most fallible methods use anyhow::Error.
I have instead added a new MemoryError, since recovering from such errors is possible and it's more expressive.
(future proofed with #[non_exhaustive]).

Could to change it to a anyhow::Error if that's the preferred strategy (or wrap the MemoryError with anyhow).

I also already added a MemoryError::Locked variant, which is not required yet, but apparently will be once threading support lands. Happy to remove it or slap on a #[doc(hidden)] to prevent confusion.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 26 2021 at 15:09):

alexcrichton merged PR #2528.

Last updated: Nov 22 2024 at 16:03 UTC