Skip to content

Use execve() to replace system() #4223

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 1 commit into
base: main
Choose a base branch
from

Conversation

lum1n0us
Copy link
Collaborator

  • Direct Execution: execve() directly executes a program, bypassing the shell. This avoids vulnerabilities like shell injection, which can occur with system() if user input is not properly sanitized.
  • Controlled Environment: With execve(), you can explicitly specify the environment variables for the new process, providing better control over the execution context.
  • No Shell Overhead: execve() does not invoke a shell, reducing the risk of unintended behavior caused by shell features or configurations.
  • Predictable Behavior: execve() only executes the specified program, whereas system() relies on the shell, which may interpret commands differently based on the shell's configuration or environment.

@lum1n0us lum1n0us force-pushed the fix/reimplement_bh_system branch 2 times, most recently from 36ffe27 to 55ad4b6 Compare April 28, 2025 03:35

#if !(defined(_WIN32) || defined(_WIN32_))
ret = system(cmd);
ret = execve(pathname, argv, envp);
Copy link
Collaborator

Choose a reason for hiding this comment

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

does this work at all? i suppose you need fork+execve+wait.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

execve() executes the program referred to by pathname. This causes the program that is currently being run by the calling process to be replaced with a new program, with newly initialized stack, heap, and (initialized and uninitialized) data segments.

IMU, in our scenarios, there is no need to fork a new process and run commands. The default behavior is sufficient

Copy link
Collaborator

Choose a reason for hiding this comment

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

i don't think so. wamrc has other things to do after invoking these external commands.
have you tested?

int
bh_system(const char *cmd)
bh_system(const char *pathname, char *const argv[], int argc)
Copy link
Collaborator

Choose a reason for hiding this comment

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

i feel the name bh_system is confusing if it doesn't work like system().

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Got you points. I will rename it to bh_execve().

- Direct Execution: execve() directly executes a program, bypassing the shell.
  This avoids vulnerabilities like shell injection, which can occur with
  system() if user input is not properly sanitized.
- Controlled Environment: With execve(), you can explicitly specify the
  environment variables for the new process, providing better control over
  the execution context.
- No Shell Overhead: execve() does not invoke a shell, reducing the risk
  of unintended behavior caused by shell features or configurations.
- Predictable Behavior: execve() only executes the specified program, whereas
  system() relies on the shell, which may interpret commands differently
  based on the shell's configuration or environment.
@lum1n0us lum1n0us force-pushed the fix/reimplement_bh_system branch from 55ad4b6 to d3a2cdd Compare April 29, 2025 05:31
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.

2 participants