Problem

If a Drush backend process creates another backend process then _drush_backend_generate_command() will escape the command using escapeshellcmd().

N.B. This does seem not occur with All-versions-5.x-dev but I'm still testing - taking sometime as the backend invocation has been rewritten :) - looks fantastic.

Steps to reproduce:

  • Create a drush.ini in your drush directory (or any other valid location) with the line "memory_limit = 256M" in it
  • Create a file called fork.php in some folder with the following contents:

    <?php
    $fork = drush_get_option('forktest');
    $pid = getmypid();
    $ourFileName = "fork-$fork-pid-$pid";
    $ourFileHandle = fopen($ourFileName, 'w') or die("can't open file");
    fclose($ourFileHandle);
    $fork++;
    if ($fork < 10) {
      drush_backend_fork("scr fork.php --forktest=$fork", array());
    }
    
  • Run the command "drush scr fork.php --forktest=1"

This should create 9 files in your current folder starting with fork- however it will only create 2.

This is because the second run of fork.php (the first as a background process) the global variable DRUSH_COMMAND is parsed already with the strings escaped.

System command to invoke 1st fork: (/usr/bin/php -d memory_limit="256M" /home/developer/build/bin/drush/drush.php --php='/usr/bin/php -d memory_limit=\"256M\" ' --quiet --verbose --debug scr 'fork.php' '--forktest=2' --backend 2>&1 &) > /dev/null

System command to invoke 2nd fork: (/usr/bin/php -d memory_limit=\\"256M\\" /home/developer/build/bin/drush/drush.php --php='/usr/bin/php -d memory_limit=\\\"256M\\\" ' --quiet --verbose --debug scr 'fork.php' '--forktest=3' --backend 2>&1 &) > /dev/null

Proposed resolution

There are a couple of possible places to fix this but I'm not sure I really like any solutions I've come up with so far. The patch attached is the best I've come up with so far. Basically it's using PHP'S stripslashes() command to cleanup drush's php option if you're in a backend process.

CommentFileSizeAuthor
drush_fork.patch996 bytesalexpott
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

greg.1.anderson’s picture

drush_backend_fork has been removed in Drush-5. Please confirm whether this problem exists in master first before we decide whether to backport or support this functionality in Drush-4.

alexpott’s picture

Hi Greg,

As I wrote:

N.B. This does seem not to occur with All-versions-5.x-dev but I'm still testing - taking sometime as the backend invocation has been rewritten :) - looks fantastic.

So I don't think it does... but I haven't quite got it to exhibit the same non blocking behaviour as it does in drush4. I'm using drush_invoke_process() which is using proc_open and then waiting till the process has completed. This is different from drush4's drush_backend_fork(). That said Drush commands in drush5 seems to be built with drush_build_drush_command() and therefore this has been called from each child process and has returned a correctly escaped drush command.

greg.1.anderson’s picture

#1433556: Aegir wants the drush_backend_fork capability restored. might potentially be backported to Drush-4. If we are to continue to support drush_backend_fork with the php option, it should be done by avoiding the double-encode rather than un-encoding and re-encoding. If #1433556 is backported, it might be more reliable to make drush_backend_fork call through to drush_invoke_process with the 'fork' option set.

kostajh’s picture

@alexpott have you noticed any issues with the patch you've posted here?

alexpott’s picture

Nope no issues at all. We'v just put your drush daemon and this patch into a rather large production site and it's working great.

However, Greg is right. The approach sucks. I'm currently looking at the drush5 forking code and once I've tested that I'll post my results on #1433556: Aegir wants the drush_backend_fork capability restored. Then time permitting I'll look at backporting to drush4 and patching drush daemon to work with both drush forking processes.

moshe weitzman’s picture

Status: Active » Closed (duplicate)

Sounds like this issue is no longer needed.