Skip to content

Fix env::ArgsOs for zkVM #139849

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

thaliaarchi
Copy link
Contributor

@thaliaarchi thaliaarchi commented Apr 15, 2025

The zkVM implementation of env::ArgsOs incorrectly reports the full length even after having iterated. Instead, use a range approach which works out to be simpler. Also, implement more iterator methods like the other platforms in #139847.

cc @flaub @jbruestle @SchmErik

@rustbot
Copy link
Collaborator

rustbot commented Apr 15, 2025

r? @ibraheemdev

rustbot has assigned @ibraheemdev.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Apr 15, 2025
@thaliaarchi
Copy link
Contributor Author

thaliaarchi commented Apr 15, 2025

This PR fixes the iterator, but leaves the arg retrieval and copying the same. There are some interesting alternatives to be considered there.

fn argv(i: usize) -> OsString {
let arg_len = unsafe { abi::sys_argv(crate::ptr::null_mut(), 0, i) };
let arg_len_words = (arg_len + WORD_SIZE - 1) / WORD_SIZE;
let words = unsafe { abi::sys_alloc_words(arg_len_words) };
let arg_len2 = unsafe { abi::sys_argv(words, arg_len_words, i) };
debug_assert_eq!(arg_len, arg_len2);
// Convert to OsString.
//
// FIXME: We can probably get rid of the extra copy here if we
// reimplement "os_str" instead of just using the generic unix
// "os_str".
let arg_bytes: &[u8] =
unsafe { crate::slice::from_raw_parts(words.cast() as *const u8, arg_len) };
OsString::from_inner(os_str::Buf { inner: arg_bytes.to_vec() })
}

It looks like the memory allocated by sys_alloc_words is never freed. If that's intentional and it's 'static, then we could retrieve all the args upon construction of the first Args and store them in a OnceLock<Vec<&'static OsStr>>, then copy them to OsString on demand. However, sys_argv documents that the host controls what is returned. Is caching then inappropriate?

As for the comment about reimplementing OsString, I don't think that's a good idea. It uses one of two abstractions over Vec<u8> for the supported encodings. Although the encoding is platform-dependent, the abstractions are platform-independent. I don't like the idea of adding platform-specific logic there. It seems that the desire is to manage allocations differently with OsString. Can that currently be done with stock Vec<u8>? I'd think such logic should go there, then OsString would work.

The zkVM implementation of `env::ArgsOs` incorrectly reports the full
length even after having iterated. Instead, use a range approach which
works out to be simpler. Also, implement more iterator methods like the
other platforms in rust-lang#139847.
Retrieve argc/argv from the host once, on demand when the first
`env::ArgsOs` is constructed, and globally cache it. Copy each argument
to an `OsString` while iterating.
@thaliaarchi
Copy link
Contributor Author

I've now implemented the caching approach. The two commits are complete fixes with different approaches, so they should be considered separately until we decide.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants