From fae4c6d146d7339b39e17fc78adf10a165caf78a Mon Sep 17 00:00:00 2001 From: modeco80 Date: Fri, 30 Aug 2024 20:30:17 -0400 Subject: [PATCH 01/20] cvmts: Fix WebSocket errors causing process crashes --- cvmts/src/WebSocket/WSClient.ts | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/cvmts/src/WebSocket/WSClient.ts b/cvmts/src/WebSocket/WSClient.ts index c2b0cbf..3018ace 100644 --- a/cvmts/src/WebSocket/WSClient.ts +++ b/cvmts/src/WebSocket/WSClient.ts @@ -1,10 +1,12 @@ import { WebSocket } from 'ws'; import NetworkClient from '../NetworkClient.js'; import EventEmitter from 'events'; +import pino from 'pino'; export default class WSClient extends EventEmitter implements NetworkClient { socket: WebSocket; ip: string; + private logger = pino({ name: "CVMTS.WebsocketClient" }); constructor(ws: WebSocket, ip: string) { super(); @@ -20,6 +22,10 @@ export default class WSClient extends EventEmitter implements NetworkClient { this.emit('msg', buf.toString('utf-8')); }); + this.socket.on('error', (err: Error) => { + this.logger.error(err, 'WebSocket recv error'); + }) + this.socket.on('close', () => { this.emit('disconnect'); }); @@ -35,11 +41,13 @@ export default class WSClient extends EventEmitter implements NetworkClient { send(msg: string): Promise { return new Promise((res, rej) => { - if (!this.isOpen()) res(); + if (!this.isOpen()) return res(); this.socket.send(msg, (err) => { if (err) { - rej(err); + this.logger.error(err, 'WebSocket send error'); + this.close(); + res(); return; } res(); @@ -49,11 +57,13 @@ export default class WSClient extends EventEmitter implements NetworkClient { sendBinary(msg: Uint8Array): Promise { return new Promise((res, rej) => { - if (!this.isOpen()) res(); + if (!this.isOpen()) return res(); this.socket.send(msg, (err) => { if (err) { - rej(err); + this.logger.error(err, 'WebSocket send error'); + this.close(); + res(); return; } res(); From 689be9d395323883f16fa2a50b2eb481d37c4935 Mon Sep 17 00:00:00 2001 From: modeco80 Date: Thu, 5 Sep 2024 04:15:19 -0400 Subject: [PATCH 02/20] cvmts: Explicitly disable ws PMD/tracking Seems to fix or at least make a pretty bad memory leak much slower. I hate ws but the only other library is written by someone who isn't a very nice person (putting it on the nice side) --- cvmts/src/WebSocket/WSServer.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cvmts/src/WebSocket/WSServer.ts b/cvmts/src/WebSocket/WSServer.ts index 82e7a24..1b1ffe4 100644 --- a/cvmts/src/WebSocket/WSServer.ts +++ b/cvmts/src/WebSocket/WSServer.ts @@ -24,7 +24,7 @@ export default class WSServer extends EventEmitter implements NetworkServer { this.Config = config; this.clients = []; this.httpServer = http.createServer(); - this.wsServer = new WebSocketServer({ noServer: true }); + this.wsServer = new WebSocketServer({ noServer: true, perMessageDeflate: false, clientTracking: false }); this.httpServer.on('upgrade', (req: http.IncomingMessage, socket: internal.Duplex, head: Buffer) => this.httpOnUpgrade(req, socket, head)); this.httpServer.on('request', (req, res) => { res.writeHead(426); From 9d57779c7500ca2775edcdfa6504a1a048e22fd3 Mon Sep 17 00:00:00 2001 From: modeco80 Date: Mon, 9 Sep 2024 22:39:01 -0400 Subject: [PATCH 03/20] fix config error logging so pino actually logs the Error object --- cvmts/src/index.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cvmts/src/index.ts b/cvmts/src/index.ts index 147bf1a..47c8c51 100644 --- a/cvmts/src/index.ts +++ b/cvmts/src/index.ts @@ -33,7 +33,7 @@ try { var configRaw = fs.readFileSync('config.toml').toString(); Config = toml.parse(configRaw); } catch (e) { - logger.error('Fatal error: Failed to read or parse the config file: {0}', (e as Error).message); + logger.error({err: e}, 'Fatal error: Failed to read or parse the config file'); process.exit(1); } From 4fdd209c8725ff2e39d52a5419d384febbbd3c29 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Thu, 12 Sep 2024 05:23:11 +0000 Subject: [PATCH 04/20] build(deps): bump micromatch from 4.0.7 to 4.0.8 Bumps [micromatch](https://github.com/micromatch/micromatch) from 4.0.7 to 4.0.8. - [Release notes](https://github.com/micromatch/micromatch/releases) - [Changelog](https://github.com/micromatch/micromatch/blob/master/CHANGELOG.md) - [Commits](https://github.com/micromatch/micromatch/compare/4.0.7...4.0.8) --- updated-dependencies: - dependency-name: micromatch dependency-type: indirect ... Signed-off-by: dependabot[bot] --- yarn.lock | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/yarn.lock b/yarn.lock index ded4934..4b65a91 100644 --- a/yarn.lock +++ b/yarn.lock @@ -2944,12 +2944,12 @@ __metadata: linkType: hard "micromatch@npm:^4.0.5": - version: 4.0.7 - resolution: "micromatch@npm:4.0.7" + version: 4.0.8 + resolution: "micromatch@npm:4.0.8" dependencies: braces: "npm:^3.0.3" picomatch: "npm:^2.3.1" - checksum: 10c0/58fa99bc5265edec206e9163a1d2cec5fabc46a5b473c45f4a700adce88c2520456ae35f2b301e4410fb3afb27e9521fb2813f6fc96be0a48a89430e0916a772 + checksum: 10c0/166fa6eb926b9553f32ef81f5f531d27b4ce7da60e5baf8c021d043b27a388fb95e46a8038d5045877881e673f8134122b59624d5cecbd16eb50a42e7a6b5ca8 languageName: node linkType: hard From 210e36f43085e10cd8e0f26cadfcf7807a97088a Mon Sep 17 00:00:00 2001 From: modeco80 Date: Thu, 12 Sep 2024 13:11:12 -0400 Subject: [PATCH 05/20] cvmts: fix memory leak javascript blows chunks --- cvmts/src/CollabVMServer.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/cvmts/src/CollabVMServer.ts b/cvmts/src/CollabVMServer.ts index d9ec4e8..bb6ce1a 100644 --- a/cvmts/src/CollabVMServer.ts +++ b/cvmts/src/CollabVMServer.ts @@ -890,7 +890,9 @@ export default class CollabVMServer { for (let rect of self.rectQueue) promises.push(doRect(rect)); - this.rectQueue = []; + // javascript is a very solidly designed language with no holes + // or usability traps inside of it whatsoever + this.rectQueue.length = 0; await Promise.all(promises); } From 072fd0691882fafd1bd68340ad1e4f0833f4b8c8 Mon Sep 17 00:00:00 2001 From: modeco80 Date: Thu, 19 Sep 2024 04:11:25 -0400 Subject: [PATCH 06/20] cvmts: fix display event handler duplication By only adding event handlers when the display is first lazily initalized. --- cvmts/src/CollabVMServer.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/cvmts/src/CollabVMServer.ts b/cvmts/src/CollabVMServer.ts index bb6ce1a..3dccc75 100644 --- a/cvmts/src/CollabVMServer.ts +++ b/cvmts/src/CollabVMServer.ts @@ -129,17 +129,17 @@ export default class CollabVMServer { if (newState == VMState.Started) { self.logger.info('VM started'); - // start the display + // start the display and add the events once if (self.VM.GetDisplay() == null) { self.VM.StartDisplay(); - } - self.VM.GetDisplay()?.on('connected', () => { - // well aware this sucks but whatever + self.logger.info('started display, adding events now'); + + // add events self.VM.GetDisplay()?.on('resize', (size: Size) => self.OnDisplayResized(size)); self.VM.GetDisplay()?.on('rect', (rect: Rect) => self.OnDisplayRectangle(rect)); self.VM.GetDisplay()?.on('frame', () => self.OnDisplayFrame()); - }); + } } if (newState == VMState.Stopped) { From 41ee71f0538196fd3c6ee53d2b77ddaf06b1d9d1 Mon Sep 17 00:00:00 2001 From: modeco80 Date: Sat, 21 Sep 2024 21:14:27 -0400 Subject: [PATCH 07/20] cvmts: Add staff audit logging support Basically what it says on the tin. More staff operations should probably be audited, but for now this provides a good starting point. --- .gitignore | 5 ++- cvmts/src/AuditLog.ts | 62 +++++++++++++++++++++++++++++++++++++ cvmts/src/CollabVMServer.ts | 8 ++++- 3 files changed, 73 insertions(+), 2 deletions(-) create mode 100644 cvmts/src/AuditLog.ts diff --git a/.gitignore b/.gitignore index 6fbbf6a..ddd9804 100644 --- a/.gitignore +++ b/.gitignore @@ -14,4 +14,7 @@ cvm-rs/target cvm-rs/index.node # geolite shit -**/geoip/ \ No newline at end of file +**/geoip/ + +# staff audit log +audit.log diff --git a/cvmts/src/AuditLog.ts b/cvmts/src/AuditLog.ts new file mode 100644 index 0000000..315e8ae --- /dev/null +++ b/cvmts/src/AuditLog.ts @@ -0,0 +1,62 @@ +import pino from 'pino'; +import { Rank, User } from './User.js'; + +// Staff audit log. +// TODO: +// - Hook this up to a db or something instead of misusing pino +export class AuditLog { + private auditLogger = pino({ + name: 'AuditLog', + transport: { + target: 'pino/file', + options: { + destination: './audit.log' + } + } + }); + + private static StaffHonorFromRank(user: User, uppercase: boolean) { + switch (user.rank) { + case Rank.Moderator: + if (uppercase) return 'Moderator'; + else return 'moderator'; + + case Rank.Admin: + if (uppercase) return 'Administrator'; + else return 'administrator'; + + default: + throw new Error("input user is not staff.. how'd you even get here?"); + } + } + + onReset(callingUser: User) { + this.auditLogger.info({ staffUsername: callingUser.username }, `${AuditLog.StaffHonorFromRank(callingUser, true)} reset the virtual machine.`); + } + + onReboot(callingUser: User) { + this.auditLogger.info({ staffUsername: callingUser.username }, `${AuditLog.StaffHonorFromRank(callingUser, true)} rebooted the virtual machine.`); + } + + onMute(callingUser: User, target: User, perm: boolean) { + this.auditLogger.info({ staffUsername: callingUser.username, targetUsername: target.username, perm: perm }, `${AuditLog.StaffHonorFromRank(callingUser, true)} muted user.`); + } + + onUnmute(callingUser: User, target: User) { + this.auditLogger.info({ staffUsername: callingUser.username, targetUsername: target.username }, `${AuditLog.StaffHonorFromRank(callingUser, true)} unmuted user.`); + } + + onKick(callingUser: User, target: User) { + this.auditLogger.info({ staffUsername: callingUser.username, targetUsername: target.username }, `${AuditLog.StaffHonorFromRank(callingUser, true)} kicked user.`); + } + + onBan(callingUser: User, target: User) { + this.auditLogger.info({ staffUsername: callingUser.username, targetUsername: target.username }, `${AuditLog.StaffHonorFromRank(callingUser, true)} banned user.`); + } + + onMonitorCommand(callingUser: User, command: string) { + this.auditLogger.info({ staffUsername: callingUser.username, commandLine: command }, `${AuditLog.StaffHonorFromRank(callingUser, true)} executed monitor command.`); + } +} + +export let TheAuditLog = new AuditLog(); diff --git a/cvmts/src/CollabVMServer.ts b/cvmts/src/CollabVMServer.ts index 3dccc75..cd8d14d 100644 --- a/cvmts/src/CollabVMServer.ts +++ b/cvmts/src/CollabVMServer.ts @@ -20,6 +20,7 @@ import { CollabVMProtocolMessage, CollabVMProtocolMessageType } from '@cvmts/col import { Size, Rect } from './Utilities.js'; import pino from 'pino'; import { BanManager } from './BanManager.js'; +import { TheAuditLog } from './AuditLog.js'; // Instead of strange hacks we can just use nodejs provided // import.meta properties, which have existed since LTS if not before @@ -526,18 +527,21 @@ export default class CollabVMServer { // QEMU Monitor if (client.rank !== Rank.Admin) return; if (msgArr.length !== 4 || msgArr[2] !== this.Config.collabvm.node) return; + TheAuditLog.onMonitorCommand(client, msgArr[3]); let output = await this.VM.MonitorCommand(msgArr[3]); client.sendMsg(cvm.guacEncode('admin', '2', String(output))); break; case '8': // Restore if (client.rank !== Rank.Admin && (client.rank !== Rank.Moderator || !this.Config.collabvm.moderatorPermissions.restore)) return; + TheAuditLog.onReset(client); this.VM.Reset(); break; case '10': // Reboot if (client.rank !== Rank.Admin && (client.rank !== Rank.Moderator || !this.Config.collabvm.moderatorPermissions.reboot)) return; if (msgArr.length !== 3 || msgArr[2] !== this.Config.collabvm.node) return; + TheAuditLog.onReboot(client); await this.VM.Reboot(); break; case '12': @@ -545,7 +549,7 @@ export default class CollabVMServer { if (client.rank !== Rank.Admin && (client.rank !== Rank.Moderator || !this.Config.collabvm.moderatorPermissions.ban)) return; var user = this.clients.find((c) => c.username === msgArr[2]); if (!user) return; - this.logger.info(`Banning ${user.username!} (${user.IP.address}) by request of ${client.username!}`); + TheAuditLog.onBan(client, user); user.ban(this.banmgr); case '13': // Force Vote @@ -578,6 +582,7 @@ export default class CollabVMServer { default: return; } + //TheAdminLogger.onMute(client, user, permamute); user.mute(permamute); break; case '15': @@ -585,6 +590,7 @@ export default class CollabVMServer { if (client.rank !== Rank.Admin && (client.rank !== Rank.Moderator || !this.Config.collabvm.moderatorPermissions.kick)) return; var user = this.clients.find((c) => c.username === msgArr[2]); if (!user) return; + TheAuditLog.onKick(client, user); user.kick(); break; case '16': From 199924ff92cce93a55a111b06ab8a9991fd78d15 Mon Sep 17 00:00:00 2001 From: modeco80 Date: Sat, 5 Oct 2024 05:00:52 -0400 Subject: [PATCH 08/20] cvmts: rate limit the 'sync' instruction the original Guacamole code did this too I'm pretty sure, and it was even more aggressive about it. --- cvmts/src/CollabVMServer.ts | 53 ++++++++++++++++++++----------------- 1 file changed, 28 insertions(+), 25 deletions(-) diff --git a/cvmts/src/CollabVMServer.ts b/cvmts/src/CollabVMServer.ts index cd8d14d..1a37704 100644 --- a/cvmts/src/CollabVMServer.ts +++ b/cvmts/src/CollabVMServer.ts @@ -30,6 +30,9 @@ const kCVMTSAssetsRoot = path.resolve(__dirname, '../../assets'); const kRestartTimeout = 5000; +// Rate in milliseconds that the 'sync' instruction should be sent to users. +const kSyncRateMs = 500; + type ChatHistory = { user: string; msg: string; @@ -85,6 +88,8 @@ export default class CollabVMServer { private ModPerms: number; private VM: VM; + private lastSync: number = Date.now(); + // Authentication manager private auth: AuthManager | null; @@ -94,9 +99,6 @@ export default class CollabVMServer { // Ban manager private banmgr: BanManager; - // queue of rects, reset every frame - private rectQueue: Rect[] = []; - private logger = pino({ name: 'CVMTS.Server' }); constructor(config: IConfig, vm: VM, banmgr: BanManager, auth: AuthManager | null, geoipReader: ReaderModel | null) { @@ -139,7 +141,6 @@ export default class CollabVMServer { // add events self.VM.GetDisplay()?.on('resize', (size: Size) => self.OnDisplayResized(size)); self.VM.GetDisplay()?.on('rect', (rect: Rect) => self.OnDisplayRectangle(rect)); - self.VM.GetDisplay()?.on('frame', () => self.OnDisplayFrame()); } } @@ -849,20 +850,7 @@ export default class CollabVMServer { } } - private OnDisplayRectangle(rect: Rect) { - this.rectQueue.push(rect); - } - - private OnDisplayResized(size: Size) { - this.clients - .filter((c) => c.connectedToNode || c.viewMode == 1) - .forEach((c) => { - if (this.screenHidden && c.rank == Rank.Unregistered) return; - c.sendMsg(cvm.guacEncode('size', '0', size.width.toString(), size.height.toString())); - }); - } - - private async OnDisplayFrame() { + private async OnDisplayRectangle(rect: Rect) { let self = this; let doRect = async (rect: Rect) => { @@ -887,20 +875,35 @@ export default class CollabVMServer { c.socket.sendBinary(encodedbin); } else { c.sendMsg(cvm.guacEncode('png', '0', '0', rect.x.toString(), rect.y.toString(), encodedb64)); - c.sendMsg(cvm.guacEncode('sync', Date.now().toString())); } }); }; - let promises: Promise[] = []; + await Promise.all([doRect(rect)]); - for (let rect of self.rectQueue) promises.push(doRect(rect)); + // Send a sync if the time since the last sync has hit or went above the rate + // to send one + let syncDeltaMs = Date.now() - self.lastSync; + if (syncDeltaMs >= kSyncRateMs) { + let syncNow = Date.now(); + self.clients + .filter((c) => c.connectedToNode || c.viewMode == 1) + .forEach((c) => { + if (self.screenHidden && c.rank == Rank.Unregistered) return; + c.sendMsg(cvm.guacEncode('sync', syncNow.toString())); + }); - // javascript is a very solidly designed language with no holes - // or usability traps inside of it whatsoever - this.rectQueue.length = 0; + self.lastSync = syncNow; + } + } - await Promise.all(promises); + private OnDisplayResized(size: Size) { + this.clients + .filter((c) => c.connectedToNode || c.viewMode == 1) + .forEach((c) => { + if (this.screenHidden && c.rank == Rank.Unregistered) return; + c.sendMsg(cvm.guacEncode('size', '0', size.width.toString(), size.height.toString())); + }); } private async SendFullScreenWithSize(client: User) { From 25ed0515dd7870db12586798e48ac22666ed6102 Mon Sep 17 00:00:00 2001 From: modeco80 Date: Sat, 5 Oct 2024 05:11:02 -0400 Subject: [PATCH 09/20] Revert "cvmts: rate limit the 'sync' instruction" This reverts commit 199924ff92cce93a55a111b06ab8a9991fd78d15. nvm the decade old shitcode webapp that people seem to still flock to for NO reason breaks honestly why do we even support that hunk of trash it's a decade old, falling apart, and all it does is stifle progress if anything --- cvmts/src/CollabVMServer.ts | 53 +++++++++++++++++-------------------- 1 file changed, 25 insertions(+), 28 deletions(-) diff --git a/cvmts/src/CollabVMServer.ts b/cvmts/src/CollabVMServer.ts index 1a37704..cd8d14d 100644 --- a/cvmts/src/CollabVMServer.ts +++ b/cvmts/src/CollabVMServer.ts @@ -30,9 +30,6 @@ const kCVMTSAssetsRoot = path.resolve(__dirname, '../../assets'); const kRestartTimeout = 5000; -// Rate in milliseconds that the 'sync' instruction should be sent to users. -const kSyncRateMs = 500; - type ChatHistory = { user: string; msg: string; @@ -88,8 +85,6 @@ export default class CollabVMServer { private ModPerms: number; private VM: VM; - private lastSync: number = Date.now(); - // Authentication manager private auth: AuthManager | null; @@ -99,6 +94,9 @@ export default class CollabVMServer { // Ban manager private banmgr: BanManager; + // queue of rects, reset every frame + private rectQueue: Rect[] = []; + private logger = pino({ name: 'CVMTS.Server' }); constructor(config: IConfig, vm: VM, banmgr: BanManager, auth: AuthManager | null, geoipReader: ReaderModel | null) { @@ -141,6 +139,7 @@ export default class CollabVMServer { // add events self.VM.GetDisplay()?.on('resize', (size: Size) => self.OnDisplayResized(size)); self.VM.GetDisplay()?.on('rect', (rect: Rect) => self.OnDisplayRectangle(rect)); + self.VM.GetDisplay()?.on('frame', () => self.OnDisplayFrame()); } } @@ -850,7 +849,20 @@ export default class CollabVMServer { } } - private async OnDisplayRectangle(rect: Rect) { + private OnDisplayRectangle(rect: Rect) { + this.rectQueue.push(rect); + } + + private OnDisplayResized(size: Size) { + this.clients + .filter((c) => c.connectedToNode || c.viewMode == 1) + .forEach((c) => { + if (this.screenHidden && c.rank == Rank.Unregistered) return; + c.sendMsg(cvm.guacEncode('size', '0', size.width.toString(), size.height.toString())); + }); + } + + private async OnDisplayFrame() { let self = this; let doRect = async (rect: Rect) => { @@ -875,35 +887,20 @@ export default class CollabVMServer { c.socket.sendBinary(encodedbin); } else { c.sendMsg(cvm.guacEncode('png', '0', '0', rect.x.toString(), rect.y.toString(), encodedb64)); + c.sendMsg(cvm.guacEncode('sync', Date.now().toString())); } }); }; - await Promise.all([doRect(rect)]); + let promises: Promise[] = []; - // Send a sync if the time since the last sync has hit or went above the rate - // to send one - let syncDeltaMs = Date.now() - self.lastSync; - if (syncDeltaMs >= kSyncRateMs) { - let syncNow = Date.now(); - self.clients - .filter((c) => c.connectedToNode || c.viewMode == 1) - .forEach((c) => { - if (self.screenHidden && c.rank == Rank.Unregistered) return; - c.sendMsg(cvm.guacEncode('sync', syncNow.toString())); - }); + for (let rect of self.rectQueue) promises.push(doRect(rect)); - self.lastSync = syncNow; - } - } + // javascript is a very solidly designed language with no holes + // or usability traps inside of it whatsoever + this.rectQueue.length = 0; - private OnDisplayResized(size: Size) { - this.clients - .filter((c) => c.connectedToNode || c.viewMode == 1) - .forEach((c) => { - if (this.screenHidden && c.rank == Rank.Unregistered) return; - c.sendMsg(cvm.guacEncode('size', '0', size.width.toString(), size.height.toString())); - }); + await Promise.all(promises); } private async SendFullScreenWithSize(client: User) { From bbc873a113811abaf44ca68a36bf514e6b89ca7d Mon Sep 17 00:00:00 2001 From: modeco80 Date: Sat, 2 Nov 2024 03:11:07 -0400 Subject: [PATCH 10/20] cvmts: Bump superqemu to 0.3.0 --- cvmts/package.json | 2 +- yarn.lock | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/cvmts/package.json b/cvmts/package.json index aaa879b..d04afe3 100644 --- a/cvmts/package.json +++ b/cvmts/package.json @@ -13,7 +13,7 @@ "license": "GPL-3.0", "dependencies": { "@computernewb/nodejs-rfb": "^0.3.0", - "@computernewb/superqemu": "0.2.4", + "@computernewb/superqemu": "0.3.0", "@cvmts/cvm-rs": "*", "@maxmind/geoip2-node": "^5.0.0", "execa": "^8.0.1", diff --git a/yarn.lock b/yarn.lock index ded4934..8c173d4 100644 --- a/yarn.lock +++ b/yarn.lock @@ -41,13 +41,13 @@ __metadata: languageName: node linkType: hard -"@computernewb/superqemu@npm:0.2.4": - version: 0.2.4 - resolution: "@computernewb/superqemu@npm:0.2.4" +"@computernewb/superqemu@npm:0.3.0": + version: 0.3.0 + resolution: "@computernewb/superqemu@npm:0.3.0" dependencies: execa: "npm:^8.0.1" pino: "npm:^9.3.1" - checksum: 10c0/9ed3190bd95c60a6f74fbb6d29cbd9909ff18b04d64b5a09c02dec91169304f439d7b0ac91848b69621066810cdfef4a0dbf97075938ee40b3aebd74376b4440 + checksum: 10c0/232a83b3061bddcdf0fcef56a289f1d22cfdb70cc333ae2422a9246760310ac432b28a54e5d9d8bbcd5b87af8142d24c3fcaa9e3ce5e196bae7cf11b4538e8cc languageName: node linkType: hard @@ -75,7 +75,7 @@ __metadata: resolution: "@cvmts/cvmts@workspace:cvmts" dependencies: "@computernewb/nodejs-rfb": "npm:^0.3.0" - "@computernewb/superqemu": "npm:0.2.4" + "@computernewb/superqemu": "npm:0.3.0" "@cvmts/cvm-rs": "npm:*" "@maxmind/geoip2-node": "npm:^5.0.0" "@types/node": "npm:^20.12.5" From e780ecabf04d7e6053267ba1821cf83debbec548 Mon Sep 17 00:00:00 2001 From: modeco80 Date: Sat, 2 Nov 2024 06:08:26 -0400 Subject: [PATCH 11/20] cvmts: Add support for cgroup process resource limits on Linux Using systemd's `Delegate=` option, it is possible to get it to let you manage your own cgroup subtree, therefore allowing you to set options and other fun stuff. This commit adds support for doing so and configuring the resource limits in config.toml. For later: The cgroup created has to be a threaded one. Iin theory, we can actually wait for the QEMU process to handshake qmp, grab the vCPU threads, and only limit those. For now, just limiting the entire QEMU process works, though and is the least complicated. NOTE: Windows support should still work, even if you have resource limits configured. If you do, it should only warn and complain, but still function. --- config.example.toml | 11 ++++ cvmts/src/IConfig.ts | 2 + cvmts/src/index.ts | 2 +- cvmts/src/util/cgroup.ts | 87 ++++++++++++++++++++++++++++ cvmts/src/vm/qemu.ts | 19 ++++++- cvmts/src/vm/qemu_launcher.ts | 104 ++++++++++++++++++++++++++++++++++ 6 files changed, 222 insertions(+), 3 deletions(-) create mode 100644 cvmts/src/util/cgroup.ts create mode 100644 cvmts/src/vm/qemu_launcher.ts diff --git a/config.example.toml b/config.example.toml index d0df6f8..0a57363 100644 --- a/config.example.toml +++ b/config.example.toml @@ -57,6 +57,17 @@ qemuArgs = "qemu-system-x86_64" vncPort = 5900 snapshots = true +# Resource limits. Only works on Linux, with `Delegate=yes` set in your .service file. +# +# cpuUsageMax specifies CPU usage limits in the common top notation, so 200% means 2 CPUs, so on so forth. +# runOnCpus specifies what CPUs the VM is allowed to run on. +# +# Either can be omitted or specified; however, if you want to disable it entirely, +# it's a better idea to just comment this inline table out, +# since the inline table existing is used to enable cgroup support. +resourceLimits = { cpuUsageMax = 100, runOnCpus = [ 2, 4 ] } + + # VNC options # Only used if vm.type is set to "vncvm" [vncvm] diff --git a/cvmts/src/IConfig.ts b/cvmts/src/IConfig.ts index 2217718..667c509 100644 --- a/cvmts/src/IConfig.ts +++ b/cvmts/src/IConfig.ts @@ -1,3 +1,4 @@ +import { CgroupLimits } from './vm/qemu_launcher'; import VNCVMDef from './vm/vnc/VNCVMDef'; export default interface IConfig { @@ -38,6 +39,7 @@ export default interface IConfig { qemuArgs: string; vncPort: number; snapshots: boolean; + resourceLimits?: CgroupLimits }; vncvm: VNCVMDef; mysql: MySQLConfig; diff --git a/cvmts/src/index.ts b/cvmts/src/index.ts index 47c8c51..720ec80 100644 --- a/cvmts/src/index.ts +++ b/cvmts/src/index.ts @@ -81,7 +81,7 @@ async function start() { vncPort: Config.qemu.vncPort, }; - VM = new QemuVMShim(def); + VM = new QemuVMShim(def, Config.qemu.resourceLimits); break; } case 'vncvm': { diff --git a/cvmts/src/util/cgroup.ts b/cvmts/src/util/cgroup.ts new file mode 100644 index 0000000..e6d1eba --- /dev/null +++ b/cvmts/src/util/cgroup.ts @@ -0,0 +1,87 @@ +// Cgroup management code +// this sucks, ill mess with it later + +import { appendFileSync, existsSync, mkdirSync, readFileSync, writeFileSync } from 'node:fs'; +import path from 'node:path'; + +export class CGroupController { + private controller; + private cg: CGroup; + + constructor(controller: string, cg: CGroup) { + this.controller = controller; + this.cg = cg; + } + + WriteValue(key: string, value: string) { + writeFileSync(path.join(this.cg.Path(), `${this.controller}.${key}`), value); + } +} + +export class CGroup { + private path; + + constructor(path: string) { + this.path = path; + } + + private InitControllers() { + // Configure this "root" cgroup to provide cpu and cpuset controllers to the leaf + // QEMU cgroups. A bit iffy but whatever. + writeFileSync(path.join(this.path, 'cgroup.subtree_control'), '+cpu +cpuset'); + } + + GetController(controller: string) { + return new CGroupController(controller, this); + } + + Path(): string { + return this.path; + } + + HasSubgroup(name: string): boolean { + let subgroup_root = path.join(this.path, name); + if (existsSync(subgroup_root)) return true; + return false; + } + + // Gets a CGroup inside of this cgroup. + GetSubgroup(name: string): CGroup { + // make the subgroup if it doesn't already exist + let subgroup_root = path.join(this.path, name); + if (!this.HasSubgroup(name)) { + mkdirSync(subgroup_root); + // We need to make the subgroup threaded before we can attach a process to it. + // It's a bit weird, but oh well. Blame linux people, not me. + writeFileSync(path.join(subgroup_root, 'cgroup.type'), 'threaded'); + } + return new CGroup(subgroup_root); + } + + // Attaches a process to this cgroup. + AttachProcess(pid: number) { + appendFileSync(path.join(this.path, 'cgroup.procs'), pid.toString()); + } + + // Returns a CGroup instance for the process' current cgroup, prepared for subgroup usage. + // This will only fail if you are not using systemd or elogind, + // since even logind user sessions are run inside of a user@[UID] slice. + // NOTE: This only supports cgroups2-only systems. Systemd practically enforces that so /shrug + static Self(): CGroup { + const kCgroupSelfPath = '/proc/self/cgroup'; + if (!existsSync(kCgroupSelfPath)) throw new Error('This process is not in a CGroup.'); + let res = readFileSync(kCgroupSelfPath, { encoding: 'utf-8' }); + + // Make sure the first/only line is a cgroups2 0::/path/to/cgroup entry. + // Legacy cgroups1 is not supported. + if (res[0] != '0') throw new Error('CGroup.Self() does not work with cgroups 1 systems. Please do not the cgroups 1.'); + let cg_path = res.substring(3, res.indexOf('\n')); + + let cg = new CGroup(path.join('/sys/fs/cgroup', cg_path)); + + // Do root level group initalization + cg.InitControllers(); + + return cg; + } +} diff --git a/cvmts/src/vm/qemu.ts b/cvmts/src/vm/qemu.ts index 38eab09..2d56343 100644 --- a/cvmts/src/vm/qemu.ts +++ b/cvmts/src/vm/qemu.ts @@ -4,6 +4,8 @@ import { QemuVM, QemuVmDefinition, VMState } from '@computernewb/superqemu'; import { VMDisplay } from '../display/interface.js'; import { VncDisplay } from '../display/vnc.js'; import pino from 'pino'; +import { CgroupLimits, QemuResourceLimitedLauncher } from './qemu_launcher.js'; + // shim over superqemu because it diverges from the VM interface export class QemuVMShim implements VM { @@ -11,9 +13,22 @@ export class QemuVMShim implements VM { private display: VncDisplay | null = null; private logger; - constructor(def: QemuVmDefinition) { - this.vm = new QemuVM(def); + constructor(def: QemuVmDefinition, resourceLimits?: CgroupLimits) { this.logger = pino({ name: `CVMTS.QemuVMShim/${def.id}` }); + + if (resourceLimits) { + if (process.platform == 'linux') { + this.vm = new QemuVM(def, new QemuResourceLimitedLauncher(def.id, resourceLimits)); + } else { + // Just use the default Superqemu launcher on non-Linux platforms, + // .. regardless of if resource control is (somehow) enabled. + this.logger.warn({platform: process.platform}, 'Resource control is not supported on this platform. Please remove or comment it out from your configuration.'); + this.vm = new QemuVM(def); + } + } else { + this.vm = new QemuVM(def); + } + } Start(): Promise { diff --git a/cvmts/src/vm/qemu_launcher.ts b/cvmts/src/vm/qemu_launcher.ts new file mode 100644 index 0000000..8e9086b --- /dev/null +++ b/cvmts/src/vm/qemu_launcher.ts @@ -0,0 +1,104 @@ +import EventEmitter from 'events'; +import { IProcess, IProcessLauncher, ProcessLaunchOptions } from '@computernewb/superqemu'; +import { execaCommand } from 'execa'; +import { Readable, Writable } from 'stream'; +import { CGroup } from '../util/cgroup.js'; + +export interface CgroupLimits { + cpuUsageMax?: number; + runOnCpus?: number[]; +} + +interface CGroupValue { + controller: string; + key: string; + value: string; +} + +function MakeValuesFromLimits(limits: CgroupLimits): CGroupValue[] { + let option_array = []; + + if (limits.cpuUsageMax) { + // cpu.max + option_array.push({ + controller: 'cpu', + key: 'max', + value: `${(limits.cpuUsageMax / 100) * 10000} 10000` + }); + } + + if(limits.runOnCpus) { + // Make sure a CPU is not specified more than once. Bit hacky but oh well + let unique = [...new Set(limits.runOnCpus)]; + option_array.push({ + controller: 'cpuset', + key: 'cpus', + value: `${unique.join(',')}` + }); + } + + return option_array; +} + +// A process automatically placed in a given cgroup. +class CGroupLimitedProcess extends EventEmitter implements IProcess { + private process; + stdin: Writable | null = null; + stdout: Readable | null = null; + stderr: Readable | null = null; + private cgroup: CGroup; + + constructor(cg: CGroup, command: string, opts?: ProcessLaunchOptions) { + super(); + this.cgroup = cg; + + this.process = execaCommand(command, opts); + + this.stdin = this.process.stdin; + this.stdout = this.process.stdout; + this.stderr = this.process.stderr; + + let self = this; + this.process.on('spawn', () => { + // it should have one! + self.cgroup.AttachProcess(self.process.pid!); + self.emit('spawn'); + }); + + this.process.on('exit', (code) => { + self.emit('exit', code); + }); + } + + kill(signal?: number | NodeJS.Signals): boolean { + return this.process.kill(signal); + } + + dispose(): void { + this.stdin = null; + this.stdout = null; + this.stderr = null; + + this.process.removeAllListeners(); + this.removeAllListeners(); + } +} + +export class QemuResourceLimitedLauncher implements IProcessLauncher { + private group; + + constructor(name: string, limits: CgroupLimits) { + let root = CGroup.Self(); + this.group = root.GetSubgroup(name); + + // Set cgroup keys. + for(const val of MakeValuesFromLimits(limits)) { + let controller = this.group.GetController(val.controller); + controller.WriteValue(val.key, val.value); + } + } + + launch(command: string, opts?: ProcessLaunchOptions | undefined): IProcess { + return new CGroupLimitedProcess(this.group, command, opts); + } +} From 4344b233bc5123bacff12e17377ac22814f7753b Mon Sep 17 00:00:00 2001 From: modeco80 Date: Sat, 2 Nov 2024 06:33:31 -0400 Subject: [PATCH 12/20] cvmts: Bump up cpu.max period to 100000 (well, it DOES still work, but i imagine it's 100000 by default for a reason) --- cvmts/src/vm/qemu_launcher.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cvmts/src/vm/qemu_launcher.ts b/cvmts/src/vm/qemu_launcher.ts index 8e9086b..5ef3aa0 100644 --- a/cvmts/src/vm/qemu_launcher.ts +++ b/cvmts/src/vm/qemu_launcher.ts @@ -23,7 +23,7 @@ function MakeValuesFromLimits(limits: CgroupLimits): CGroupValue[] { option_array.push({ controller: 'cpu', key: 'max', - value: `${(limits.cpuUsageMax / 100) * 10000} 10000` + value: `${(limits.cpuUsageMax / 100) * 100000} 100000` }); } From 86f1345a2dee4cb4235d6cb774a1537298eae30e Mon Sep 17 00:00:00 2001 From: modeco80 Date: Sat, 2 Nov 2024 07:46:59 -0400 Subject: [PATCH 13/20] cvmts: Only target QEMU vCPU threads by default Previous behaviour was to limit the whole QEMU process; this only limits the vCPU threads. A bit (very) hacky how I did this but it does work. --- config.example.toml | 3 +++ cvmts/src/util/cgroup.ts | 6 ++++++ cvmts/src/vm/qemu.ts | 27 +++++++++++++++++++++++---- cvmts/src/vm/qemu_launcher.ts | 20 +++++++++++++++----- 4 files changed, 47 insertions(+), 9 deletions(-) diff --git a/config.example.toml b/config.example.toml index 0a57363..48098d1 100644 --- a/config.example.toml +++ b/config.example.toml @@ -61,6 +61,9 @@ snapshots = true # # cpuUsageMax specifies CPU usage limits in the common top notation, so 200% means 2 CPUs, so on so forth. # runOnCpus specifies what CPUs the VM is allowed to run on. +# limitProcess is optional (default false) and determines if only qemu vCPU threads are put into the cgroup, +# or the entire QEMU process (incl. all its threads). This is rarely what you want, so the example does not +# provide it. # # Either can be omitted or specified; however, if you want to disable it entirely, # it's a better idea to just comment this inline table out, diff --git a/cvmts/src/util/cgroup.ts b/cvmts/src/util/cgroup.ts index e6d1eba..a1514af 100644 --- a/cvmts/src/util/cgroup.ts +++ b/cvmts/src/util/cgroup.ts @@ -29,6 +29,7 @@ export class CGroup { // Configure this "root" cgroup to provide cpu and cpuset controllers to the leaf // QEMU cgroups. A bit iffy but whatever. writeFileSync(path.join(this.path, 'cgroup.subtree_control'), '+cpu +cpuset'); + //writeFileSync(path.join(this.path, 'cgroup.subtree_control'), '+cpu'); } GetController(controller: string) { @@ -63,6 +64,11 @@ export class CGroup { appendFileSync(path.join(this.path, 'cgroup.procs'), pid.toString()); } + // Attaches a thread to this cgroup. (The CGroup is a threaded one. See above) + AttachThread(tid: number) { + appendFileSync(path.join(this.path, 'cgroup.threads'), tid.toString()); + } + // Returns a CGroup instance for the process' current cgroup, prepared for subgroup usage. // This will only fail if you are not using systemd or elogind, // since even logind user sessions are run inside of a user@[UID] slice. diff --git a/cvmts/src/vm/qemu.ts b/cvmts/src/vm/qemu.ts index 2d56343..3e2c4f7 100644 --- a/cvmts/src/vm/qemu.ts +++ b/cvmts/src/vm/qemu.ts @@ -6,29 +6,31 @@ import { VncDisplay } from '../display/vnc.js'; import pino from 'pino'; import { CgroupLimits, QemuResourceLimitedLauncher } from './qemu_launcher.js'; - // shim over superqemu because it diverges from the VM interface export class QemuVMShim implements VM { private vm; private display: VncDisplay | null = null; private logger; + private cg_launcher: QemuResourceLimitedLauncher | null = null; + private resource_limits: CgroupLimits | null = null; constructor(def: QemuVmDefinition, resourceLimits?: CgroupLimits) { this.logger = pino({ name: `CVMTS.QemuVMShim/${def.id}` }); if (resourceLimits) { if (process.platform == 'linux') { - this.vm = new QemuVM(def, new QemuResourceLimitedLauncher(def.id, resourceLimits)); + this.resource_limits = resourceLimits; + this.cg_launcher = new QemuResourceLimitedLauncher(def.id, resourceLimits); + this.vm = new QemuVM(def, this.cg_launcher); } else { // Just use the default Superqemu launcher on non-Linux platforms, // .. regardless of if resource control is (somehow) enabled. - this.logger.warn({platform: process.platform}, 'Resource control is not supported on this platform. Please remove or comment it out from your configuration.'); + this.logger.warn({ platform: process.platform }, 'Resource control is not supported on this platform. Please remove or comment it out from your configuration.'); this.vm = new QemuVM(def); } } else { this.vm = new QemuVM(def); } - } Start(): Promise { @@ -54,7 +56,24 @@ export class QemuVMShim implements VM { return this.vm.MonitorCommand(command); } + async PlaceVCPUThreadsIntoCGroup() { + if (this.cg_launcher) { + if (!this.resource_limits?.limitProcess) { + // Get all vCPUs and pin them to the CGroup. + let cpu_res = await this.vm.QmpCommand('query-cpus-fast', {}); + for (let cpu of cpu_res) { + this.logger.info(`Placing vCPU thread with TID ${cpu['thread-id']} to cgroup`); + this.cg_launcher.group.AttachThread(cpu['thread-id']); + } + } + } + } + StartDisplay(): void { + // HACK: We should probably use another subscribed eventemitter for this. For now, + // this "works". I guess. + this.PlaceVCPUThreadsIntoCGroup(); + // boot it up let info = this.vm.GetDisplayInfo(); diff --git a/cvmts/src/vm/qemu_launcher.ts b/cvmts/src/vm/qemu_launcher.ts index 5ef3aa0..30648ce 100644 --- a/cvmts/src/vm/qemu_launcher.ts +++ b/cvmts/src/vm/qemu_launcher.ts @@ -7,6 +7,7 @@ import { CGroup } from '../util/cgroup.js'; export interface CgroupLimits { cpuUsageMax?: number; runOnCpus?: number[]; + limitProcess?: boolean; } interface CGroupValue { @@ -47,10 +48,15 @@ class CGroupLimitedProcess extends EventEmitter implements IProcess { stdout: Readable | null = null; stderr: Readable | null = null; private cgroup: CGroup; + private limits; - constructor(cg: CGroup, command: string, opts?: ProcessLaunchOptions) { + constructor(cg: CGroup, limits: CgroupLimits, command: string, opts?: ProcessLaunchOptions) { super(); this.cgroup = cg; + this.limits = limits; + + if(!this.limits.limitProcess) + this.limits.limitProcess = false; this.process = execaCommand(command, opts); @@ -60,8 +66,10 @@ class CGroupLimitedProcess extends EventEmitter implements IProcess { let self = this; this.process.on('spawn', () => { - // it should have one! - self.cgroup.AttachProcess(self.process.pid!); + if(self.limits.limitProcess) { + // it should have one! + self.cgroup.AttachProcess(self.process.pid!); + } self.emit('spawn'); }); @@ -85,11 +93,13 @@ class CGroupLimitedProcess extends EventEmitter implements IProcess { } export class QemuResourceLimitedLauncher implements IProcessLauncher { - private group; + public group; + private limits; constructor(name: string, limits: CgroupLimits) { let root = CGroup.Self(); this.group = root.GetSubgroup(name); + this.limits = limits; // Set cgroup keys. for(const val of MakeValuesFromLimits(limits)) { @@ -99,6 +109,6 @@ export class QemuResourceLimitedLauncher implements IProcessLauncher { } launch(command: string, opts?: ProcessLaunchOptions | undefined): IProcess { - return new CGroupLimitedProcess(this.group, command, opts); + return new CGroupLimitedProcess(this.group, this.limits, command, opts); } } From 9fbdb84822469f66adc74177b630b0271e271773 Mon Sep 17 00:00:00 2001 From: modeco80 Date: Sat, 2 Nov 2024 07:54:26 -0400 Subject: [PATCH 14/20] ok fiiiiine i'll do it the non hacky way --- cvmts/src/vm/qemu.ts | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/cvmts/src/vm/qemu.ts b/cvmts/src/vm/qemu.ts index 3e2c4f7..7857e5f 100644 --- a/cvmts/src/vm/qemu.ts +++ b/cvmts/src/vm/qemu.ts @@ -31,6 +31,12 @@ export class QemuVMShim implements VM { } else { this.vm = new QemuVM(def); } + + this.vm.on('statechange', async (newState) => { + if(newState == VMState.Started) { + await this.PlaceVCPUThreadsIntoCGroup(); + } + }); } Start(): Promise { @@ -70,10 +76,6 @@ export class QemuVMShim implements VM { } StartDisplay(): void { - // HACK: We should probably use another subscribed eventemitter for this. For now, - // this "works". I guess. - this.PlaceVCPUThreadsIntoCGroup(); - // boot it up let info = this.vm.GetDisplayInfo(); From a3581854d20c5a1f0b1b79b88520f1576df556f1 Mon Sep 17 00:00:00 2001 From: modeco80 Date: Sat, 2 Nov 2024 08:06:36 -0400 Subject: [PATCH 15/20] had to help this hellfest of a language along slightly --- cvmts/src/vm/qemu.ts | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/cvmts/src/vm/qemu.ts b/cvmts/src/vm/qemu.ts index 7857e5f..a0b4b00 100644 --- a/cvmts/src/vm/qemu.ts +++ b/cvmts/src/vm/qemu.ts @@ -33,7 +33,7 @@ export class QemuVMShim implements VM { } this.vm.on('statechange', async (newState) => { - if(newState == VMState.Started) { + if (newState == VMState.Started) { await this.PlaceVCPUThreadsIntoCGroup(); } }); @@ -63,8 +63,16 @@ export class QemuVMShim implements VM { } async PlaceVCPUThreadsIntoCGroup() { + let pin_vcpu_threads = false; if (this.cg_launcher) { - if (!this.resource_limits?.limitProcess) { + // messy as all hell but oh well + if (this.resource_limits?.limitProcess == undefined) { + pin_vcpu_threads = true; + } else { + pin_vcpu_threads = !this.resource_limits?.limitProcess; + } + + if (pin_vcpu_threads) { // Get all vCPUs and pin them to the CGroup. let cpu_res = await this.vm.QmpCommand('query-cpus-fast', {}); for (let cpu of cpu_res) { From e7a06b714121f08ca3f84c437fb1f8da63f8bdd8 Mon Sep 17 00:00:00 2001 From: modeco80 Date: Sat, 2 Nov 2024 11:58:35 -0400 Subject: [PATCH 16/20] cvmts: Delete cgroup on VM stop Makes clean shutdown with systemd actually work. I've also made superqemu version a SemVer thing so that we don't need to bump it as often, only on a major or minor bump. --- cvmts/package.json | 2 +- cvmts/src/util/cgroup.ts | 12 +++++++++++- cvmts/src/vm/qemu_launcher.ts | 13 ++++++++++--- yarn.lock | 10 +++++----- 4 files changed, 27 insertions(+), 10 deletions(-) diff --git a/cvmts/package.json b/cvmts/package.json index d04afe3..fafeb5d 100644 --- a/cvmts/package.json +++ b/cvmts/package.json @@ -13,7 +13,7 @@ "license": "GPL-3.0", "dependencies": { "@computernewb/nodejs-rfb": "^0.3.0", - "@computernewb/superqemu": "0.3.0", + "@computernewb/superqemu": "^0.3.0", "@cvmts/cvm-rs": "*", "@maxmind/geoip2-node": "^5.0.0", "execa": "^8.0.1", diff --git a/cvmts/src/util/cgroup.ts b/cvmts/src/util/cgroup.ts index a1514af..6f0bb00 100644 --- a/cvmts/src/util/cgroup.ts +++ b/cvmts/src/util/cgroup.ts @@ -1,7 +1,7 @@ // Cgroup management code // this sucks, ill mess with it later -import { appendFileSync, existsSync, mkdirSync, readFileSync, writeFileSync } from 'node:fs'; +import { appendFileSync, existsSync, mkdirSync, readFileSync, rmdirSync, writeFileSync } from 'node:fs'; import path from 'node:path'; export class CGroupController { @@ -46,6 +46,16 @@ export class CGroup { return false; } + DeleteSubgroup(name: string): void { + let subgroup_root = path.join(this.path, name); + if (!this.HasSubgroup(name)) { + throw new Error(`Subgroup ${name} does not exist`); + } + + //console.log("Deleting subgroup", name); + rmdirSync(subgroup_root); + } + // Gets a CGroup inside of this cgroup. GetSubgroup(name: string): CGroup { // make the subgroup if it doesn't already exist diff --git a/cvmts/src/vm/qemu_launcher.ts b/cvmts/src/vm/qemu_launcher.ts index 30648ce..d1c86d6 100644 --- a/cvmts/src/vm/qemu_launcher.ts +++ b/cvmts/src/vm/qemu_launcher.ts @@ -47,12 +47,16 @@ class CGroupLimitedProcess extends EventEmitter implements IProcess { stdin: Writable | null = null; stdout: Readable | null = null; stderr: Readable | null = null; + private cgroup_root: CGroup; private cgroup: CGroup; + private id; private limits; - constructor(cg: CGroup, limits: CgroupLimits, command: string, opts?: ProcessLaunchOptions) { + constructor(cgroup_root: CGroup, id: string, limits: CgroupLimits, command: string, opts?: ProcessLaunchOptions) { super(); - this.cgroup = cg; + this.cgroup_root = cgroup_root; + this.cgroup = cgroup_root.GetSubgroup(id); + this.id = id; this.limits = limits; if(!this.limits.limitProcess) @@ -87,6 +91,7 @@ class CGroupLimitedProcess extends EventEmitter implements IProcess { this.stdout = null; this.stderr = null; + this.cgroup_root.DeleteSubgroup(this.id); this.process.removeAllListeners(); this.removeAllListeners(); } @@ -95,9 +100,11 @@ class CGroupLimitedProcess extends EventEmitter implements IProcess { export class QemuResourceLimitedLauncher implements IProcessLauncher { public group; private limits; + private name; constructor(name: string, limits: CgroupLimits) { let root = CGroup.Self(); + this.name = name; this.group = root.GetSubgroup(name); this.limits = limits; @@ -109,6 +116,6 @@ export class QemuResourceLimitedLauncher implements IProcessLauncher { } launch(command: string, opts?: ProcessLaunchOptions | undefined): IProcess { - return new CGroupLimitedProcess(this.group, this.limits, command, opts); + return new CGroupLimitedProcess(CGroup.Self(), this.name, this.limits, command, opts); } } diff --git a/yarn.lock b/yarn.lock index 8c173d4..a3a15d3 100644 --- a/yarn.lock +++ b/yarn.lock @@ -41,13 +41,13 @@ __metadata: languageName: node linkType: hard -"@computernewb/superqemu@npm:0.3.0": - version: 0.3.0 - resolution: "@computernewb/superqemu@npm:0.3.0" +"@computernewb/superqemu@npm:^0.3.0": + version: 0.3.2 + resolution: "@computernewb/superqemu@npm:0.3.2" dependencies: execa: "npm:^8.0.1" pino: "npm:^9.3.1" - checksum: 10c0/232a83b3061bddcdf0fcef56a289f1d22cfdb70cc333ae2422a9246760310ac432b28a54e5d9d8bbcd5b87af8142d24c3fcaa9e3ce5e196bae7cf11b4538e8cc + checksum: 10c0/845f1732f1e92b19bbf09b4bfc75381e707d367902535b1d520f1dc323e57f97cdf56d37a2d98e79c99443222224276d488d920e34010d199d798da7c564f7d1 languageName: node linkType: hard @@ -75,7 +75,7 @@ __metadata: resolution: "@cvmts/cvmts@workspace:cvmts" dependencies: "@computernewb/nodejs-rfb": "npm:^0.3.0" - "@computernewb/superqemu": "npm:0.3.0" + "@computernewb/superqemu": "npm:^0.3.0" "@cvmts/cvm-rs": "npm:*" "@maxmind/geoip2-node": "npm:^5.0.0" "@types/node": "npm:^20.12.5" From fe82973b9f3e2a53af2183816c39ed225c12a022 Mon Sep 17 00:00:00 2001 From: modeco80 Date: Sat, 2 Nov 2024 18:01:07 -0400 Subject: [PATCH 17/20] Only try enabling cpuset controller if required Additionally, "handle" (in many airquotes) errors trying to set controller values. --- config.example.toml | 19 ++++++++++------- cvmts/src/util/cgroup.ts | 27 +++++++++++++++++------- cvmts/src/vm/qemu_launcher.ts | 39 ++++++++++++++++++++++++----------- 3 files changed, 58 insertions(+), 27 deletions(-) diff --git a/config.example.toml b/config.example.toml index 48098d1..b7d8d48 100644 --- a/config.example.toml +++ b/config.example.toml @@ -57,17 +57,20 @@ qemuArgs = "qemu-system-x86_64" vncPort = 5900 snapshots = true -# Resource limits. Only works on Linux, with `Delegate=yes` set in your .service file. +# Resource limits. Only works on Linux, with `Delegate=yes` set in your .service file. No-op on other platforms. +# +# cpuUsageMax optionally specifies the max CPU usage as percentage in the common top notation, so 200% means 2 CPUs, 400% is 4 CPUs, +# so on. +# +# runOnCpus is an optional array that specifies what CPUs the VM is allowed to run on. +# Systemd user slices can not delegate the cpuset controller, meaning this option will *not* work if you do not use a system service. +# (effectively, it will be a loud no-op that logs an error on startup) # -# cpuUsageMax specifies CPU usage limits in the common top notation, so 200% means 2 CPUs, so on so forth. -# runOnCpus specifies what CPUs the VM is allowed to run on. # limitProcess is optional (default false) and determines if only qemu vCPU threads are put into the cgroup, -# or the entire QEMU process (incl. all its threads). This is rarely what you want, so the example does not -# provide it. +# or the entire QEMU process (incl. all its threads). The default behaviour of only limiting vCPU threads +# is more than likely what you want, so the example configuration omits specifying this key. # -# Either can be omitted or specified; however, if you want to disable it entirely, -# it's a better idea to just comment this inline table out, -# since the inline table existing is used to enable cgroup support. +# Commenting this inline table from the configuration disables resource limiting entirely. resourceLimits = { cpuUsageMax = 100, runOnCpus = [ 2, 4 ] } diff --git a/cvmts/src/util/cgroup.ts b/cvmts/src/util/cgroup.ts index 6f0bb00..44783ce 100644 --- a/cvmts/src/util/cgroup.ts +++ b/cvmts/src/util/cgroup.ts @@ -3,6 +3,9 @@ import { appendFileSync, existsSync, mkdirSync, readFileSync, rmdirSync, writeFileSync } from 'node:fs'; import path from 'node:path'; +import pino from 'pino'; + +let logger = pino({ name: 'CVMTS/CGroup' }); export class CGroupController { private controller; @@ -14,7 +17,11 @@ export class CGroupController { } WriteValue(key: string, value: string) { - writeFileSync(path.join(this.cg.Path(), `${this.controller}.${key}`), value); + try { + writeFileSync(path.join(this.cg.Path(), `${this.controller}.${key}`), value); + } catch(e) { + logger.error({error: e, controller_key: `${this.controller}.${key}`, value: value }, 'Failed to set CGroup controller value') + } } } @@ -25,11 +32,20 @@ export class CGroup { this.path = path; } - private InitControllers() { + InitControllers(wants_cpuset: boolean) { // Configure this "root" cgroup to provide cpu and cpuset controllers to the leaf // QEMU cgroups. A bit iffy but whatever. - writeFileSync(path.join(this.path, 'cgroup.subtree_control'), '+cpu +cpuset'); - //writeFileSync(path.join(this.path, 'cgroup.subtree_control'), '+cpu'); + if(wants_cpuset) { + try { + writeFileSync(path.join(this.path, 'cgroup.subtree_control'), '+cpu +cpuset'); + } catch(err) { + logger.error({error: err}, 'Could not provide cpuset controller to subtree'); + // just give up if this fails + writeFileSync(path.join(this.path, 'cgroup.subtree_control'), '+cpu'); + } + } else { + writeFileSync(path.join(this.path, 'cgroup.subtree_control'), '+cpu'); + } } GetController(controller: string) { @@ -95,9 +111,6 @@ export class CGroup { let cg = new CGroup(path.join('/sys/fs/cgroup', cg_path)); - // Do root level group initalization - cg.InitControllers(); - return cg; } } diff --git a/cvmts/src/vm/qemu_launcher.ts b/cvmts/src/vm/qemu_launcher.ts index d1c86d6..a0ff2d8 100644 --- a/cvmts/src/vm/qemu_launcher.ts +++ b/cvmts/src/vm/qemu_launcher.ts @@ -47,14 +47,14 @@ class CGroupLimitedProcess extends EventEmitter implements IProcess { stdin: Writable | null = null; stdout: Readable | null = null; stderr: Readable | null = null; - private cgroup_root: CGroup; + private root_cgroup: CGroup; private cgroup: CGroup; private id; private limits; constructor(cgroup_root: CGroup, id: string, limits: CgroupLimits, command: string, opts?: ProcessLaunchOptions) { super(); - this.cgroup_root = cgroup_root; + this.root_cgroup = cgroup_root; this.cgroup = cgroup_root.GetSubgroup(id); this.id = id; this.limits = limits; @@ -70,6 +70,8 @@ class CGroupLimitedProcess extends EventEmitter implements IProcess { let self = this; this.process.on('spawn', () => { + self.initCgroup(); + if(self.limits.limitProcess) { // it should have one! self.cgroup.AttachProcess(self.process.pid!); @@ -82,6 +84,14 @@ class CGroupLimitedProcess extends EventEmitter implements IProcess { }); } + initCgroup() { + // Set cgroup keys. + for(const val of MakeValuesFromLimits(this.limits)) { + let controller = this.cgroup.GetController(val.controller); + controller.WriteValue(val.key, val.value); + } + } + kill(signal?: number | NodeJS.Signals): boolean { return this.process.kill(signal); } @@ -91,31 +101,36 @@ class CGroupLimitedProcess extends EventEmitter implements IProcess { this.stdout = null; this.stderr = null; - this.cgroup_root.DeleteSubgroup(this.id); + this.root_cgroup.DeleteSubgroup(this.id); this.process.removeAllListeners(); this.removeAllListeners(); } } export class QemuResourceLimitedLauncher implements IProcessLauncher { - public group; private limits; private name; + private root; + public group; constructor(name: string, limits: CgroupLimits) { - let root = CGroup.Self(); + this.root = CGroup.Self(); + + // Make sure + if(limits.runOnCpus) { + this.root.InitControllers(true); + } else { + this.root.InitControllers(false); + } + this.name = name; - this.group = root.GetSubgroup(name); this.limits = limits; - // Set cgroup keys. - for(const val of MakeValuesFromLimits(limits)) { - let controller = this.group.GetController(val.controller); - controller.WriteValue(val.key, val.value); - } + // XXX figure something better out + this.group = this.root.GetSubgroup(this.name); } launch(command: string, opts?: ProcessLaunchOptions | undefined): IProcess { - return new CGroupLimitedProcess(CGroup.Self(), this.name, this.limits, command, opts); + return new CGroupLimitedProcess(this.root, this.name, this.limits, command, opts); } } From 405e88bd1b75e618ec1f336fdf8e3cef2de6f277 Mon Sep 17 00:00:00 2001 From: modeco80 Date: Sun, 3 Nov 2024 13:10:08 -0500 Subject: [PATCH 18/20] cvmts: Allow specifying cgroup cpu period probably limited utility wise but it's there now --- config.example.toml | 23 ++++++++++++++++------- cvmts/src/util/cgroup.ts | 10 +++++----- cvmts/src/vm/qemu_launcher.ts | 10 +++++++++- 3 files changed, 30 insertions(+), 13 deletions(-) diff --git a/config.example.toml b/config.example.toml index b7d8d48..2506247 100644 --- a/config.example.toml +++ b/config.example.toml @@ -57,20 +57,29 @@ qemuArgs = "qemu-system-x86_64" vncPort = 5900 snapshots = true -# Resource limits. Only works on Linux, with `Delegate=yes` set in your .service file. No-op on other platforms. +# Resource limits. +# Only works on Linux, with `Delegate=yes` set in your .service file. No-op on other platforms. # -# cpuUsageMax optionally specifies the max CPU usage as percentage in the common top notation, so 200% means 2 CPUs, 400% is 4 CPUs, -# so on. +# cpuUsageMax is an optional value specifies the max CPU usage as percentage in the common top notation. +# 200% means 2 CPUs, 400% is 4 CPUs. +# +# A general reccomendation is to set this to 100*[vCPU count]. +# For example, if your QEMU command line contains -smp cores=2, then cpuUsageMax should be 200. +# For an overcomitted host, you can use lower values, +# but it *can* get noticable if you throttle CPU too low. # # runOnCpus is an optional array that specifies what CPUs the VM is allowed to run on. -# Systemd user slices can not delegate the cpuset controller, meaning this option will *not* work if you do not use a system service. -# (effectively, it will be a loud no-op that logs an error on startup) +# This option will *not* work if you do not use a system service. (effectively, it will be a loud no-op that logs an error on startup). # -# limitProcess is optional (default false) and determines if only qemu vCPU threads are put into the cgroup, +# periodMs is an optional value in milliseconds that specifies the cgroup's CPU accounting period. +# The default is 100 ms (which matches the cgroups2 defaults), and should work in pretty much all cases, but +# it's a knob provided for any addl. tuning purposes. +# +# limitProcess is an optional boolean (default false) that determines if only qemu vCPU threads are put into the cgroup, # or the entire QEMU process (incl. all its threads). The default behaviour of only limiting vCPU threads # is more than likely what you want, so the example configuration omits specifying this key. # -# Commenting this inline table from the configuration disables resource limiting entirely. +# Commenting or removing this table from the configuration disables resource limiting entirely. resourceLimits = { cpuUsageMax = 100, runOnCpus = [ 2, 4 ] } diff --git a/cvmts/src/util/cgroup.ts b/cvmts/src/util/cgroup.ts index 44783ce..7c97e89 100644 --- a/cvmts/src/util/cgroup.ts +++ b/cvmts/src/util/cgroup.ts @@ -19,8 +19,8 @@ export class CGroupController { WriteValue(key: string, value: string) { try { writeFileSync(path.join(this.cg.Path(), `${this.controller}.${key}`), value); - } catch(e) { - logger.error({error: e, controller_key: `${this.controller}.${key}`, value: value }, 'Failed to set CGroup controller value') + } catch (e) { + logger.error({ error: e, controller_name: this.controller, controller_key: `${this.controller}.${key}`, value: value }, 'Failed to set CGroup controller value'); } } } @@ -35,11 +35,11 @@ export class CGroup { InitControllers(wants_cpuset: boolean) { // Configure this "root" cgroup to provide cpu and cpuset controllers to the leaf // QEMU cgroups. A bit iffy but whatever. - if(wants_cpuset) { + if (wants_cpuset) { try { writeFileSync(path.join(this.path, 'cgroup.subtree_control'), '+cpu +cpuset'); - } catch(err) { - logger.error({error: err}, 'Could not provide cpuset controller to subtree'); + } catch (err) { + logger.error({ error: err }, 'Could not provide cpuset controller to subtree. runOnCpus will not function.'); // just give up if this fails writeFileSync(path.join(this.path, 'cgroup.subtree_control'), '+cpu'); } diff --git a/cvmts/src/vm/qemu_launcher.ts b/cvmts/src/vm/qemu_launcher.ts index a0ff2d8..54134d2 100644 --- a/cvmts/src/vm/qemu_launcher.ts +++ b/cvmts/src/vm/qemu_launcher.ts @@ -7,6 +7,7 @@ import { CGroup } from '../util/cgroup.js'; export interface CgroupLimits { cpuUsageMax?: number; runOnCpus?: number[]; + periodMs?: number; limitProcess?: boolean; } @@ -19,12 +20,19 @@ interface CGroupValue { function MakeValuesFromLimits(limits: CgroupLimits): CGroupValue[] { let option_array = []; + // The default period is 100 ms, which matches cgroups2 defaults. + let periodUs = 100 * 1000; + + // Convert a user-configured period to us, since that's what cgroups2 expects. + if(limits.periodMs) + periodUs = limits.periodMs * 1000; + if (limits.cpuUsageMax) { // cpu.max option_array.push({ controller: 'cpu', key: 'max', - value: `${(limits.cpuUsageMax / 100) * 100000} 100000` + value: `${(limits.cpuUsageMax / 100) * periodUs} ${periodUs}` }); } From b015274e3b39482ea03fcfda6e525b78c7ad996e Mon Sep 17 00:00:00 2001 From: modeco80 Date: Thu, 6 Mar 2025 16:46:09 -0500 Subject: [PATCH 19/20] cvm-rs: Fix out of bound element length scanning Also, add a unit test just to make sure this doesn't happen again. --- cvm-rs/src/guac.rs | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/cvm-rs/src/guac.rs b/cvm-rs/src/guac.rs index a83517b..67b6572 100644 --- a/cvm-rs/src/guac.rs +++ b/cvm-rs/src/guac.rs @@ -92,6 +92,12 @@ pub fn decode_instruction(element_string: &String) -> DecodeResult { // We bound this anyways and do quite the checks, so even though it's not great, // it should be generally fine (TM). loop { + // Make sure scanning the element length does not try to + // go out of bounds. + if current_position >= chars.len() { + return Err(DecodeError::InvalidFormat); + } + let c = chars[current_position]; if c >= '0' && c <= '9' { @@ -103,6 +109,7 @@ pub fn decode_instruction(element_string: &String) -> DecodeResult { return Err(DecodeError::InvalidFormat); } + current_position += 1; } @@ -189,4 +196,11 @@ mod tests { assert!(res.is_ok()); assert_eq!(res.unwrap(), vec); } + + #[test] + fn out_of_bounds_element_does_not_panic() { + let element_string = "0".into(); + let res = decode_instruction(&element_string); + assert!(res.is_err()); + } } From 53dd05ccc2e721637da2e0c718673260f1b35beb Mon Sep 17 00:00:00 2001 From: modeco80 Date: Thu, 6 Mar 2025 17:08:58 -0500 Subject: [PATCH 20/20] cvmts: Fix protocol error disconnection behavior so it actually works --- cvmts/src/CollabVMServer.ts | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/cvmts/src/CollabVMServer.ts b/cvmts/src/CollabVMServer.ts index cd8d14d..876ab3b 100644 --- a/cvmts/src/CollabVMServer.ts +++ b/cvmts/src/CollabVMServer.ts @@ -708,9 +708,14 @@ export default class CollabVMServer { break; } } catch (err) { - // No - this.logger.error(`User ${user?.IP.address} ${user?.username ? `with username ${user?.username}` : ''} sent broken Guacamole: ${err as Error}`); - user?.kick(); + this.logger.error({ + ip: client.IP.address, + username: client.username, + error_message: (err as Error).message + }, 'Error in CollabVMServer#onMessage.'); + + // should probably only do this for protocol errors + client.kick(); } }