MINA SSHD
  1. MINA SSHD
  2. SSHD-106

Wrong use of ProcessBuilder.environment() in ProcessShellFactory.java

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.5.0
    • Fix Version/s: 0.6.0
    • Labels:
      None
    • Environment:
      Android 1.6 (Dalvik)

      Description

      It seems ProcessBuilder.environment() behaves differently on android and sun's JDK. The latest javadoc says;

      The behavior of the returned map is system-dependent. A system may not allow modifications to environment variables or may forbid certain variable names or values. For this reason, attempts to modify the map may fail with UnsupportedOperationException or IllegalArgumentException if the modification is not permitted by the operating system.

      http://download.oracle.com/javase/6/docs/api/java/lang/ProcessBuilder.html#environment()

      I'm porting sshd to android platform. i got UnsupportedOperationException if I modify the return value of ProcessBuilder.environment(). Instead, it's OK to make a copy of it.

      Here's my patch:

      Index: sshd-core/src/main/java/org/apache/sshd/server/shell/ProcessShellFactory.java
      ===================================================================
      — sshd-core/src/main/java/org/apache/sshd/server/shell/ProcessShellFactory.java (revision 1069206)
      +++ sshd-core/src/main/java/org/apache/sshd/server/shell/ProcessShellFactory.java (working copy)
      @@ -24,6 +24,7 @@
      import java.io.InputStream;
      import java.io.OutputStream;
      import java.util.EnumSet;
      +import java.util.HashMap;
      import java.util.Map;

      import org.apache.sshd.common.Factory;
      @@ -94,10 +95,12 @@
      }
      }
      ProcessBuilder builder = new ProcessBuilder(cmds);
      + Map<String, String> mergedEnv = new HashMap<String, String>();
      + mergedEnv.putAll(env);
      if (env != null)

      { - builder.environment().putAll(env); + mergedEnv.putAll(builder.environment()); }
      • LOG.info("Starting shell with command: '{}' and env: {}", builder.command(), builder.environment());
        + LOG.info("Starting shell with command: '{}' and env: {}", builder.command(), mergedEnv);
        process = builder.start();
        out = new TtyFilterInputStream(process.getInputStream());
        err = new TtyFilterInputStream(process.getErrorStream());

      It would work for both 0.5.0 and trunk.

        Activity

        Hide
        Guillaume Nodet added a comment -

        I think the patch is wrong because the env map is overriden by the builder.environment(), and the mergedEnv isn't used at all.
        Maybe a better idea would be simply to catch the exception:

        ProcessBuilder builder = new ProcessBuilder(cmds); 
        if (env != null) {
           try {
              builer.environment().putAll(env);
           } catch (Exception e) {
              LOG.info("Could not set environment for command", e);
           }
        }
        LOG.info("Starting shell with command: '{}' and env: {}", builder.command(), builder.environment()); 
        
        Show
        Guillaume Nodet added a comment - I think the patch is wrong because the env map is overriden by the builder.environment(), and the mergedEnv isn't used at all. Maybe a better idea would be simply to catch the exception: ProcessBuilder builder = new ProcessBuilder(cmds); if (env != null ) { try { builer.environment().putAll(env); } catch (Exception e) { LOG.info( "Could not set environment for command" , e); } } LOG.info( "Starting shell with command: '{}' and env: {}" , builder.command(), builder.environment());
        Hide
        Chao Shi added a comment -

        Oh yes, you are right. Sorry for my careless.

        Show
        Chao Shi added a comment - Oh yes, you are right. Sorry for my careless.

          People

          • Assignee:
            Unassigned
            Reporter:
            Chao Shi
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development