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

calls external commands #64

Closed
wants to merge 1 commit into from
Closed

calls external commands #64

wants to merge 1 commit into from

Conversation

wbehrens-on-gh
Copy link

@wbehrens-on-gh wbehrens-on-gh commented Mar 6, 2021

Added an exec command to the Process class that basically wraps C's system() function so now Wren-cli can call external programs.

Example:

import  "os" for Process

Process.exec("echo hello") // Should print hello to the console
Process.exec("not a command") // Errors out saying command not found
Process.exec(12) // Aborts current fiber saying that exec requires a string

Don't think this is a perfect implementation but it's minimal.

@ChayimFriedman2
Copy link

I think the C system() API is not good. It does not allow passing arguments separately, for example, which can lead to attacks.

As the CLI is based on libuv, I think we should provide an API based on libuv processes.

@wbehrens-on-gh
Copy link
Author

I'm not familiar with working with libuv but I'll see if I can throw something up.

@PureFox48
Copy link

PureFox48 commented Apr 11, 2021

Even if it's only done as a temporary expedient, I think it would make sense to merge this relatively simple PR into the forthcoming v0.4.0 release as it would make a significant difference to what can be done with Wren-cli.

An API based on libuv processes can be added later.

@ChayimFriedman2
Copy link

ChayimFriedman2 commented Apr 12, 2021

If so, then at least have arguments passed separately using a list.

@wbehrens-on-gh
Copy link
Author

Could you explain why there is a need to pass arguments as a list? most users will just pass a single string with the arguments in it anyway.

@ChayimFriedman2
Copy link

Security.

@wbehrens-on-gh
Copy link
Author

Could you explain how it would help security?

@ChayimFriedman2
Copy link

If I do,

Process.exec("cat \"%(fileName)\"")

When fileName is user-provided, the user can inject for example \"; some_malicious_code \" executing arbitrary code. This can be solved by santizing the input, but sanitizing input is hard. For this reason, it's prefered to pass arguments as a separate list, where Process.exec("cat", [fileName]) can never execute arbitrary code.

@joshgoebel
Copy link
Contributor

uv_spawn looks pretty dead simple though at first glance (compared to networking). http://docs.libuv.org/en/v1.x/guide/processes.html#spawning-child-processes

Of course this quickly gets into:

  • pipes, input/output redirection
  • process control / signaling
  • process monitoring

But I think for starters just a run it and let you know the error code would be trivial. And if UV gets us cross-platform for free, even better. My first ideas for packages require being able to call git, so I'd need this too.

@joshgoebel
Copy link
Contributor

See #94 for a quick pass at this based on UV.

@joshgoebel
Copy link
Contributor

I believe this will be closed now that we are moving forward with #94.

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.

None yet

4 participants