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.

Sign up for GitHub

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

revert due to buffer overflow on subprocess #21762

Merged
merged 1 commit into from
Aug 4, 2023
Merged

revert due to buffer overflow on subprocess #21762

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 4 additions & 23 deletions src/client/testing/testController/common/server.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,10 @@

import * as net from 'net';
import * as crypto from 'crypto';
import { Disposable, Event, EventEmitter, TestRun } from 'vscode';
import { Disposable, Event, EventEmitter } from 'vscode';
import * as path from 'path';
import {
ExecutionFactoryCreateWithEnvironmentOptions,
ExecutionResult,
IPythonExecutionFactory,
SpawnOptions,
} from '../../../common/process/types';
Expand All @@ -16,7 +15,6 @@ import { DataReceivedEvent, ITestServer, TestCommandOptions } from './types';
import { ITestDebugLauncher, LaunchOptions } from '../../common/types';
import { UNITTEST_PROVIDER } from '../../common/constants';
import { jsonRPCHeaders, jsonRPCContent, JSONRPC_UUID_HEADER } from './utils';
import { createDeferred } from '../../../common/utils/async';

export class PythonTestServer implements ITestServer, Disposable {
private _onDataReceived: EventEmitter<DataReceivedEvent> = new EventEmitter<DataReceivedEvent>();
Expand Down Expand Up @@ -142,12 +140,7 @@ export class PythonTestServer implements ITestServer, Disposable {
return this._onDataReceived.event;
}

async sendCommand(
options: TestCommandOptions,
runTestIdPort?: string,
runInstance?: TestRun,
callback?: () => void,
): Promise<void> {
async sendCommand(options: TestCommandOptions, runTestIdPort?: string, callback?: () => void): Promise<void> {
const { uuid } = options;

const pythonPathParts: string[] = process.env.PYTHONPATH?.split(path.delimiter) ?? [];
Expand All @@ -161,7 +154,7 @@ export class PythonTestServer implements ITestServer, Disposable {
};

if (spawnOptions.extraVariables) spawnOptions.extraVariables.RUN_TEST_IDS_PORT = runTestIdPort;
const isRun = runTestIdPort !== undefined;
const isRun = !options.testIds;
// Create the Python environment in which to execute the command.
const creationOptions: ExecutionFactoryCreateWithEnvironmentOptions = {
allowEnvironmentFetchExceptions: false,
Expand Down Expand Up @@ -202,19 +195,7 @@ export class PythonTestServer implements ITestServer, Disposable {
// This means it is running discovery
traceLog(`Discovering unittest tests with arguments: ${args}\r\n`);
}
const deferred = createDeferred<ExecutionResult<string>>();

const result = execService.execObservable(args, spawnOptions);

runInstance?.token.onCancellationRequested(() => {
result?.proc?.kill();
});
result?.proc?.on('close', () => {
traceLog('Exec server closed.', uuid);
deferred.resolve({ stdout: '', stderr: '' });
callback?.();
});
await deferred.promise;
await execService.exec(args, spawnOptions);
}
} catch (ex) {
this.uuids = this.uuids.filter((u) => u !== uuid);
Expand Down
7 changes: 1 addition & 6 deletions src/client/testing/testController/common/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,12 +174,7 @@ export interface ITestServer {
readonly onDataReceived: Event<DataReceivedEvent>;
readonly onRunDataReceived: Event<DataReceivedEvent>;
readonly onDiscoveryDataReceived: Event<DataReceivedEvent>;
sendCommand(
options: TestCommandOptions,
runTestIdsPort?: string,
runInstance?: TestRun,
callback?: () => void,
): Promise<void>;
sendCommand(options: TestCommandOptions, runTestIdsPort?: string, callback?: () => void): Promise<void>;
serverReady(): Promise<void>;
getPort(): number;
createUUID(cwd: string): string;
Expand Down
27 changes: 14 additions & 13 deletions src/client/testing/testController/pytest/pytestDiscoveryAdapter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,13 @@ import * as path from 'path';
import { Uri } from 'vscode';
import {
ExecutionFactoryCreateWithEnvironmentOptions,
ExecutionResult,
IPythonExecutionFactory,
SpawnOptions,
} from '../../../common/process/types';
import { IConfigurationService, ITestOutputChannel } from '../../../common/types';
import { createDeferred } from '../../../common/utils/async';
import { EXTENSION_ROOT_DIR } from '../../../constants';
import { traceVerbose } from '../../../logging';
import { traceError, traceVerbose } from '../../../logging';
import {
DataReceivedEvent,
DiscoveredTestPayload,
Expand Down Expand Up @@ -49,7 +48,7 @@ export class PytestTestDiscoveryAdapter implements ITestDiscoveryAdapter {
return discoveryPayload;
}

async runPytestDiscovery(uri: Uri, executionFactory?: IPythonExecutionFactory): Promise<void> {
async runPytestDiscovery(uri: Uri, executionFactory?: IPythonExecutionFactory): Promise<DiscoveredTestPayload> {
const deferred = createDeferred<DiscoveredTestPayload>();
const relativePathToPytest = 'pythonFiles';
const fullPluginPath = path.join(EXTENSION_ROOT_DIR, relativePathToPytest);
Expand Down Expand Up @@ -79,15 +78,17 @@ export class PytestTestDiscoveryAdapter implements ITestDiscoveryAdapter {
};
const execService = await executionFactory?.createActivatedEnvironment(creationOptions);
// delete UUID following entire discovery finishing.
const deferredExec = createDeferred<ExecutionResult<string>>();
const execArgs = ['-m', 'pytest', '-p', 'vscode_pytest', '--collect-only'].concat(pytestArgs);
const result = execService?.execObservable(execArgs, spawnOptions);

result?.proc?.on('close', () => {
deferredExec.resolve({ stdout: '', stderr: '' });
this.testServer.deleteUUID(uuid);
deferred.resolve();
});
await deferredExec.promise;
execService
?.exec(['-m', 'pytest', '-p', 'vscode_pytest', '--collect-only'].concat(pytestArgs), spawnOptions)
.then(() => {
this.testServer.deleteUUID(uuid);
return deferred.resolve();
})
.catch((err) => {
traceError(`Error while trying to run pytest discovery, \n${err}\r\n\r\n`);
this.testServer.deleteUUID(uuid);
return deferred.reject(err);
});
return deferred.promise;
}
}
45 changes: 17 additions & 28 deletions src/client/testing/testController/pytest/pytestExecutionAdapter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,15 +15,20 @@ import {
} from '../common/types';
import {
ExecutionFactoryCreateWithEnvironmentOptions,
ExecutionResult,
IPythonExecutionFactory,
SpawnOptions,
} from '../../../common/process/types';
import { removePositionalFoldersAndFiles } from './arguments';
import { ITestDebugLauncher, LaunchOptions } from '../../common/types';
import { PYTEST_PROVIDER } from '../../common/constants';
import { EXTENSION_ROOT_DIR } from '../../../common/constants';
import * as utils from '../common/utils';
import { startTestIdServer } from '../common/utils';

// eslint-disable-next-line @typescript-eslint/no-explicit-any
// (global as any).EXTENSION_ROOT_DIR = EXTENSION_ROOT_DIR;
/**
* Wrapper Class for pytest test execution..
*/

export class PytestTestExecutionAdapter implements ITestExecutionAdapter {
constructor(
Expand All @@ -43,20 +48,18 @@ export class PytestTestExecutionAdapter implements ITestExecutionAdapter {
): Promise<ExecutionTestPayload> {
const uuid = this.testServer.createUUID(uri.fsPath);
traceVerbose(uri, testIds, debugBool);
const disposedDataReceived = this.testServer.onRunDataReceived((e: DataReceivedEvent) => {
const disposable = this.testServer.onRunDataReceived((e: DataReceivedEvent) => {
if (runInstance) {
this.resultResolver?.resolveExecution(JSON.parse(e.data), runInstance);
}
});
const dispose = function (testServer: ITestServer) {
testServer.deleteUUID(uuid);
disposedDataReceived.dispose();
};
runInstance?.token.onCancellationRequested(() => {
dispose(this.testServer);
});
await this.runTestsNew(uri, testIds, uuid, runInstance, debugBool, executionFactory, debugLauncher);

try {
await this.runTestsNew(uri, testIds, uuid, debugBool, executionFactory, debugLauncher);
} finally {
this.testServer.deleteUUID(uuid);
disposable.dispose();
// confirm with testing that this gets called (it must clean this up)
}
// placeholder until after the rewrite is adopted
// TODO: remove after adoption.
const executionPayload: ExecutionTestPayload = {
Expand All @@ -71,7 +74,6 @@ export class PytestTestExecutionAdapter implements ITestExecutionAdapter {
uri: Uri,
testIds: string[],
uuid: string,
runInstance?: TestRun,
debugBool?: boolean,
executionFactory?: IPythonExecutionFactory,
debugLauncher?: ITestDebugLauncher,
Expand Down Expand Up @@ -122,7 +124,7 @@ export class PytestTestExecutionAdapter implements ITestExecutionAdapter {
}
traceLog(`Running PYTEST execution for the following test ids: ${testIds}`);

const pytestRunTestIdsPort = await utils.startTestIdServer(testIds);
const pytestRunTestIdsPort = await startTestIdServer(testIds);
if (spawnOptions.extraVariables)
spawnOptions.extraVariables.RUN_TEST_IDS_PORT = pytestRunTestIdsPort.toString();

Expand All @@ -141,27 +143,14 @@ export class PytestTestExecutionAdapter implements ITestExecutionAdapter {
traceInfo(`Running DEBUG pytest with arguments: ${testArgs.join(' ')}\r\n`);
await debugLauncher!.launchDebugger(launchOptions, () => {
deferred.resolve();
this.testServer.deleteUUID(uuid);
});
} else {
// combine path to run script with run args
const scriptPath = path.join(fullPluginPath, 'vscode_pytest', 'run_pytest_script.py');
const runArgs = [scriptPath, ...testArgs];
traceInfo(`Running pytests with arguments: ${runArgs.join(' ')}\r\n`);

const deferredExec = createDeferred<ExecutionResult<string>>();
const result = execService?.execObservable(runArgs, spawnOptions);

runInstance?.token.onCancellationRequested(() => {
result?.proc?.kill();
});

result?.proc?.on('close', () => {
deferredExec.resolve({ stdout: '', stderr: '' });
this.testServer.deleteUUID(uuid);
deferred.resolve();
});
await deferredExec.promise;
await execService?.exec(runArgs, spawnOptions);
}
} catch (ex) {
traceError(`Error while running tests: ${testIds}\r\n${ex}\r\n\r\n`);
Expand Down
11 changes: 6 additions & 5 deletions src/client/testing/testController/unittest/testDiscoveryAdapter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,11 +46,12 @@ export class UnittestTestDiscoveryAdapter implements ITestDiscoveryAdapter {
const disposable = this.testServer.onDiscoveryDataReceived((e: DataReceivedEvent) => {
this.resultResolver?.resolveDiscovery(JSON.parse(e.data));
});

await this.callSendCommand(options, () => {
try {
await this.callSendCommand(options);
} finally {
this.testServer.deleteUUID(uuid);
disposable.dispose();
});
}
// placeholder until after the rewrite is adopted
// TODO: remove after adoption.
const discoveryPayload: DiscoveredTestPayload = {
Expand All @@ -60,8 +61,8 @@ export class UnittestTestDiscoveryAdapter implements ITestDiscoveryAdapter {
return discoveryPayload;
}

private async callSendCommand(options: TestCommandOptions, callback: () => void): Promise<DiscoveredTestPayload> {
await this.testServer.sendCommand(options, undefined, undefined, callback);
private async callSendCommand(options: TestCommandOptions): Promise<DiscoveredTestPayload> {
await this.testServer.sendCommand(options);
const discoveryPayload: DiscoveredTestPayload = { cwd: '', status: 'success' };
return discoveryPayload;
}
Expand Down
21 changes: 8 additions & 13 deletions src/client/testing/testController/unittest/testExecutionAdapter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,19 +37,18 @@ export class UnittestTestExecutionAdapter implements ITestExecutionAdapter {
runInstance?: TestRun,
): Promise<ExecutionTestPayload> {
const uuid = this.testServer.createUUID(uri.fsPath);
const disposedDataReceived = this.testServer.onRunDataReceived((e: DataReceivedEvent) => {
const disposable = this.testServer.onRunDataReceived((e: DataReceivedEvent) => {
if (runInstance) {
this.resultResolver?.resolveExecution(JSON.parse(e.data), runInstance);
}
});
const dispose = function () {
disposedDataReceived.dispose();
};
runInstance?.token.onCancellationRequested(() => {
try {
await this.runTestsNew(uri, testIds, uuid, debugBool);
} finally {
this.testServer.deleteUUID(uuid);
dispose();
});
await this.runTestsNew(uri, testIds, uuid, runInstance, debugBool, dispose);
disposable.dispose();
// confirm with testing that this gets called (it must clean this up)
}
const executionPayload: ExecutionTestPayload = { cwd: uri.fsPath, status: 'success', error: '' };
return executionPayload;
}
Expand All @@ -58,9 +57,7 @@ export class UnittestTestExecutionAdapter implements ITestExecutionAdapter {
uri: Uri,
testIds: string[],
uuid: string,
runInstance?: TestRun,
debugBool?: boolean,
dispose?: () => void,
): Promise<ExecutionTestPayload> {
const settings = this.configSettings.getSettings(uri);
const { unittestArgs } = settings.testing;
Expand All @@ -83,10 +80,8 @@ export class UnittestTestExecutionAdapter implements ITestExecutionAdapter {

const runTestIdsPort = await startTestIdServer(testIds);

await this.testServer.sendCommand(options, runTestIdsPort.toString(), runInstance, () => {
this.testServer.deleteUUID(uuid);
await this.testServer.sendCommand(options, runTestIdsPort.toString(), () => {
deferred.resolve();
dispose?.();
});
// placeholder until after the rewrite is adopted
// TODO: remove after adoption.
Expand Down
Loading