Skip to content

Split wrapper string into separate command and arguments #3900

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.

Sign up for GitHub

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 9 commits into
base: main
Choose a base branch
from
+52 −4
Open

Split wrapper string into separate command and arguments #3900

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
9e9318a
split wrapper into separate args
webbgamers Jul 5, 2025
c7b3c2f
use shlex to split hooks
webbgamers Jul 5, 2025
c6841b8
add wrapper type flag + migration
webbgamers Jul 6, 2025
a786267
Revert "add wrapper type flag + migration"
webbgamers Jul 12, 2025
1a43add
handle wrapper migration
webbgamers Jul 12, 2025
4e10adf
Merge branch 'main' into wrapper-args
webbgamers Jul 12, 2025
32a48f4
Update packages/app-lib/src/launcher/mod.rs
webbgamers Jul 14, 2025
d0dcc4c
migrate pre-launch and post-exit
webbgamers Jul 14, 2025
e4fc4d5
Merge branch 'main' into wrapper-args
webbgamers Jul 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,7 @@ serde-xml-rs = "0.8.1" # Also an XML (de)serializer, consider dropping yaserde
sha1 = "0.10.6"
sha1_smol = { version = "1.0.1", features = ["std"] }
sha2 = "0.10.9"
shlex = "1.3.0"
spdx = "0.10.8"
sqlx = { version = "0.8.6", default-features = false }
sysinfo = { version = "0.35.2", default-features = false }
Expand Down
1 change: 1 addition & 0 deletions packages/app-lib/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ hashlink.workspace = true
png.workspace = true
bytemuck.workspace = true
rgb.workspace = true
shlex.workspace = true

chrono = { workspace = true, features = ["serde"] }
daedalus.workspace = true
Expand Down
6 changes: 5 additions & 1 deletion packages/app-lib/src/api/profile/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -664,7 +664,11 @@ async fn run_credentials(
.filter(|hook_command| !hook_command.is_empty());
if let Some(hook) = pre_launch_hooks {
// TODO: hook parameters
let mut cmd = hook.split(' ');
let mut cmd = shlex::split(hook)
.ok_or(crate::ErrorKind::LauncherError(
"Unable to parse pre-launch hook".to_string(),
))?
.into_iter();
if let Some(command) = cmd.next() {
let full_path = get_full_path(&profile.path).await?;
let result = Command::new(command)
Expand Down
14 changes: 13 additions & 1 deletion packages/app-lib/src/launcher/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -563,7 +563,19 @@ pub async fn launch_minecraft(
let args = version_info.arguments.clone().unwrap_or_default();
let mut command = match wrapper {
Some(hook) => {
let mut command = Command::new(hook);
let mut hook = shlex::split(hook)
.ok_or(crate::ErrorKind::LauncherError(
"Unable to parse wrapper hook".to_string(),
))?
.into_iter();
let cmd = hook.next().ok_or(crate::ErrorKind::LauncherError(
"Empty wrapper hook".to_string(),
))?;

let mut command = Command::new(cmd);
hook.for_each(|arg| {
command.arg(arg);
});
command.arg(&java_version.path);
command
}
Expand Down
6 changes: 5 additions & 1 deletion packages/app-lib/src/state/process.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -711,7 +711,11 @@ impl Process {
// We do not wait on the post exist command to finish running! We let it spawn + run on its own.
// This behaviour may be changed in the future
if let Some(hook) = post_exit_command {
let mut cmd = hook.split(' ');
let mut cmd = shlex::split(&hook)
.ok_or(crate::ErrorKind::LauncherError(
"Unable to parse post-exit hook".to_string(),
))?
.into_iter();
if let Some(command) = cmd.next() {
let mut command = Command::new(command);
command.args(cmd).current_dir(
Expand Down
19 changes: 18 additions & 1 deletion packages/app-lib/src/state/profiles.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,17 +103,19 @@ impl ProfileInstallStage {
pub enum LauncherFeatureVersion {
None,
MigratedServerLastPlayTime,
MigratedWrapperHook,
}

impl LauncherFeatureVersion {
pub const MOST_RECENT: Self = Self::MigratedServerLastPlayTime;
pub const MOST_RECENT: Self = Self::MigratedWrapperHook;

pub fn as_str(&self) -> &'static str {
match *self {
Self::None => "none",
Self::MigratedServerLastPlayTime => {
"migrated_server_last_play_time"
}
Self::MigratedWrapperHook => "migrated_wrapper_hook",
}
}

Expand All @@ -123,6 +125,7 @@ impl LauncherFeatureVersion {
"migrated_server_last_play_time" => {
Self::MigratedServerLastPlayTime
}
"migrated_wrapper_hook" => Self::MigratedWrapperHook,
_ => Self::None,
}
}
Expand Down Expand Up @@ -782,6 +785,20 @@ impl Profile {
self.launcher_feature_version =
LauncherFeatureVersion::MigratedServerLastPlayTime;
}
LauncherFeatureVersion::MigratedServerLastPlayTime => {
if let Some(wrapper) = self.hooks.wrapper.as_ref() {
self.hooks.wrapper = Some(
shlex::Quoter::new()
.allow_nul(true)
.quote(wrapper)
.unwrap()
.to_string(),
)
}

self.launcher_feature_version =
LauncherFeatureVersion::MigratedWrapperHook;
Copy link
Member

@AlexTMjugador AlexTMjugador Jul 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Several notes here:

  • A migration for the pre_launch and post_exit hooks seems to be missing here. Such migrations must be added. (To avoid repetitive code, you could try iterating over an array of mutable references to the different command strings.)
  • As noted in the shlex documentation, NUL characters have no place in the middle of proper command strings. They will cause the command to fail to launch, or even worse, introduce pretty odd behavior or security vulnerabilities. That said, treating their presence as a fatal migration error could render the application unusable (which is a pretty heavy-handed approach when only one of tens of instances may have odd data), while ignoring them (as the app currently seems to do) will break command hook functionality for the affected profiles. I propose stripping out NUL bytes from the string before quoting it with shlex, and logging a warning if such a replacement occurs. This way, in the unlikely event that NUL bytes are present (perhaps due to database corruption or similar anomalies), we provide the best possible automated recovery in most cases that cleans up the wrong data, while keeping users and support people informed about what correction we applied.

Copy link
Author

@webbgamers webbgamers Jul 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

  1. pre_launch and post_exit both previously used the "naive" whitespace splitting method so switching to shlex splitting won't break existing configs.

  2. I don't think ignoring NUL chars will break any existing wrapper hooks since whatever was there was just being run that way before. All we are really doing is running shlex::quote so they can be later parsed by shlex::split which doesn't care about NUL chars as far as I can tell. I figure if there wasn't any validation before, no need to add it now as part of this PR. If anything I would suggest making a new issue for it to allow for further discussion on what kind of validation should be done and adding the same validation to pre_launch and post_exit. For a thorough validation, there are other problematic characters that would need to be addressed properly.

Copy link
Member

Choose a reason for hiding this comment

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

Actually, there's a slight chance that switching to shlex could break existing pre-launch and post-exit hooks if they aren't properly migrated, despite them already having a naive argument splitting logic. To illustrate, let's consider a hook string like /home/user/"stuff"/program arg. Previously, the app would launch /home/user/"stuff"/program, passing arg to it. But with shlex, it would interpret the program to run as /home/user/stuff/program, which would point to a completely different directory lacking quotation characters. So I think this migration should be handled; while it might only affect, say, 0.01% of users, the Modrinth App is deployed at such a scale that even such a tiny percentage still translates to a non-zero and significant number of people.

As for the issue with NUL characters, to rephrase what I said before: if they were present in the past, they shouldn't have been, since they wouldn't have worked anyway. While shlex::quote, shlex::split, and most Rust string functions handle NULs fine as you mentioned, the operating system and other external programs typically don't: they treat NUL as the end of a string, as is standard in the C world.

That's why I proposed doing a one-time NUL cleanup during this migration: it's a good opportunity to fix some odd behavior that may have existed previously. And indeed, this isn't a proper validation step: we're not enforcing that new shlex-quoted wrapper commands are free of interior NULs, which leaves room for advanced users to deliberately use NUL bytes if they really need to.

Eventually, though, I believe we should move toward either validating the complete absence of NULs or stripping them out before use, depending on how this migration plays out. Having clean, migrated data now will give us a solid foundation for whatever we decide later: garbage in, garbage out, after all.

Copy link
Author

@webbgamers webbgamers Jul 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the example, I would argue that anyone with quotes in their filenames deserves to have their config broken... jk
Anyways, I will need to handle migrations differently for pre-launch and post-exit hooks since they were previously parsed differently, but should be straight forwards.

I have also realized that this system will not migrate the global setting hooks so I need to come up with a way to do that. Probably will involve creating a migration system similar to the profile launcher_feature_version system... 🫠 Any suggestions for this would be appreciated.

On the NUL issue, I'm not sure if just stripping them from hook strings is necessarily the best approach. If there is a NUL in the string, as you mentioned it is almost certainly the result of some kind of database corruption and in that case you can't make any assumptions about the integrity of the rest of the wrapper string or really the entire database. SQLite is very robust to corruption and my guess is that you would get SQL errors before getting invalid data from the database. If there is a need to be resilient from invalid data in the database I don't think it is best addressed by this PR as the original goal was just bringing wrapper parsing to parity with pre-launch and post-exit parsing, and that has already expanded in scope. I would really prefer to just assume whatever was in old string is valid and handle the migration without trying to perform any kind of validation.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I see your point about not bothering with the NUL fixups. You're right that once the database is corrupted, trying to salvage that particular piece of data is probably a lost cause. Let's not do it in this PR; we can always revisit that later if needed.

Regarding the global hooks, I agree that something along the lines of the migration approach I've previously suggested may be necessary. It sounds like the cleanest approach would be to migrate both global and instance-specific hooks using such a migration. You could add a new function to the migrate_legacy_data module, similar to the already existing migrate_legacy_data, and call it from within State::initialize_state.

}
LauncherFeatureVersion::MOST_RECENT => unreachable!(
"LauncherFeatureVersion::MOST_RECENT was not updated"
),
Expand Down