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

Fix passing environment variables #25

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

orivej
Copy link

@orivej orivej commented Jun 4, 2015

I was astonished to find out that external-program does not support passing environment variables under SBCL as provided by the user, adding single quotes around them instead.

This is an attempt to improve handling of environment variables.

  1. Adding quotes around values is wrong. All the system calls take an array of unquoted arguments. And quoting, when necessary, is never as simple as adding single quote marks around the value.
  2. Adding PATH= to env -i is wrong; empty environment is not the one with PATH defined to be an empty string. Yes, this allows some programs to run without full path, because as man 3 exec says, for the purpose of exec undefined PATH defaults to confstr(_CS_PATH) (in C) or getconf CS_PATH (in shell), and on my system, for example, that evaluates to /bin:/usr/bin.
  3. should-have-access-to-shell-builtins — why? Running programs under shell is difficult to get right specifically because of quoting, and provides no benefits over running them directly (unless the user wants shell features, but external-program does not really facilitate that).
  4. empty-env-should-erase-all — yes, but ls may succeed even with empty environment.

I tested the change under SBCL, CCL, CLISP and ECL.

@orivej
Copy link
Author

orivej commented Jun 4, 2015

ABCL fails the last test because it does not implement proper quoting of the arguments that it passes to shell (but it is not external-program's job to do that when its interface requires no escaping on the callers part).

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.

1 participant