Skip to content

Commit

Permalink
revert due to buffer overflow on subprocess (microsoft#21762)
Browse files Browse the repository at this point in the history 
revert microsoft#21667 because it
causes buffer overflow in the python testing subprocess when larger
repos are used. Specifically seen on pytest discovery with >200 tests.
Revert to align with the stable release and put in a fix next week.
  • Loading branch information
@eleanorjboyd
eleanorjboyd authored Aug 4, 2023
1 parent 40ff6e9 commit dd20561
Show file tree
Hide file tree
Showing 11 changed files with 157 additions and 664 deletions.
27 changes: 4 additions & 23 deletions src/client/testing/testController/common/server.ts
View file
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
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
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
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
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
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

0 comments on commit dd20561

Please sign in to comment.