Add read_cstring/write_cstring memory helpers#611
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds Ruby-facing helpers to read/write NUL-terminated C strings from Wasm linear memory, along with unit specs to validate behavior.
Changes:
- Added
Memory#read_cstringto read bytes until NUL (or end) and return a UTF-8 RubyString. - Added
Memory#write_cstringto write string bytes plus a trailing NUL, with bounds checking. - Added RSpec coverage for common/edge cases (leading NUL, out-of-bounds reads/writes).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| spec/unit/memory_spec.rb | Adds RSpec coverage for read_cstring/write_cstring behaviors and edge cases. |
| ext/src/ruby_api/memory.rs | Implements and wires up read_cstring/write_cstring methods on Memory. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Hi @omohokcoj since I've seen you request copilot reviews on PRs a few times now: please note that the BA has an AI tool use policy that asks that contributors don't use automated AI tools in this way. If you want to use copilot reviews, one approach is to do so in your own repo before opening a PR upstream. Thanks! |
|
@tschneidereit thanks for the heads up - I didn't request Copilot code review manually, but it looks like GitHub does this automatically for all pull requests without a notice/prompt. I'll have this feature disabled. 
|
We had other PRs in the last two weeks in which no code review was automatically triggered e.g., #601. I am not entirely sure what heuristic is used to trigger the review though. @tschneidereit At any rate, I noticed that the Copilot Code Review setting was enabled by default in the repository. I have now disabled it. Hopefully that will reduce the noise. |
|
I just disabled it globally for the BA. It took a support ticket and waiting a few weeks to learn how to even do that: turns out you have to enable copilot in an org to have access to the settings page that allows you to disable use of copilot in that same org. You've probably been able to see that setting in the repo now for the same reason ... |
| let len = slice.len(); | ||
| let mut context = self.store.context_mut()?; | ||
| let dst = self | ||
| .get_wasmtime_memory() | ||
| .data_mut(&mut context) | ||
| .get_mut(offset..) | ||
| .and_then(|s| s.get_mut(..len + 1)) |
There was a problem hiding this comment.
Shouldn't this validate against arbitrary null bytes in the incoming Ruby string?
There was a problem hiding this comment.
@saulecabrera makes sense, validation has been added in 48f0470
Following the memory-access helpers added recently in #601 could we please also have read_cstring/write_cstring - reading nul-terminated C strings is a hot path when working with C libraries, and doing the nul byte scan in Ruby is very inefficient.