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

Implement directory mapping and WASI file functions #208

Merged
merged 1 commit into from
Feb 16, 2024

Conversation

kulcsaradam
Copy link
Contributor

@kulcsaradam kulcsaradam commented Dec 1, 2023

Refactor the importing of WASI functions to be clearer.
Implement path_open, fd_seek, fd_read, environ_sizes_get, environ_get for file acces.
Implement the mapping of real directories to virtual ones so that WASI can use different directories.
Add flag '--mapdirs' 'real' 'virtual' for mapping directories.
Add flag '--env' for sharing host envrionment variables.
Add flag '--help' for printing available walrus options.
Also improve random_get test to not have a result.

@kulcsaradam kulcsaradam force-pushed the wasi_emscripten_more branch 13 times, most recently from ec68b20 to 2c9e383 Compare December 6, 2023 12:00
src/shell/Shell.cpp Outdated Show resolved Hide resolved
@clover2123
Copy link
Collaborator

@kulcsaradam
Would you please elaborate more about the following?

Implement the mapping of real directories to virtual ones so that WASI can use different directories.

What is the main feature of mapping directories and where is this used?

@kulcsaradam
Copy link
Contributor Author

kulcsaradam commented Dec 8, 2023

@clover2123
Of course, my sources:
https://github.com/WebAssembly/wasi-filesystem/blob/main/path-resolution.md
https://github.com/bytecodealliance/wasmtime/blob/main/docs/WASI-overview.md - Section: Capability-Oriented

WASI by design has an idea, where for security reasons runtimes may not allow programs to freely move between host directories. By default they only allow files to manipulate in their own directories and in directories which they have handles to. Also, to obfuscate the system they are on, inside they use virtual directories like '/var' or '/home' which are actualy directories like 'home/user/walrus/test' for example.

Use cases are any file manipulation operations like reading from a file or writing to one.

I hope this is an acceptable answer!

@kulcsaradam kulcsaradam force-pushed the wasi_emscripten_more branch 5 times, most recently from bcd1681 to be1d9fb Compare December 15, 2023 15:14
@kulcsaradam kulcsaradam force-pushed the wasi_emscripten_more branch 3 times, most recently from e556f25 to 87359e0 Compare January 10, 2024 11:31
@kulcsaradam kulcsaradam force-pushed the wasi_emscripten_more branch 4 times, most recently from ea81e17 to 478a393 Compare January 19, 2024 12:41
Copy link
Collaborator

@zherczeg zherczeg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kulcsaradam kulcsaradam force-pushed the wasi_emscripten_more branch 2 times, most recently from 0ed0b74 to 88b6c6b Compare January 23, 2024 10:46
@kulcsaradam kulcsaradam reopened this Jan 25, 2024
@kulcsaradam
Copy link
Contributor Author

The pr was closed by a mistake I caused while tinkering with Visual Studio. Reopened it now.

@kulcsaradam kulcsaradam force-pushed the wasi_emscripten_more branch 2 times, most recently from b5c8f11 to 8e9d83b Compare January 26, 2024 11:24
@kulcsaradam kulcsaradam force-pushed the wasi_emscripten_more branch 2 times, most recently from 08ed3ce to 9c89d72 Compare February 13, 2024 09:07
Comment on lines +113 to +118
std::vector<std::string> variables = {
// Common Unix environment variables.
"PATH",
"LIB",
"PWD",
"SHELL",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder about how did you define these common env variables
Is there any reference?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines +1032 to +1038
preopen.real_path = reinterpret_cast<char*>(calloc(1, it->length() + 1));
it->nonstd::sv_lite::string_view::copy(const_cast<char*>(preopen.real_path), it->length());

it++;

preopen.mapped_path = reinterpret_cast<char*>(calloc(1, it->length() + 1));
it->nonstd::sv_lite::string_view::copy(const_cast<char*>(preopen.mapped_path), it->length());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will these strings allocated by calloc be correctly freed at the end of Walrus?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I forgot about those. After running valgrind I fixed these and also added initialization to wasi envrionment variables, so valgrind did not return any errors.

@kulcsaradam kulcsaradam force-pushed the wasi_emscripten_more branch 3 times, most recently from c059163 to fac5807 Compare February 14, 2024 11:33
"SystemDrive",
};

char** envp = (char**)malloc(sizeof(char**) * variables.size());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about simply allocating a vector of std::string here?
This kind of manual memory alloc is so complicated to handle and hard for correct freeing

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After spending some time thinking how I would change it to be easily used with uvwasi, I could not think of a way. If you have an idea, it would be greatly appriciated.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. I'll revise this later

Comment on lines 167 to 173
if (init_options.preopenc > 0) {
init_options.preopens = reinterpret_cast<uvwasi_preopen_t*>(calloc(init_options.preopenc, sizeof(uvwasi_preopen_s)));
}
for (unsigned i = 0; i < preopenDirs.size(); i++) {
init_options.preopens[i].mapped_path = preopenDirs[i].mapped_path;
init_options.preopens[i].real_path = preopenDirs[i].real_path;
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this case, preopenDirs already contains all the necessary information about directory mapping.
So, it seems that simply assigning the preopenDirs into init_options.preopens is possible like below

init_options.preopens = preopenDirs.data();

What do you think about this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right and it is also easier to understand so I changed it to be this way.

Refactor the importing of WASI functions to be clearer.
Implement path_open, fd_seek, fd_read, environ_sizes_get, environ_get for file acces.
Implement the mapping of real directories to virtual ones so that WASI can use different directories.
Add flag '--mapdirs' 'real' 'virtual' for mapping directories.
Add flag '--env' for sharing host envrionment variables.
Add flag '--help' for printing available walrus options.
Also improve random_get test to not have a result.

Signed-off-by: Adam Laszlo Kulcsar <kuladam@inf.u-szeged.hu>
Copy link
Collaborator

@clover2123 clover2123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@clover2123 clover2123 merged commit caf34e2 into Samsung:main Feb 16, 2024
12 checks passed
@kulcsaradam kulcsaradam deleted the wasi_emscripten_more branch February 21, 2024 10:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants