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

execAndReturn in OS.java fails to collect command output. #1809

Closed
sunshouxiang opened this Issue Jun 1, 2018 · 4 comments

Comments

Projects
None yet
2 participants
@sunshouxiang
Copy link

sunshouxiang commented Jun 1, 2018

Information

  1. Apktool Version (2.3.3) -
  2. Operating System (Linux) -
  3. APK From? (Other) -

Stacktrace/Logcat

Include stacktrace here

Steps to Reproduce

  1. apktool

Cause

Although you used process.waitFor() to wait for termination of child process, you did not wait for the termination of collector thread. This results in collector.get returns incorrect text as the output of child process might not be collected completely at the point. executor.shutdown and executor.awaitTermination should be used to fix the problem.

@iBotPeaches

This comment has been minimized.

Copy link
Owner

iBotPeaches commented Jun 6, 2018

Thanks for the report/tip. Would you like to submit a PR to correct this behavior?

@sunshouxiang

This comment has been minimized.

Copy link
Author

sunshouxiang commented Jun 11, 2018

It seems I do not have permission to create a branch for PR, so please refer to the patch output below.

***************
*** 14,35 ****
--- 14,36 ----
  package brut.util;
  
  import brut.common.BrutException;
  import java.io.*;
  import java.util.Arrays;
  import java.util.concurrent.ExecutorService;
  import java.util.concurrent.Executors;
  import java.util.logging.Logger;
+ import java.util.concurrent.TimeUnit;
  
  import org.apache.commons.io.IOUtils;
  
  /**
   * @author Ryszard Wiśniewski <brut.alll@gmail.com>
   */
  public class OS {
  
      private static final Logger LOGGER = Logger.getLogger("");
  
      public static void rmdir(File dir) throws BrutException {
***************
*** 105,126 ****
--- 106,132 ----
      public static String execAndReturn(String[] cmd) {
          ExecutorService executor = Executors.newCachedThreadPool();
          try {
              ProcessBuilder builder = new ProcessBuilder(cmd);
              builder.redirectErrorStream(true);
  
              Process process = builder.start();
              StreamCollector collector = new StreamCollector(process.getInputStream());
              executor.execute(collector);
  
              process.waitFor();
+             if (!executor.awaitTermination(15, TimeUnit.SECONDS)) {
+                 executor.shutdownNow();
+                 if (!executor.awaitTermination(5, TimeUnit.SECONDS))
+                     System.err.println("Stream collector did not terminate.");
+             }
              return collector.get();
          } catch (IOException | InterruptedException e) {
              return null;
          }
      }
@iBotPeaches

This comment has been minimized.

Copy link
Owner

iBotPeaches commented Jun 11, 2018

@sunshouxiang thank you! I will take a look. In the future, you usually just "fork" the project, so you own the repo and can make whatever branch, then make a pull request from your branch and repo to this project.

iBotPeaches added a commit that referenced this issue Jul 23, 2018

@iBotPeaches iBotPeaches added this to the 2.3.4 milestone Jul 23, 2018

@iBotPeaches

This comment has been minimized.

Copy link
Owner

iBotPeaches commented Jul 23, 2018

Merged into the codebase. Thanks again, sorry for the delay.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.