undici leaking Symbol to global space in 18.2 (but not 18.1) #43157

Activity

1. climba03003 commented on May 20, 2022

   

   climba03003

   on May 20, 2022

   Contributor

   More actions

   I think it is true that the symbol is added to global because it allow the support of shared global dispatcher between node and undici.

   It is added #43059

   https://github.com/nodejs/undici/blob/d023b2d808169b7269307d2546125b780e7be5ad/lib/global.js#L5-L27

   👍React with 👍1Reacted by Benjamin Gruenbaum
2. targos commented on May 20, 2022

   

   targos

   on May 20, 2022

   Member

   More actions

   Aren't we supposed to detect leaked globals in the Node.js CI?
3. mscdex commented on May 20, 2022

   

   mscdex

   on May 20, 2022

   Contributor

   More actions

   Aren't we supposed to detect leaked globals in the Node.js CI?

   We probably only test for actual globals and not global symbols. Does V8 even provide an API to get a list of global/shared symbols (created via Symbol.for())?
4. Trott commented on May 20, 2022

   

   Trott

   on May 20, 2022

   MemberAuthor

   More actions

   Aren't we supposed to detect leaked globals in the Node.js CI?

   The check is in test/common/index.js. I'm guessing that it would miss a symbol, though.

   node/test/common/index.js

   Lines 266 to 368 in 26846a0

   	let knownGlobals = [
   	atob,
   	btoa,
   	clearImmediate,
   	clearInterval,
   	clearTimeout,
   	global,
   	setImmediate,
   	setInterval,
   	setTimeout,
   	queueMicrotask,
   	];
   	// TODO(@jasnell): This check can be temporary. AbortController is
   	// not currently supported in either Node.js 12 or 10, making it
   	// difficult to run tests comparatively on those versions. Once
   	// all supported versions have AbortController as a global, this
   	// check can be removed and AbortController can be added to the
   	// knownGlobals list above.
   	if (global.AbortController)
   	knownGlobals.push(global.AbortController);
   	if (global.gc) {
   	knownGlobals.push(global.gc);
   	}
   	if (global.performance) {
   	knownGlobals.push(global.performance);
   	}
   	if (global.PerformanceMark) {
   	knownGlobals.push(global.PerformanceMark);
   	}
   	if (global.PerformanceMeasure) {
   	knownGlobals.push(global.PerformanceMeasure);
   	}
   	// TODO(@ethan-arrowood): Similar to previous checks, this can be temporary
   	// until v16.x is EOL. Once all supported versions have structuredClone we
   	// can add this to the list above instead.
   	if (global.structuredClone) {
   	knownGlobals.push(global.structuredClone);
   	}
   	if (global.fetch) {
   	knownGlobals.push(fetch);
   	}
   	if (hasCrypto && global.crypto) {
   	knownGlobals.push(global.crypto);
   	knownGlobals.push(global.Crypto);
   	knownGlobals.push(global.CryptoKey);
   	knownGlobals.push(global.SubtleCrypto);
   	}
   	if (global.ReadableStream) {
   	knownGlobals.push(
   	global.ReadableStream,
   	global.ReadableStreamDefaultReader,
   	global.ReadableStreamBYOBReader,
   	global.ReadableStreamBYOBRequest,
   	global.ReadableByteStreamController,
   	global.ReadableStreamDefaultController,
   	global.TransformStream,
   	global.TransformStreamDefaultController,
   	global.WritableStream,
   	global.WritableStreamDefaultWriter,
   	global.WritableStreamDefaultController,
   	global.ByteLengthQueuingStrategy,
   	global.CountQueuingStrategy,
   	global.TextEncoderStream,
   	global.TextDecoderStream,
   	global.CompressionStream,
   	global.DecompressionStream,
   	);
   	}
   	function allowGlobals(...allowlist) {
   	knownGlobals = knownGlobals.concat(allowlist);
   	}
   	if (process.env.NODE_TEST_KNOWN_GLOBALS !== '0') {
   	if (process.env.NODE_TEST_KNOWN_GLOBALS) {
   	const knownFromEnv = process.env.NODE_TEST_KNOWN_GLOBALS.split(',');
   	allowGlobals(...knownFromEnv);
   	}
   	function leakedGlobals() {
   	const leaked = [];
   	for (const val in global) {
   	if (!knownGlobals.includes(global[val])) {
   	leaked.push(val);
   	}
   	}
   	return leaked;
   	}
   	process.on('exit', function() {
   	const leaked = leakedGlobals();
   	if (leaked.length > 0) {
   	assert.fail(`Unexpected global(s) found: ${leaked.join(', ')}`);
   	}
   	});
   	}
5. aduh95 commented on May 21, 2022

   

   aduh95

   on May 21, 2022

   Contributor

   More actions

   Aren't we supposed to detect leaked globals in the Node.js CI?

   We currently only test for enumerable globals, for some reason. I've #42056 open that should address this, but it's still blocked on nodejs/build#2879.
6. bnoordhuis commented on Aug 1, 2023

   

   bnoordhuis

   on Aug 1, 2023

   Member

   More actions

   As the undici.globalDispatcher.1 symbol is used by people now (we mention it as a workaround in a couple of places in the bug tracker) I don't think we can - or want to - remove it again, at least not until #43187 is done, and even then a grace period is needed.

   I'll go ahead and close this but please reopen if needed.
7. bnoordhuis

   closed this as not plannedon Aug 1, 2023