Remove timeout from advanced security scan (#444)

* Refactor execute command cancellation

* Handle errors better when killing child processes

src/main/scanLogic/scanRunners/analyzerManager.ts

@@ -8,7 +8,6 @@ import { IProxyConfig, JfrogClient } from 'jfrog-client-js';
 import { ConnectionUtils } from '../../connect/connectionUtils';
 import { Configuration } from '../../utils/configuration';
 import { Translators } from '../../utils/translators';
-import { RunUtils } from '../../utils/runUtils';
 
 /**
  * Analyzer manager is responsible for running the analyzer on the workspace.
@@ -30,7 +29,6 @@ export class AnalyzerManager {
             AnalyzerManager.BINARY_NAME
     );
     private static readonly JFROG_RELEASES_URL: string = 'https://releases.jfrog.io';
-    public static readonly TIMEOUT_MILLISECS: number = 1000 * 60 * 5;
     public static readonly ENV_PLATFORM_URL: string = 'JF_PLATFORM_URL';
     public static readonly ENV_TOKEN: string = 'JF_TOKEN';
     public static readonly ENV_USER: string = 'JF_USER';
@@ -93,27 +91,15 @@ export class AnalyzerManager {
         return false;
     }
 
-    public async runWithTimeout(checkCancel: () => void, args: string[], executionLogDirectory?: string): Promise<void> {
-        await AnalyzerManager.FINISH_UPDATE_PROMISE;
-        await RunUtils.runWithTimeout(AnalyzerManager.TIMEOUT_MILLISECS, checkCancel, {
-            title: this._binary.name,
-            task: this.run(args, executionLogDirectory)
-        });
-    }
-
     /**
      * Execute the cmd command to run the binary with given arguments
-     * @param args  - the arguments for the command
+     * @param args - the arguments for the command
+     * @param checkCancel - A function that throws ScanCancellationError if the user chose to stop the scan
      * @param executionLogDirectory - the directory to save the execution log in
      */
-    private async run(args: string[], executionLogDirectory?: string): Promise<any> {
-        let std: any = await this._binary.run(args, this.createEnvForRun(executionLogDirectory));
-        if (std.stdout && std.stdout.length > 0) {
-            this._logManager.logMessage('Done executing with log, log:\n' + std.stdout, 'DEBUG');
-        }
-        if (std.stderr && std.stderr.length > 0) {
-            this._logManager.logMessage('Done executing with log, log:\n' + std.stderr, 'ERR');
-        }
+    public async run(args: string[], checkCancel: () => void, executionLogDirectory?: string): Promise<void> {
+        await AnalyzerManager.FINISH_UPDATE_PROMISE;
+        await this._binary.run(args, checkCancel, this.createEnvForRun(executionLogDirectory));
     }
 
     /**

src/main/scanLogic/scanRunners/applicabilityScan.ts

@@ -61,7 +61,7 @@ export class ApplicabilityRunner extends JasRunner {
 
     /** @override */
     protected async runBinary(yamlConfigPath: string, executionLogDirectory: string | undefined, checkCancel: () => void): Promise<void> {
-        await this.executeBinary(checkCancel, ['ca', yamlConfigPath], executionLogDirectory);
+        await this.runAnalyzerManager(checkCancel, ['ca', yamlConfigPath], executionLogDirectory);
     }
 
     /** @override */

src/main/scanLogic/scanRunners/iacScan.ts

@@ -31,7 +31,7 @@ export class IacRunner extends JasRunner {
 
     /** @override */
     protected async runBinary(yamlConfigPath: string, executionLogDirectory: string | undefined, checkCancel: () => void): Promise<void> {
-        await this.executeBinary(checkCancel, ['iac', yamlConfigPath], executionLogDirectory);
+        await this.runAnalyzerManager(checkCancel, ['iac', yamlConfigPath], executionLogDirectory);
     }
 
     /**

src/main/scanLogic/scanRunners/jasRunner.ts

@@ -87,13 +87,14 @@ export abstract class JasRunner {
     }
 
     /**
-     * Execute the cmd command to run the binary with given arguments and an option to abort the operation.
-     * @param checkCancel           - Check if should cancel
-     * @param args                  - Arguments for the command
+     * Run Analyzer Manager with given arguments and an option to abort the operation.
+     * @param checkCancel - A function that throws ScanCancellationError if the user chose to stop the scan
+     * @param args - Arguments for the command
      * @param executionLogDirectory - Directory to save the execution log in
      */
-    protected async executeBinary(checkCancel: () => void, args: string[], executionLogDirectory?: string): Promise<void> {
-        await this._analyzerManager.runWithTimeout(checkCancel, args, executionLogDirectory);
+    protected async runAnalyzerManager(checkCancel: () => void, args: string[], executionLogDirectory?: string): Promise<void> {
+        checkCancel();
+        await this._analyzerManager.run(args, checkCancel, executionLogDirectory);
     }
 
     protected logStartScanning(request: AnalyzeScanRequest): void {

src/main/scanLogic/scanRunners/sastScan.ts

@@ -74,7 +74,7 @@ export class SastRunner extends JasRunner {
         checkCancel: () => void,
         responsePath: string
     ): Promise<void> {
-        await this.executeBinary(checkCancel, ['zd', yamlConfigPath, responsePath], executionLogDirectory);
+        await this.runAnalyzerManager(checkCancel, ['zd', yamlConfigPath, responsePath], executionLogDirectory);
     }
 
     /** @override */

src/main/scanLogic/scanRunners/secretsScan.ts

@@ -31,7 +31,7 @@ export class SecretsRunner extends JasRunner {
 
     /** @override */
     protected async runBinary(yamlConfigPath: string, executionLogDirectory: string | undefined, checkCancel: () => void): Promise<void> {
-        await this.executeBinary(checkCancel, ['sec', yamlConfigPath], executionLogDirectory);
+        await this.runAnalyzerManager(checkCancel, ['sec', yamlConfigPath], executionLogDirectory);
     }
 
     /**

src/main/utils/resource.ts

@@ -1,6 +1,5 @@
 import * as fs from 'fs';
 import * as path from 'path';
-
 import { IChecksumResult, JfrogClient } from 'jfrog-client-js';
 import { LogManager } from '../log/logManager';
 import { Utils } from './utils';
@@ -155,10 +154,17 @@ export class Resource {
         return ScanUtils.Hash('SHA256', fileBuffer);
     }
 
-    public async run(args: string[], env?: NodeJS.ProcessEnv | undefined): Promise<any> {
+    public async run(args: string[], checkCancel: () => void, env?: NodeJS.ProcessEnv | undefined): Promise<void> {
         let command: string = '"' + this.fullPath + '" ' + args.join(' ');
         this._logManager.debug("Executing '" + command + "' in directory '" + this._targetDir + "'");
-        return await ScanUtils.executeCmdAsync(command, this._targetDir, env);
+        try {
+            const output: string = await ScanUtils.executeCmdAsync(command, checkCancel, this._targetDir, env);
+            if (output.length > 0) {
+                this._logManager.logMessage('Done executing "' + command + '" with output:\n' + output, 'DEBUG');
+            }
+        } catch (error) {
+            throw new Error('Failed to execute "' + command + '" err: ' + error);
+        }
     }
 
     public get fullPath(): string {

src/main/utils/runUtils.ts

@@ -1,53 +1,4 @@
-import { ScanTimeoutError } from './scanUtils';
-
-export interface Task<T> {
-    title: string;
-    task: Promise<T>;
-}
-
 export class RunUtils {
-    // every 0.1 sec
-    private static readonly CHECK_INTERVAL_MILLISECS: number = 100;
-
-    /**
-     * Creates a Promise that is resolved with an array of results when all of the provided Promises resolve, or rejected when:
-     * 1. Any Promise is rejected.
-     * 2. Cancel was requested.
-     * 3. Timeout reached.
-     * @param timeout - time in millisecs until execution timeout
-     * @param checkCancel - check if cancel was requested
-     * @param tasks - the promises that the new promise will wrap
-     * @returns Promise that wrap the given promises
-     */
-    static async runWithTimeout<T>(timeout: number, checkCancel: () => void, ...tasks: (Promise<T> | Task<T>)[]): Promise<T[]> {
-        let results: T[] = [];
-        const wrappedTasks: Promise<T>[] = <Promise<T>[]>tasks.map(async (task, index) => {
-            let result: T = <T>await Promise.race([
-                // Add task from argument
-                !(task instanceof Promise) && task.task ? task.task : task,
-                // Add task to check if cancel was requested from the user or reached timeout
-                this.checkCancelAndTimeoutTask(!(task instanceof Promise) && task.title ? task.title : '' + (index + 1), timeout, checkCancel)
-            ]);
-            results.push(result);
-        });
-        await Promise.all(wrappedTasks);
-        return results;
-    }
-
-    /**
-     * Async task that checks if an abort signal was given.
-     * If the active task is <= 0 the task is completed
-     * @param tasksBundle - an object that holds the information about the active async tasks count and the abort signal for them
-     */
-    private static async checkCancelAndTimeoutTask(title: string, timeout: number, checkCancel: () => void): Promise<void> {
-        let checkInterval: number = timeout < RunUtils.CHECK_INTERVAL_MILLISECS ? timeout : RunUtils.CHECK_INTERVAL_MILLISECS;
-        for (let elapsed: number = 0; elapsed < timeout; elapsed += checkInterval) {
-            checkCancel();
-            await this.delay(checkInterval);
-        }
-        throw new ScanTimeoutError(title, timeout);
-    }
-
     /**
      * Sleep and delay task for sleepIntervalMilliseconds
      * @param sleepIntervalMilliseconds - the amount of time in milliseconds to wait

src/main/utils/scanUtils.ts

@@ -5,7 +5,6 @@ import * as fse from 'fs-extra';
 import * as os from 'os';
 import * as path from 'path';
 import * as tmp from 'tmp';
-import * as util from 'util';
 import * as vscode from 'vscode';
 import { ContextKeys } from '../constants/contextKeys';
 import { LogManager } from '../log/logManager';
@@ -21,6 +20,8 @@ export class ScanUtils {
 
     public static readonly RESOURCES_DIR: string = ScanUtils.getResourcesDir();
     public static readonly SPAWN_PROCESS_BUFFER_SIZE: number = 104857600;
+    // every 0.1 sec
+    private static readonly CANCELLATION_CHECK_INTERVAL_MS: number = 100;
 
     public static async scanWithProgress(
         scanCbk: (progress: vscode.Progress<{ message?: string; increment?: number }>, checkCanceled: () => void) => Promise<void>,
@@ -158,8 +159,62 @@ export class ScanUtils {
         return exec.execSync(command, { cwd: cwd, maxBuffer: ScanUtils.SPAWN_PROCESS_BUFFER_SIZE, env: env });
     }
 
-    public static async executeCmdAsync(command: string, cwd?: string, env?: NodeJS.ProcessEnv | undefined): Promise<any> {
-        return await util.promisify(exec.exec)(command, { cwd: cwd, maxBuffer: ScanUtils.SPAWN_PROCESS_BUFFER_SIZE, env: env });
+    /**
+     * Executes a command asynchronously and returns the output.
+     * @param command - The command to execute.
+     * @param checkCancel - A function that throws ScanCancellationError if the user chose to stop the scan
+     * @param cwd - The current working directory for the command execution.
+     * @param env - Optional environment variables for the command execution.
+     * @returns Command output or rejects with an error.
+     */
+    public static async executeCmdAsync(
+        command: string,
+        checkCancel: () => void,
+        cwd?: string,
+        env?: NodeJS.ProcessEnv | undefined
+    ): Promise<string> {
+        return new Promise<string>((resolve, reject) => {
+            try {
+                const childProcess: exec.ChildProcess = exec.exec(
+                    command,
+                    { cwd: cwd, maxBuffer: ScanUtils.SPAWN_PROCESS_BUFFER_SIZE, env: env } as exec.ExecOptions,
+                    (error: exec.ExecException | null, stdout: string, stderr: string) => {
+                        clearInterval(checkCancellationInterval);
+                        if (error) {
+                            reject(error);
+                        } else {
+                            stderr.trim() ? reject(new Error(stderr.trim())) : resolve(stdout.trim());
+                        }
+                    }
+                );
+
+                const checkCancellationInterval: NodeJS.Timer = setInterval(() => {
+                    ScanUtils.cancelIfRequested(childProcess, checkCancel, reject);
+                }, ScanUtils.CANCELLATION_CHECK_INTERVAL_MS);
+            } catch (error) {
+                reject(error);
+            }
+        });
+    }
+
+    /**
+     * Cancels the child process if cancellation is requested.
+     * @param childProcess - The child process to be cancelled.
+     * @param checkCancel - A function that throws ScanCancellationError if the user chose to stop the scan
+     * @param reject - A function to reject the promise.
+     */
+    private static cancelIfRequested(childProcess: exec.ChildProcess, checkCancel: () => void, reject: (reason?: any) => void): void {
+        try {
+            checkCancel();
+        } catch (error) {
+            const killSuccess: boolean = childProcess.kill('SIGTERM');
+            if (!killSuccess) {
+                const originalError: string = error instanceof Error ? error.message : String(error);
+                reject(`${originalError}\nFailed to kill the process.`);
+                return;
+            }
+            reject(error);
+        }
     }
 
     public static setScanInProgress(state: boolean) {
@@ -253,7 +308,7 @@ export class ScanUtils {
             logger.logMessage(error.message, 'DEBUG');
             return undefined;
         }
-        if (handle || error instanceof ScanTimeoutError) {
+        if (handle) {
             logger.logError(error, true);
             return undefined;
         }
@@ -301,9 +356,3 @@ export class FileScanError extends Error {
 export class ScanCancellationError extends Error {
     message: string = 'Scan was cancelled';
 }
-
-export class ScanTimeoutError extends Error {
-    constructor(scan: string, public time_millisecs: number) {
-        super(`Task ${scan} timed out after ${time_millisecs}ms`);
-    }
-}