From 51e63ea22b876580de276808eca891ff6ab2a6c5 Mon Sep 17 00:00:00 2001 From: Christina Wang Date: Mon, 7 Mar 2022 14:10:51 -0800 Subject: [PATCH] Add lockfile binary and internal lib for interfacing with it The linked issue describes the Supervisor not cleaning up locks it creates due to crashing at just the wrong time. After internal discussion we decided to differentiate Supervisor-created lockfiles from user-created lockfiles by using the `nobody` UID (65534) for Supervisor-created lockfiles. As the existing NPM lockfile lib does not allow creating lockfiles atomically with different UIDs, we move to using the lockfile binary, which is part of the procmail package. To allow nonroot users to write to lock directories, permissions are changed to allow write access by nonroot users. See: https://www.flowdock.com/app/rulemotion/r-resinos/threads/gWMgK5hmR26TzWGHux62NpgJtVl Change-type: minor Closes: #1758 Signed-off-by: Christina Wang --- Dockerfile.template | 5 +- entry.sh | 7 ++ src/lib/lockfile.ts | 117 ++++++++++++++++++++++++ test/src/lib/lockfile.spec.ts | 167 ++++++++++++++++++++++++++++++++++ 4 files changed, 295 insertions(+), 1 deletion(-) create mode 100644 src/lib/lockfile.ts create mode 100644 test/src/lib/lockfile.spec.ts diff --git a/Dockerfile.template b/Dockerfile.template index da8d58ae..c28490e2 100644 --- a/Dockerfile.template +++ b/Dockerfile.template @@ -34,7 +34,9 @@ RUN apk add --no-cache \ sqlite-libs \ sqlite-dev \ dmidecode \ - dbus-dev + dbus-dev \ + procmail +# procmail is installed for the lockfile binary COPY build-utils/node-sums.txt . @@ -106,6 +108,7 @@ RUN apk add --no-cache \ WORKDIR /usr/src/app COPY --from=BUILD /usr/local/bin/node /usr/local/bin/node +COPY --from=BUILD /usr/bin/lockfile /usr/bin/lockfile COPY --from=BUILD /usr/src/app/dist ./dist COPY --from=BUILD /usr/src/app/package.json ./ COPY --from=BUILD /usr/src/app/node_modules ./node_modules diff --git a/entry.sh b/entry.sh index 0e242e8d..e680c354 100755 --- a/entry.sh +++ b/entry.sh @@ -56,6 +56,13 @@ fi # not a problem. modprobe ip6_tables || true +export BASE_LOCK_DIR="/tmp/balena-supervisor/services" +export LOCKFILE_UID=65534 + +# Cleanup leftover Supervisor-created lockfiles from any previous processes. +# Supervisor-created lockfiles have a UID of 65534. +find "/mnt/root${BASE_LOCK_DIR}" -type f -user "${LOCKFILE_UID}" -name "*updates.lock" -delete + if [ "${LIVEPUSH}" = "1" ]; then exec npx nodemon --watch src --watch typings --ignore tests -e js,ts,json \ --exec node -r ts-node/register/transpile-only src/app.ts diff --git a/src/lib/lockfile.ts b/src/lib/lockfile.ts new file mode 100644 index 00000000..cef2968d --- /dev/null +++ b/src/lib/lockfile.ts @@ -0,0 +1,117 @@ +import * as fs from 'fs'; +import { dirname } from 'path'; +import { isRight } from 'fp-ts/lib/Either'; + +import { exec, unlinkAll } from './fs-utils'; +import { NumericIdentifier } from '../types'; + +// Equivalent to `drwxrwxrwt` +const STICKY_WRITE_PERMISSIONS = 0o1777; +export const BASE_LOCK_DIR = + process.env.BASE_LOCK_DIR || '/tmp/balena-supervisor/services'; + +const decodedUid = NumericIdentifier.decode(process.env.LOCKFILE_UID); +export const LOCKFILE_UID = isRight(decodedUid) ? decodedUid.right : 65534; + +/** + * Internal lockfile manager to track files in memory + */ +// Track locksTaken, so that the proper locks can be cleaned up on process exit +const locksTaken: { [lockName: string]: boolean } = {}; + +// Returns all current locks taken, as they've been stored in-memory. +export const getLocksTaken = (): string[] => Object.keys(locksTaken); + +// Try to clean up any existing locks when the process exits +process.on('exit', () => { + for (const lockName of getLocksTaken()) { + try { + unlockSync(lockName); + } catch (e) { + // Ignore unlocking errors + } + } +}); + +interface ChildProcessError { + code: number; + stderr: string; + stdout: string; +} + +export class LockfileExistsError implements ChildProcessError { + public code: number; + public stderr: string; + public stdout: string; + + constructor(path: string) { + this.code = 73; + this.stderr = `lockfile: Sorry, giving up on "${path}"`; + this.stdout = ''; + } + + // Check if an error is an instance of LockfileExistsError. + // This is necessary because the error thrown is a child process + // error that isn't typed by default, so instanceof will not work. + public static is(error: unknown): error is LockfileExistsError { + return (error as LockfileExistsError).code === 73; + } +} + +export async function lock(path: string, uid = LOCKFILE_UID) { + /** + * Set parent directory permissions to `drwxrwxrwt` (octal 1777), which are needed + * for lockfile binary to run successfully as the any non-root uid, if executing + * this command as a privileged uid. + * NOTE: This will change the permissions of the parent directory at `path`, + * which may not be expected if using lockfile as an independent module. + * + * `chmod` does not fail or throw if the directory already has the proper permissions. + */ + await fs.promises.chmod(dirname(path), STICKY_WRITE_PERMISSIONS); + + /** + * Run the lockfile binary as the provided UID. See https://linux.die.net/man/1/lockfile + * `-r 0` means that lockfile will not retry if the lock exists. + * If `uid` is not privileged or does not have write permissions to the path, this command will not succeed. + */ + try { + // Lock the file using binary + await exec(`lockfile -r 0 ${path}`, { uid }); + // Store a lock in memory as taken + locksTaken[path] = true; + } catch (error) { + // Code 73 refers to EX_CANTCREAT (73) in sysexits.h, or: + // A (user specified) output file cannot be created. + // See: https://nxmnpg.lemoda.net/3/sysexits + if (LockfileExistsError.is(error)) { + // If error code is 73, updates.lock file already exists, so throw this error directly + throw error; + } else { + /** + * For the most part, a child process error with code 73 should be thrown, + * indicating the lockfile already exists. Any other error's child process + * code should be included in the error message to more clearly signal + * what went wrong. Other errors that are not the typical "file exists" + * errors include but aren't limited to: + * - running out of file descriptors + * - binary corruption + * - other systems-based errors + */ + throw new Error( + `Got code ${(error as ChildProcessError).code} while locking updates: ${ + (error as ChildProcessError).stderr + }`, + ); + } + } +} + +export async function unlock(path: string) { + // Removing the updates.lock file releases the lock + return await unlinkAll(path); +} + +export function unlockSync(path: string) { + return fs.unlinkSync(path); +} diff --git a/test/src/lib/lockfile.spec.ts b/test/src/lib/lockfile.spec.ts new file mode 100644 index 00000000..b23c3159 --- /dev/null +++ b/test/src/lib/lockfile.spec.ts @@ -0,0 +1,167 @@ +import { expect } from 'chai'; +import { dirname, basename } from 'path'; +import { stub, SinonStub } from 'sinon'; +import { promises as fs } from 'fs'; +import mock = require('mock-fs'); + +import * as lockfile from '../../../src/lib/lockfile'; +import * as fsUtils from '../../../src/lib/fs-utils'; +import { ChildProcessError } from '../../../src/lib/errors'; + +describe('lib/lockfile', () => { + const lockPath = `${lockfile.BASE_LOCK_DIR}/1234567/updates.lock`; + // mock-fs expects an octal file mode, however, Node's fs.stat.mode returns a bit field: + // - 16877 (Node) == octal 0755 (drwxr-xr-x) + // - 17407 (Node) == octal 1777 (drwxrwxrwt) + const DEFAULT_PERMISSIONS = { + unix: 0o755, + node: 16877, + }; + const STICKY_WRITE_PERMISSIONS = { + unix: 0o1777, + node: 17407, + }; + + const mockDir = ( + mode: number = DEFAULT_PERMISSIONS.unix, + opts: { createLock: boolean } = { createLock: false }, + ) => { + const items: any = {}; + if (opts.createLock) { + items[basename(lockPath)] = mock.file({ uid: lockfile.LOCKFILE_UID }); + } + + mock({ + [dirname(lockPath)]: mock.directory({ + mode, + items, + }), + }); + }; + + const checkLockDirFiles = async ( + path: string, + opts: { shouldExist: boolean } = { shouldExist: true }, + ) => { + const files = await fs.readdir(dirname(path)); + if (opts.shouldExist) { + expect(files).to.include(basename(path)); + } else { + expect(files).to.have.length(0); + } + }; + + let execStub: SinonStub; + + beforeEach(() => { + // @ts-ignore + execStub = stub(fsUtils, 'exec').callsFake(async (command, opts) => { + // Sanity check for the command call + expect(command.trim().startsWith('lockfile')).to.be.true; + + // Remove any `lockfile` command options to leave just the command and the target filepath + const [, targetPath] = command + .replace(/-v|-nnn|-r\s+\d+|-l\s+\d+|-s\s+\d+|-!|-ml|-mu/g, '') + .split(/\s+/); + + // Emulate the lockfile binary exec call + await fsUtils.touch(targetPath); + await fs.chown(targetPath, opts!.uid!, 0); + }); + }); + + afterEach(() => { + execStub.restore(); + mock.restore(); + }); + + it('should create a lockfile as the `nobody` user at target path', async () => { + // Mock directory with default permissions + mockDir(); + + await lockfile.lock(lockPath); + + // Verify lockfile exists + await checkLockDirFiles(lockPath, { shouldExist: true }); + + // Verify lockfile UID + expect((await fs.stat(lockPath)).uid).to.equal(lockfile.LOCKFILE_UID); + }); + + it('should create a lockfile with the provided `uid` if specified', async () => { + // Mock directory with default permissions + mockDir(); + + await lockfile.lock(lockPath, 2); + + // Verify lockfile exists + await checkLockDirFiles(lockPath, { shouldExist: true }); + + // Verify lockfile UID + expect((await fs.stat(lockPath)).uid).to.equal(2); + }); + + it('should not create a lockfile if `lock` throws', async () => { + // Mock directory with default permissions + mockDir(); + + // Override default exec stub declaration, as it normally emulates a lockfile call with + // no errors, but we want it to throw an error just for this unit test + execStub.restore(); + + const childProcessError = new Error() as ChildProcessError; + childProcessError.code = 73; + childProcessError.stderr = 'lockfile: Test error'; + + execStub = stub(fsUtils, 'exec').throws(childProcessError); + + try { + await lockfile.lock(lockPath); + expect.fail('lockfile.lock should have thrown an error'); + } catch (err) { + expect(err).to.exist; + } + + // Verify lockfile does not exist + await checkLockDirFiles(lockPath, { shouldExist: false }); + }); + + it('should asynchronously unlock a lockfile', async () => { + // Mock directory with sticky + write permissions and existing lockfile + mockDir(STICKY_WRITE_PERMISSIONS.unix, { createLock: true }); + + // Verify lockfile exists + await checkLockDirFiles(lockPath, { shouldExist: true }); + + await lockfile.unlock(lockPath); + + // Verify lockfile removal + await checkLockDirFiles(lockPath, { shouldExist: false }); + }); + + it('should not error on async unlock if lockfile does not exist', async () => { + // Mock directory with sticky + write permissions + mockDir(STICKY_WRITE_PERMISSIONS.unix, { createLock: false }); + + // Verify lockfile does not exist + await checkLockDirFiles(lockPath, { shouldExist: false }); + + try { + await lockfile.unlock(lockPath); + } catch (err) { + expect.fail((err as Error)?.message ?? err); + } + }); + + it('should synchronously unlock a lockfile', () => { + // Mock directory with sticky + write permissions + mockDir(STICKY_WRITE_PERMISSIONS.unix, { createLock: true }); + + lockfile.unlockSync(lockPath); + + // Verify lockfile does not exist + return checkLockDirFiles(lockPath, { shouldExist: false }).catch((err) => { + expect.fail((err as Error)?.message ?? err); + }); + }); +});