Page MenuHomePhorge
Create Task
Maniphest T15281

$HOME missing from commands issued by ExecFuture
Open, NormalPublic
Actions

Description

As far as I can see, it seems to me that every command issued by the ExecFuture system, does not have the $HOME environment variable.

Documentation:

https://we.phorge.it/book/arcanist/class/ExecFuture/

Among other things, this is a problem in the Config page (/config) since it issues git log but without an $HOME it cannot parse any .gitconfig and cannot read safe.directory and so it fails.

Questions:

  1. Is $HOME missing by design in ExecFuture? I think no.
  2. Should we enable $HOME as default? I think yes.

Tested with:

  1. It does NOT work with Apache mod_php PHP 7.4 variables_order = "GPCS" (default)
  2. It does NOT work with apache mod_php PHP 7.4 variables_order = "EGPCS"
  3. ...

Interestingly, the Config page start working with this patch I crafted, to manually create an HOME environment variable in this way:

      $log_futures[$lib] = id(new ExecFuture('%C', $log_command))
+       ->setEnv(array('HOME' => posix_getpwuid(posix_getuid())['dir']))
        ->setCWD($root);

The above workaround was tested in Linux with mod_php and it's probably too much system-centric. This specific HOME-related sub-problem is probably better to be discussed here:

T15298: Get the HOME of the user running the PHP webserver

Revisions and Commits

rP Phorge
AbandonedD25149 Config page: add $HOME to allow a gitconfig and help on "dubious ownership"

Related Objects
Search...

Event Timeline

valerio.bozzolan created this task.Apr 27 2023, 07:55
valerio.bozzolan triaged this task as Normal priority.
valerio.bozzolan created this object in space S1 Public.
Herald added subscribers: Cigaryno, Matthew, chris and 2 others. · View Herald TranscriptApr 27 2023, 07:55
valerio.bozzolan mentioned this in T15282: Fix/avoid/simplify similar fatal: detected dubious ownership in repository at '/var/www/phorge'.Apr 27 2023, 08:01
valerio.bozzolan added a parent task: T15282: Fix/avoid/simplify similar fatal: detected dubious ownership in repository at '/var/www/phorge'.
valerio.bozzolan mentioned this in Z1: Phorge.Apr 29 2023, 12:12
speck added a comment.Apr 29 2023, 13:58

Could you check the server configuration of php.ini for the value of variable_orders? This open task on phab suggests that on servers it should enforce including E causing phab's environment to be passed to sub-commands, but does not want to mess with it on users' machines.

https://secure.phabricator.com/T12071

valerio.bozzolan updated the task description. (Show Details)Apr 29 2023, 20:26
valerio.bozzolan updated the task description. (Show Details)Apr 29 2023, 22:54
valerio.bozzolan edited parent tasks, added: T15299: Config page runs git commands that should have an HOME to open gitconfig correctly; removed: T15282: Fix/avoid/simplify similar fatal: detected dubious ownership in repository at '/var/www/phorge'.Apr 29 2023, 23:32
valerio.bozzolan added a revision: D25149: Config page: add $HOME to allow a gitconfig and help on "dubious ownership".Apr 29 2023, 23:44
valerio.bozzolan added a comment.Apr 30 2023, 22:49
In T15281#7096, @speck wrote:

Could you check the server configuration of php.ini for the value of variable_orders? This open task on phab suggests that on servers it should enforce including E causing phab's environment to be passed to sub-commands, but does not want to mess with it on users' machines.

https://secure.phabricator.com/T12071

Yup for some reasons it seems with or without that "E" in variables_order the $HOME is not passed to underlying Future commands

speck added a comment.May 7 2023, 19:31

In upstream phabricator I worked with Evan to update the Mercurial API in a way that all mercurial commands/futures are executed through a common path and allow making modifications to only those executions. For this we should look at something similar so that we're only passing $HOME to git commands and not to others.

https://secure.phabricator.com/D21697 is the change I'm thinking of, and might be useful as reference

valerio.bozzolan closed subtask T15298: Get the HOME of the user running the PHP webserver as Resolved.May 8 2023, 07:01
valerio.bozzolan added a comment.May 8 2023, 08:03

Thank you speck, very interesting.

I agree and I think that D25149 follows your suggestion of just exposing an $HOME environment variable, only to the specific commands needing that.

Feel free to take a quick look to tell if I'm misusing Futures

(Ouch I couldn't figure out where HOME comes from by looking at the mentioned diff in secure dot)

valerio.bozzolan moved this task from Backlog to PingDeath 🌚 on the User-valerio.bozzolan board.May 23 2023, 09:52
avivey subscribed.Jun 10 2023, 08:46

Ok, looks like my personal install is missing $HOME as well, so I can probably try to reproduce.

avivey added a comment.Jun 10 2023, 08:51

In T12071, @epriestly mentions that individually selecting env-vars to copy "build[s] some resistance to "Shellshock" class vulnerabilities", which is kind of a compelling argument.

valerio.bozzolan moved this task from PingDeath 🌚 to 🔥 Trap on the User-valerio.bozzolan board.Jun 12 2023, 13:06
valerio.bozzolan mentioned this in Q65: Bug: Unhandled Exception ("Exception") - Diff Parse Exception: Expected '\ No newline at end of file'. (Answer 84).Jun 30 2023, 15:22
avivey mentioned this in T15533: Bug: Unhandled Exception ("Exception") - Diff Parse Exception: Expected '\ No newline at end of file'..Jul 11 2023, 17:25
avivey added a comment.Jul 11 2023, 18:42

See https://we.phorge.it/T15533#11839 - the env-selecting code in ExecFuture has this logic:

E \ custom envyesno
have Ecustom + copy envCopy Env
no Ecustom + 2 copy important envvarsEmpty env
  • "custom env" means "setEnv() was called".
  • There's also a way to specify "custom env" without copying any env vars, but I hope it's not used.

All executions in the form of exec*() count as "no custom env".

I think the "empty env" scenario is not really good, and that there are a few more important envvars (LC_*, HOME) we can copy in the non-E case (Or just require E, but see previous comment about that).

I wasn't able to track down the logic behind this - I guess E used to always be enabled, and then it doesn't really matter?

Content licensed under Creative Commons Attribution-ShareAlike 4.0 (CC-BY-SA) unless otherwise noted; code licensed under Apache 2.0 or other open source licenses. · CC BY-SA 4.0 · Apache 2.0