mirror of
https://github.com/compiler-explorer/compiler-explorer.git
synced 2026-07-22 01:46:48 -04:00
Fix hang and uncaught exception when conan download is severed mid-stream (#8812)
Part of #8811 (production disk-full incident). ## The incident bug If a conan library package download's connection died mid-transfer, `downloadAndExtractPackage()`: - threw an **uncaught exception** (`TypeError: terminated`): the error was emitted on the `Readable.from(res.body)` wrapper, which had no `'error'` listener — `.pipe()` neither attaches nor forwards one; - **never settled its promise**: gunzip/tar saw neither an error nor an end, so neither `resolve` nor `reject` was ever called. This wedged a compilation queue slot forever, which permanently disabled temp dir cleanup (it only runs when the queue is idle) and filled the disk on prod. The download path is rewritten around `stream.pipeline()`, which propagates errors through every stage and guarantees settlement. Per-entry file writes also go through `pipeline()`, so write errors propagate (via `extract.destroy`) instead of being silently dropped, and `next()` fires after the file is fully flushed rather than on stream `'end'`. ## Extraction hardening (from review + an adversarial security pass) - **Zip-slip guard anchored at the per-library extraction root** (`downloadPath/<libId>`; plain `downloadPath` for `extractAllToRoot`): a package can no longer write over a sibling library's files or the compilation's own. The check is `path.relative`-based — immune to prefix collisions (`/tmp/pkg` vs `/tmp/pkg-evil`) and to directories merely *named* with leading dots. - **Only regular-file entries are ever written**: directories, symlinks, hardlinks and other types are drained and skipped, including malformed entries (e.g. a directory claiming non-zero size) whose body could otherwise land on disk as a file. CE never creates links of any kind; prod additionally runs under the `/nosym/tmp` nosymfollow mount. - **Zero-length files extract correctly** (the pre-pipeline code created them as a side effect of an early `createWriteStream` — with a leaked fd; the first pipeline version dropped them entirely). - **Logs preserve stacks** (error objects passed to winston, not interpolated) and the archive-controlled entry name is JSON-stringified. Deliberately out of scope (filed as #8817): decompressed-size caps and `packageUrl` scheme/redirect validation — defense-in-depth against our own conan server, not blockers. See the review-convergence comment below for vectors evaluated and rejected with rationale. ## Tests `test/buildenvsetup-ceconan-tests.ts` (new), against a local HTTP server: - **Severed mid-stream download rejects rather than hanging** — against the pre-fix code this reproduces the production failure exactly (unhandled `TypeError: terminated` + timeout). - Happy-path extraction including a zero-length file. - 404 → rejection. - Zip-slip: full escape and sibling-library escape are skipped; a `..`-named directory inside the library root still extracts. - A malformed sized-directory entry settles (no wedged promise) and writes nothing. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: mattgodbolt-molty <mattgodbolt-molty@users.noreply.github.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
committed by
GitHub
parent
23fd078e73
commit
a80b7eb210
@@ -26,6 +26,7 @@ import fs from 'node:fs';
|
||||
import os from 'node:os';
|
||||
import path from 'node:path';
|
||||
import {Readable} from 'node:stream';
|
||||
import {pipeline} from 'node:stream/promises';
|
||||
import zlib from 'node:zlib';
|
||||
|
||||
import tar from 'tar-stream';
|
||||
@@ -142,90 +143,93 @@ export class BuildEnvSetupCeConanDirect extends BuildEnvSetupBase {
|
||||
downloadPath: string,
|
||||
packageUrl: string,
|
||||
): Promise<BuildEnvDownloadInfo> {
|
||||
return new Promise((resolve, reject) => {
|
||||
const startTime = process.hrtime.bigint();
|
||||
const extract = tar.extract();
|
||||
const gunzip = zlib.createGunzip();
|
||||
const startTime = process.hrtime.bigint();
|
||||
const extract = tar.extract();
|
||||
const gunzip = zlib.createGunzip();
|
||||
|
||||
extract.on('entry', async (header, stream, next) => {
|
||||
try {
|
||||
const filepath = this.getDestinationFilepath(downloadPath, header.name, libId);
|
||||
extract.on('entry', async (header, stream, next) => {
|
||||
// Drained streams (skipped entries) have no other error listener; without this an
|
||||
// entry-stream error would throw as an unhandled 'error' event.
|
||||
stream.on('error', (error: any) => extract.destroy(error));
|
||||
try {
|
||||
const filepath = this.getDestinationFilepath(downloadPath, header.name, libId);
|
||||
|
||||
const resolved = path.resolve(path.dirname(filepath));
|
||||
if (!resolved.startsWith(downloadPath)) {
|
||||
logger.error(`Library ${libId}/${version} is using a zip-slip, skipping file`);
|
||||
stream.resume();
|
||||
next();
|
||||
return;
|
||||
}
|
||||
|
||||
if (!this.extractAllToRoot) {
|
||||
await fs.promises.mkdir(path.dirname(filepath), {recursive: true});
|
||||
}
|
||||
|
||||
const filestream = fs.createWriteStream(filepath);
|
||||
if (header.size === 0) {
|
||||
// See https://github.com/mafintosh/tar-stream/issues/145
|
||||
stream.resume();
|
||||
next();
|
||||
} else {
|
||||
stream
|
||||
.on('error', (error: any) => {
|
||||
logger.error(`Error in stream handling: ${error}`);
|
||||
reject(error);
|
||||
})
|
||||
.on('end', next)
|
||||
.pipe(filestream);
|
||||
stream.resume();
|
||||
}
|
||||
} catch (error) {
|
||||
logger.error(`Error in entry handling: ${error}`);
|
||||
reject(error);
|
||||
// Each library extracts under its own root; skip entries that would resolve outside
|
||||
// it (and so could overwrite other libraries' files or the compilation's own).
|
||||
const extractionRoot = path.resolve(
|
||||
this.extractAllToRoot ? downloadPath : path.join(downloadPath, libId),
|
||||
);
|
||||
const relative = path.relative(extractionRoot, path.resolve(path.dirname(filepath)));
|
||||
if (relative === '..' || relative.startsWith(`..${path.sep}`) || path.isAbsolute(relative)) {
|
||||
logger.error(`Library ${libId}/${version} is using a zip-slip, skipping file`);
|
||||
stream.resume();
|
||||
next();
|
||||
return;
|
||||
}
|
||||
});
|
||||
|
||||
extract
|
||||
.on('error', (error: any) => {
|
||||
logger.error(`Error in tar handling: ${error}`);
|
||||
reject(error);
|
||||
})
|
||||
.on('finish', () => {
|
||||
const endTime = process.hrtime.bigint();
|
||||
resolve({
|
||||
step: `Download of ${libId} ${version}`,
|
||||
packageUrl: packageUrl,
|
||||
time: utils.deltaTimeNanoToMili(startTime, endTime),
|
||||
});
|
||||
});
|
||||
if (header.type !== 'file') {
|
||||
// Only regular files are ever written; symlinks, hardlinks, directories etc
|
||||
// are drained and skipped. Draining also avoids a hang on malformed entries
|
||||
// (e.g. a directory with a non-zero size, whose stream tar-stream never ends).
|
||||
stream.resume();
|
||||
next();
|
||||
return;
|
||||
}
|
||||
|
||||
gunzip
|
||||
.on('error', error => {
|
||||
logger.error(`Error in gunzip handling: ${error}`);
|
||||
reject(error);
|
||||
})
|
||||
.pipe(extract);
|
||||
if (!this.extractAllToRoot) {
|
||||
await fs.promises.mkdir(path.dirname(filepath), {recursive: true});
|
||||
}
|
||||
|
||||
const settings: RequestInit = {
|
||||
method: 'GET',
|
||||
headers: {
|
||||
'Content-Type': 'application/json',
|
||||
},
|
||||
};
|
||||
|
||||
fetch(packageUrl, settings)
|
||||
.then((res: Response) => {
|
||||
if (res.ok && res.body) {
|
||||
Readable.from(res.body).pipe(gunzip);
|
||||
} else {
|
||||
logger.error(`Error requesting package from conan: ${res.status} for ${packageUrl}`);
|
||||
reject(new Error(`Unable to request library from conan: ${res.status}`));
|
||||
}
|
||||
})
|
||||
.catch((error: any) => {
|
||||
logger.error(`Error in request handling: ${error}`);
|
||||
reject(error);
|
||||
});
|
||||
if (header.size === 0) {
|
||||
// tar-stream emits no data for zero-length entries (see
|
||||
// https://github.com/mafintosh/tar-stream/issues/145): create empty files
|
||||
// explicitly.
|
||||
await fs.promises.writeFile(filepath, '');
|
||||
stream.resume();
|
||||
next();
|
||||
} else {
|
||||
await pipeline(stream, fs.createWriteStream(filepath));
|
||||
next();
|
||||
}
|
||||
} catch (error) {
|
||||
logger.error(`Error extracting entry ${JSON.stringify(header.name)} of ${libId}/${version}:`, error);
|
||||
// Propagate the failure through the extract stream so the outer pipeline rejects.
|
||||
extract.destroy(error as Error);
|
||||
}
|
||||
});
|
||||
|
||||
const settings: RequestInit = {
|
||||
method: 'GET',
|
||||
headers: {
|
||||
'Content-Type': 'application/json',
|
||||
},
|
||||
};
|
||||
|
||||
const response = await fetch(packageUrl, settings);
|
||||
if (!response.ok || !response.body) {
|
||||
logger.error(`Error requesting package from conan: ${response.status} for ${packageUrl}`);
|
||||
throw new Error(`Unable to request library from conan: ${response.status}`);
|
||||
}
|
||||
|
||||
try {
|
||||
// pipeline propagates errors from every stage (including a download severed
|
||||
// mid-stream) and guarantees this settles; hand-rolled .pipe() chains do neither.
|
||||
await pipeline(
|
||||
Readable.fromWeb(response.body as import('node:stream/web').ReadableStream),
|
||||
gunzip,
|
||||
extract,
|
||||
);
|
||||
} catch (error) {
|
||||
logger.error(`Error downloading/extracting package from conan (${packageUrl}):`, error);
|
||||
throw error;
|
||||
}
|
||||
|
||||
const endTime = process.hrtime.bigint();
|
||||
return {
|
||||
step: `Download of ${libId} ${version}`,
|
||||
packageUrl: packageUrl,
|
||||
time: utils.deltaTimeNanoToMili(startTime, endTime),
|
||||
};
|
||||
}
|
||||
|
||||
async getConanBuildProperties(key: CacheKey, buildType = 'Debug'): Promise<ConanBuildProperties> {
|
||||
|
||||
201
test/buildenvsetup-ceconan-tests.ts
Normal file
201
test/buildenvsetup-ceconan-tests.ts
Normal file
@@ -0,0 +1,201 @@
|
||||
// Copyright (c) 2026, Compiler Explorer Authors
|
||||
// All rights reserved.
|
||||
//
|
||||
// Redistribution and use in source and binary forms, with or without
|
||||
// modification, are permitted provided that the following conditions are met:
|
||||
//
|
||||
// * Redistributions of source code must retain the above copyright notice,
|
||||
// this list of conditions and the following disclaimer.
|
||||
// * Redistributions in binary form must reproduce the above copyright
|
||||
// notice, this list of conditions and the following disclaimer in the
|
||||
// documentation and/or other materials provided with the distribution.
|
||||
//
|
||||
// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
|
||||
// AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
|
||||
// IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
|
||||
// ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE
|
||||
// LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
|
||||
// CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
|
||||
// SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
|
||||
// INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
|
||||
// CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
|
||||
// ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
|
||||
// POSSIBILITY OF SUCH DAMAGE.
|
||||
|
||||
import crypto from 'node:crypto';
|
||||
import fs from 'node:fs';
|
||||
import http from 'node:http';
|
||||
import path from 'node:path';
|
||||
import zlib from 'node:zlib';
|
||||
|
||||
import tar from 'tar-stream';
|
||||
import {afterAll, beforeAll, describe, expect, it} from 'vitest';
|
||||
|
||||
import {BuildEnvSetupCeConanDirect} from '../lib/buildenvsetup/ceconan.js';
|
||||
import * as temp from '../lib/temp.js';
|
||||
import {makeCompilationEnvironment, makeFakeCompilerInfo} from './utils.js';
|
||||
|
||||
const languages = {
|
||||
'c++': {id: 'c++'},
|
||||
} as const;
|
||||
|
||||
// Hand-built tar entry: tar-stream's packer won't produce malformed archives (e.g. a
|
||||
// directory entry claiming a non-zero size), which we need to test robustness against.
|
||||
function rawTarEntry(name: string, typeflag: string, size: number, content: Buffer): Buffer {
|
||||
const header = Buffer.alloc(512);
|
||||
header.write(name, 0);
|
||||
header.write('0000777\0', 100);
|
||||
header.write('0000000\0', 108);
|
||||
header.write('0000000\0', 116);
|
||||
header.write(`${size.toString(8).padStart(11, '0')}\0`, 124);
|
||||
header.write('00000000000\0', 136);
|
||||
header.fill(' ', 148, 156);
|
||||
header.write(typeflag, 156);
|
||||
header.write('ustar\0', 257);
|
||||
header.write('00', 263);
|
||||
let checksum = 0;
|
||||
for (const byte of header) checksum += byte;
|
||||
header.write(`${checksum.toString(8).padStart(6, '0')}\0 `, 148);
|
||||
const body = Buffer.alloc(Math.ceil(size / 512) * 512);
|
||||
content.copy(body);
|
||||
return Buffer.concat([header, body]);
|
||||
}
|
||||
|
||||
async function makeTarGz(files: Record<string, string>): Promise<Buffer> {
|
||||
const pack = tar.pack();
|
||||
for (const [name, content] of Object.entries(files)) {
|
||||
pack.entry({name}, content);
|
||||
}
|
||||
pack.finalize();
|
||||
const chunks: Buffer[] = [];
|
||||
for await (const chunk of pack) {
|
||||
chunks.push(chunk as Buffer);
|
||||
}
|
||||
return zlib.gzipSync(Buffer.concat(chunks));
|
||||
}
|
||||
|
||||
describe('BuildEnvSetupCeConanDirect.downloadAndExtractPackage', () => {
|
||||
let server: http.Server;
|
||||
let baseUrl: string;
|
||||
let setup: BuildEnvSetupCeConanDirect;
|
||||
let packageTarGz: Buffer;
|
||||
let zipSlipTarGz: Buffer;
|
||||
let sizedDirTarGz: Buffer;
|
||||
const fileContents = crypto.randomBytes(64 * 1024).toString('hex');
|
||||
|
||||
beforeAll(async () => {
|
||||
packageTarGz = await makeTarGz({
|
||||
'include/somelib.h': fileContents,
|
||||
'empty.txt': '',
|
||||
});
|
||||
zipSlipTarGz = await makeTarGz({
|
||||
'good.txt': 'good',
|
||||
// A directory merely *named* with leading dots is legitimate, not a traversal.
|
||||
'..odd/inside.txt': 'inside',
|
||||
// Escapes the download path entirely.
|
||||
'../../escape.txt': 'escaped',
|
||||
// Stays within the download path but escapes this library's own subdirectory.
|
||||
'../sibling.txt': 'sneaky',
|
||||
});
|
||||
sizedDirTarGz = zlib.gzipSync(
|
||||
Buffer.concat([
|
||||
rawTarEntry('ok.txt', '0', 2, Buffer.from('ok')),
|
||||
rawTarEntry('evildir/', '5', 10, Buffer.from('whoops....')),
|
||||
Buffer.alloc(1024),
|
||||
]),
|
||||
);
|
||||
|
||||
server = http.createServer((req, res) => {
|
||||
if (req.url === '/package.tgz') {
|
||||
res.writeHead(200, {'Content-Type': 'application/octet-stream'});
|
||||
res.end(packageTarGz);
|
||||
} else if (req.url === '/zipslip.tgz') {
|
||||
res.writeHead(200, {'Content-Type': 'application/octet-stream'});
|
||||
res.end(zipSlipTarGz);
|
||||
} else if (req.url === '/sizeddir.tgz') {
|
||||
res.writeHead(200, {'Content-Type': 'application/octet-stream'});
|
||||
res.end(sizedDirTarGz);
|
||||
} else if (req.url === '/severed.tgz') {
|
||||
// Send a valid gzip prefix then cut the connection mid-stream, as seen when a
|
||||
// download is interrupted: the client must reject, not hang forever.
|
||||
res.writeHead(200, {'Content-Type': 'application/octet-stream'});
|
||||
res.write(packageTarGz.subarray(0, Math.floor(packageTarGz.length / 2)));
|
||||
setTimeout(() => res.destroy(), 50);
|
||||
} else {
|
||||
res.writeHead(404);
|
||||
res.end();
|
||||
}
|
||||
});
|
||||
await new Promise<void>(resolve => server.listen(0, '127.0.0.1', resolve));
|
||||
const address = server.address();
|
||||
if (address === null || typeof address !== 'object') throw new Error('no server address');
|
||||
baseUrl = `http://127.0.0.1:${address.port}`;
|
||||
|
||||
const ce = makeCompilationEnvironment({languages});
|
||||
const compilerInfo = makeFakeCompilerInfo({
|
||||
lang: 'c++',
|
||||
exe: '/dev/null',
|
||||
options: '',
|
||||
buildenvsetup: {
|
||||
id: 'ceconan',
|
||||
props: (name: string, def?: any) => def,
|
||||
},
|
||||
});
|
||||
setup = new BuildEnvSetupCeConanDirect(compilerInfo, ce);
|
||||
});
|
||||
|
||||
afterAll(async () => {
|
||||
await new Promise<void>(resolve => {
|
||||
server.close(() => resolve());
|
||||
});
|
||||
});
|
||||
|
||||
it('downloads and extracts a package', async () => {
|
||||
const downloadPath = await temp.mkdir('ce-conan-test');
|
||||
const info = await setup.downloadAndExtractPackage('somelib', '1.0', downloadPath, `${baseUrl}/package.tgz`);
|
||||
expect(info.packageUrl).toEqual(`${baseUrl}/package.tgz`);
|
||||
const extracted = await fs.promises.readFile(path.join(downloadPath, 'somelib', 'include/somelib.h'), 'utf8');
|
||||
expect(extracted).toEqual(fileContents);
|
||||
const empty = await fs.promises.readFile(path.join(downloadPath, 'somelib', 'empty.txt'), 'utf8');
|
||||
expect(empty).toEqual('');
|
||||
});
|
||||
|
||||
it('skips entries that try to escape the library extraction root', async () => {
|
||||
const downloadPath = await temp.mkdir('ce-conan-test');
|
||||
await setup.downloadAndExtractPackage('somelib', '1.0', downloadPath, `${baseUrl}/zipslip.tgz`);
|
||||
await expect(fs.promises.readFile(path.join(downloadPath, 'somelib', 'good.txt'), 'utf8')).resolves.toEqual(
|
||||
'good',
|
||||
);
|
||||
await expect(
|
||||
fs.promises.readFile(path.join(downloadPath, 'somelib', '..odd', 'inside.txt'), 'utf8'),
|
||||
).resolves.toEqual('inside');
|
||||
await expect(fs.promises.access(path.resolve(downloadPath, '..', 'escape.txt'))).rejects.toThrow();
|
||||
await expect(fs.promises.access(path.join(downloadPath, 'sibling.txt'))).rejects.toThrow();
|
||||
});
|
||||
|
||||
it('rejects when the server returns an error status', async () => {
|
||||
const downloadPath = await temp.mkdir('ce-conan-test');
|
||||
await expect(
|
||||
setup.downloadAndExtractPackage('somelib', '1.0', downloadPath, `${baseUrl}/missing.tgz`),
|
||||
).rejects.toThrow('Unable to request library from conan: 404');
|
||||
});
|
||||
|
||||
it('does not hang on a directory entry claiming a non-zero size', async () => {
|
||||
const downloadPath = await temp.mkdir('ce-conan-test');
|
||||
// Whether such a malformed archive extracts or rejects is tar-stream's business; ours is
|
||||
// that the promise settles rather than wedging a compilation queue slot forever.
|
||||
await setup
|
||||
.downloadAndExtractPackage('somelib', '1.0', downloadPath, `${baseUrl}/sizeddir.tgz`)
|
||||
.catch(() => {});
|
||||
// The good entry preceding the malformed one proves the archive was actually processed.
|
||||
await expect(fs.promises.readFile(path.join(downloadPath, 'somelib', 'ok.txt'), 'utf8')).resolves.toEqual('ok');
|
||||
await expect(fs.promises.access(path.join(downloadPath, 'somelib', 'evildir'))).rejects.toThrow();
|
||||
}, 10_000);
|
||||
|
||||
it('rejects when the download is severed mid-stream', async () => {
|
||||
const downloadPath = await temp.mkdir('ce-conan-test');
|
||||
await expect(
|
||||
setup.downloadAndExtractPackage('somelib', '1.0', downloadPath, `${baseUrl}/severed.tgz`),
|
||||
).rejects.toThrow();
|
||||
}, 10_000);
|
||||
});
|
||||
Reference in New Issue
Block a user