diff --git a/CHANGELOG.md b/CHANGELOG.md index 1a0a60c..da0220f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,11 @@ # Changelog +## [Unreleased] + +### Fixed + +- Semaphore capacity is validated as a bounded positive decimal and normalized before storage, arithmetic, and JSON serialization. Leading-zero values such as `01`, `08`, and `010` keep their decimal meaning, including when reading metadata written by older versions. Invalid stored capacities fail with `store-read` (#35). + All notable changes to this project are recorded here. The format follows Keep a Changelog; versions follow SemVer. ## [Unreleased] diff --git a/Makefile b/Makefile index fede374..02ceaf9 100644 --- a/Makefile +++ b/Makefile @@ -14,6 +14,7 @@ lint: test: bash test/test.sh + python3 test/capacity.py test-docker: # the same suite inside the official bash image, for a wall between the tests and your machine docker run --rm -v "$(CURDIR)":/src -w /src bash:5.2 bash -c 'apk add --no-cache git python3 py3-jsonschema >/dev/null && git config --global user.email t@example.invalid && git config --global user.name t && bash test/test.sh' diff --git a/README.md b/README.md index 64cdd64..92564c5 100644 --- a/README.md +++ b/README.md @@ -261,6 +261,8 @@ In summary, "all or nothing" is never a loop with a rollback. It is one stanza, A path lock says one holder. Some resources are better described by a number: two GPUs, five build agents. This section shows how git-locks gives a named resource a capacity while keeping the same atomicity, and it needs one new idea, a generation token, that the reader has all the pieces for. +Capacity is a positive decimal integer from `1` through `9223372036854775807`. Leading zeros are accepted and normalized: `--capacity 010` means ten and emits JSON `10`. Reads also normalize leading-zero capacities stored by older versions without rewriting their metadata. Invalid or out-of-range input is refused before semaphore refs are created; invalid stored capacity is a `store-read` error. + A semaphore is three kinds of ref under `refs/locks/sem//`. `meta` points at a blob holding the capacity. `slots/` is one ref per holder, pointing at a slot record with the holder and an expiry, exactly like a lock record. And `gen` points at a blob whose only purpose is to change: every transaction on the semaphore writes a fresh generation blob and `update`s `gen` from the generation it read. Two acquirers that both read "2 of 3 live" both try to move `gen` from the same old value; git lets exactly one through, and the other re-reads and finds the semaphore full. Here is the example's semaphore, capacity 2, filled by alice and bob, then refused to carol: ```text diff --git a/bin/git-locks b/bin/git-locks index ff04446..fc13c2e 100755 --- a/bin/git-locks +++ b/bin/git-locks @@ -1733,6 +1733,19 @@ with_release_sem() { # record -> releases this invocation's slot, if it is still # acquirers who both counted "n of N live" contend on one compare-and-swap and # exactly one commits. The other re-reads. +valid_capacity() { # VAR value: canonical positive decimal in Bash's signed 64-bit arithmetic range + [[ "$2" =~ ^[0-9]+$ ]] || return 1 + local _capacity="${2#"${2%%[!0]*}"}" # strip leading zeros without evaluating input as arithmetic + [[ -n "${_capacity}" ]] || return 1 + ((${#_capacity} <= 19)) || return 1 + # Compare equal-length decimal strings: arithmetic would overflow before rejecting the input. + # shellcheck disable=SC2071 + if ((${#_capacity} == 19)) && [[ "${_capacity}" > 9223372036854775807 ]]; then + return 1 + fi + printf -v "$1" '%s' "${_capacity}" +} + sem_meta_ref() { printf '%s/sem/%s/meta' "${NS}" "$1"; } sem_gen_ref() { printf '%s/sem/%s/gen' "${NS}" "$1"; } sem_slot_ref() { printf '%s/sem/%s/slots/%s' "${NS}" "$1" "$2"; } @@ -1771,6 +1784,7 @@ sem_read() { # name -> 0, or 1 when the semaphore does not exist SEM_META_OID="$(ref_oid "${mref}")" [[ -n "${SEM_META_OID}" ]] || return 1 SEM_CAP="$(field "${SEM_META_OID}" capacity)" + valid_capacity SEM_CAP "${SEM_CAP}" || store_error "semaphore ${name} has an invalid capacity" gref="$(sem_gen_ref "${name}")" SEM_GEN_OID="$(ref_oid "${gref}")" SLOT_JOBS=() @@ -2008,7 +2022,7 @@ cmd_sem() { done case "${verb}" in create) - [[ "${capacity}" =~ ^[0-9]+$ && "${capacity}" -gt 0 ]] || fail '--capacity is a positive number' 2 + valid_capacity capacity "${capacity}" || fail '--capacity is a decimal integer from 1 through 9223372036854775807' 2 if sem_read "${name}"; then sem_refusal "${name}" exists exit 1 diff --git a/lib/170-semaphores.sh b/lib/170-semaphores.sh index 5242a6d..88133a0 100644 --- a/lib/170-semaphores.sh +++ b/lib/170-semaphores.sh @@ -5,6 +5,19 @@ # acquirers who both counted "n of N live" contend on one compare-and-swap and # exactly one commits. The other re-reads. +valid_capacity() { # VAR value: canonical positive decimal in Bash's signed 64-bit arithmetic range + [[ "$2" =~ ^[0-9]+$ ]] || return 1 + local _capacity="${2#"${2%%[!0]*}"}" # strip leading zeros without evaluating input as arithmetic + [[ -n "${_capacity}" ]] || return 1 + ((${#_capacity} <= 19)) || return 1 + # Compare equal-length decimal strings: arithmetic would overflow before rejecting the input. + # shellcheck disable=SC2071 + if ((${#_capacity} == 19)) && [[ "${_capacity}" > 9223372036854775807 ]]; then + return 1 + fi + printf -v "$1" '%s' "${_capacity}" +} + sem_meta_ref() { printf '%s/sem/%s/meta' "${NS}" "$1"; } sem_gen_ref() { printf '%s/sem/%s/gen' "${NS}" "$1"; } sem_slot_ref() { printf '%s/sem/%s/slots/%s' "${NS}" "$1" "$2"; } @@ -43,6 +56,7 @@ sem_read() { # name -> 0, or 1 when the semaphore does not exist SEM_META_OID="$(ref_oid "${mref}")" [[ -n "${SEM_META_OID}" ]] || return 1 SEM_CAP="$(field "${SEM_META_OID}" capacity)" + valid_capacity SEM_CAP "${SEM_CAP}" || store_error "semaphore ${name} has an invalid capacity" gref="$(sem_gen_ref "${name}")" SEM_GEN_OID="$(ref_oid "${gref}")" SLOT_JOBS=() @@ -280,7 +294,7 @@ cmd_sem() { done case "${verb}" in create) - [[ "${capacity}" =~ ^[0-9]+$ && "${capacity}" -gt 0 ]] || fail '--capacity is a positive number' 2 + valid_capacity capacity "${capacity}" || fail '--capacity is a decimal integer from 1 through 9223372036854775807' 2 if sem_read "${name}"; then sem_refusal "${name}" exists exit 1 diff --git a/test/capacity.py b/test/capacity.py new file mode 100644 index 0000000..b383f16 --- /dev/null +++ b/test/capacity.py @@ -0,0 +1,127 @@ +#!/usr/bin/env python3 +"""Exercise decimal capacity through the CLI with an independent integer oracle.""" +import concurrent.futures +import json +import os +from pathlib import Path +import random +import subprocess +import tempfile + +import jsonschema + +ROOT = Path(__file__).resolve().parents[1] +CLI = ROOT / 'bin/git-locks' +SCHEMA = json.loads((ROOT / 'schema/git-locks.schema.json').read_text()) +VALIDATOR = jsonschema.Draft202012Validator(SCHEMA) +SEED = 3507 +LIMIT = 9223372036854775807 +checks = 0 + + +def check(condition, message): + global checks + assert condition, message + checks += 1 + + +with tempfile.TemporaryDirectory(prefix='git-locks-capacity-') as tmp: + base = Path(tmp) + env = {key: value for key, value in os.environ.items() + if not key.startswith('GIT_')} + env.update(HOME=tmp, GIT_LOCKS_STORE=str(base / 'store.git'), + GIT_LOCKS_NOW='1000000', LC_ALL='C') + + def run(*args, expected=0): + result = subprocess.run([str(CLI), *args], cwd=tmp, env=env, + text=True, capture_output=True, timeout=45) + check(result.returncode == expected, + f'{args}: exit {result.returncode}, expected {expected}: {result.stderr}') + data = [] + for output in (result.stdout, result.stderr): + for line in output.splitlines(): + value = json.loads(line) + VALIDATOR.validate(value) + data.append(value) + check(bool(data), f'{args}: missing structured output') + return data + + def git(*args, input=None): + return subprocess.run(['git', '--git-dir=' + env['GIT_LOCKS_STORE'], *args], + cwd=tmp, env=env, text=True, input=input, + capture_output=True, check=True).stdout.strip() + + rng = random.Random(SEED) + values = ['1', '01', '08', '010', str(LIMIT), '000' + str(LIMIT), '0' * 256 + '8'] + values += ['0' * rng.randrange(1, 25) + str(rng.randrange(1, 1000000)) for _ in range(32)] + for index, value in enumerate(values): + name = f'valid-{index}' + want = int(value, 10) + check(run('sem', 'create', name, '--capacity', value)[0]['capacity'] == want, + f'create did not normalize {value!r}') + check(run('sem', 'show', name)[0]['capacity'] == want, 'show capacity mismatch') + meta = git('show', f'refs/locks/sem/{name}/meta') + check(f'capacity: {want}' in meta.splitlines(), 'stored capacity is not canonical') + check(run('sem', 'acquire', name, '--job', 'a', '--holder', 'alice')[0]['capacity'] == want, + 'acquire capacity mismatch') + check(run('sem', 'release', name, '--job', 'a')[0]['capacity'] == want, + 'release capacity mismatch') + run('sem', 'delete', name) + + invalid = ['', '0', '00', '-1', '+1', '1.0', '1e2', ' 1', '1 ', '08x', '12', + str(LIMIT + 1), str(2**64 + 1), '9' * 100, '0' * 100 + str(LIMIT + 1)] + invalid += [str(rng.randrange(LIMIT + 1, 2**100)) for _ in range(16)] + before = git('for-each-ref', '--format=%(refname) %(objectname)', 'refs/locks/') + for index, value in enumerate(invalid): + result = run('sem', 'create', f'invalid-{index}', '--capacity', value, expected=2) + check(result[0]['reason'] == 'usage', 'invalid capacity is not a usage error') + check(git('for-each-ref', '--format=%(refname) %(objectname)', 'refs/locks/') == before, + 'invalid input changed authoritative refs') + + # Stores written by 0.7.0 may have leading zeros; reads normalize without rewriting. + run('sem', 'create', 'legacy', '--capacity', '1') + meta = git('show', 'refs/locks/sem/legacy/meta') + oid = git('hash-object', '-w', '--stdin', input=meta.replace('capacity: 1', 'capacity: 01') + '\n') + git('update-ref', 'refs/locks/sem/legacy/meta', oid) + check(run('sem', 'show', 'legacy')[0]['capacity'] == 1, 'legacy show is not normalized') + check(run('sem', 'list')[0]['capacity'] == 1, 'legacy list is not normalized') + check(git('rev-parse', 'refs/locks/sem/legacy/meta') == oid, 'read rewrote legacy metadata') + run('sem', 'acquire', 'legacy', '--job', 'first', '--holder', 'alice') + refused = run('sem', 'acquire', 'legacy', '--job', 'second', '--holder', 'bob', expected=1)[0] + check(refused['reason'] == 'capacity' and refused['capacity'] == 1, 'legacy capacity not enforced') + run('sem', 'release', 'legacy', '--job', 'first') + run('sem', 'delete', 'legacy') + + # A fresh metadata record can be corrupt independently of its JSON serialization. + for value in ('0', '08x', str(LIMIT + 1)): + run('sem', 'create', 'bad-meta', '--capacity', '1') + meta = git('show', 'refs/locks/sem/bad-meta/meta') + original = git('rev-parse', 'refs/locks/sem/bad-meta/meta') + oid = git('hash-object', '-w', '--stdin', input=meta.replace('capacity: 1', 'capacity: ' + value) + '\n') + git('update-ref', 'refs/locks/sem/bad-meta/meta', oid) + result = run('sem', 'show', 'bad-meta', expected=2) + check(result[0]['reason'] == 'store-read', 'bad stored capacity did not fail closed') + check(git('rev-parse', 'refs/locks/sem/bad-meta/meta') == oid, 'bad read changed metadata') + git('update-ref', 'refs/locks/sem/bad-meta/meta', original) + run('sem', 'delete', 'bad-meta') + + run('sem', 'create', 'race', '--capacity', '03') + + def contender(index): + result = subprocess.run([str(CLI), 'sem', 'acquire', 'race', '--job', f'r{index}', + '--holder', f'h{index}'], cwd=tmp, env=env, text=True, + capture_output=True, timeout=45) + assert result.returncode in (0, 1), result + for line in (result.stdout + result.stderr).splitlines(): + VALIDATOR.validate(json.loads(line)) + return result.returncode + + with concurrent.futures.ThreadPoolExecutor(max_workers=12) as pool: + statuses = list(pool.map(contender, range(12))) + check(statuses.count(0) == 3, f'12 racers on capacity 03 had {statuses.count(0)} winners') + observed = run('sem', 'show', 'race')[0] + check(observed['capacity'] == 3 and observed['live'] == 3 and len(observed['slots']) == 3, + 'post-race slot state violates capacity') + check(run('doctor')[0]['healthy'], 'post-race doctor unhealthy') + +print(f'capacity: {checks} checks passed; seed {SEED}; {len(values)} valid and {len(invalid)} invalid inputs; 12 racers / 3 winners') diff --git a/test/test.sh b/test/test.sh index 22657a5..84f44de 100755 --- a/test/test.sh +++ b/test/test.sh @@ -727,6 +727,20 @@ valid "sem delete line" "${out}" git-locks sem show batch >/dev/null 2>&1 check "a deleted semaphore is gone" "$?" "1" +# Decimal capacity must preserve its numeric value in storage and every JSON line. +for capacity_case in '01:1' '08:8' '010:10'; do + capacity_input="${capacity_case%:*}" + capacity_want="${capacity_case#*:}" + out="$(git-locks sem create "decimal-${capacity_want}" --capacity "${capacity_input}" 2>&1)" + check "capacity ${capacity_input} creates a semaphore" "$?" "0" + jfields "capacity ${capacity_input} emits decimal ${capacity_want}" "${out}" "capacity=${capacity_want}" + valid "capacity ${capacity_input} create line" "${out}" + out="$(git-locks sem show "decimal-${capacity_want}" 2>&1)" + check "capacity ${capacity_input} can be read" "$?" "0" + jfields "capacity ${capacity_input} reads decimal ${capacity_want}" "${out}" "capacity=${capacity_want}" + valid "capacity ${capacity_input} show line" "${out}" +done + # exactly K winners under contention git-locks sem create race --capacity 3 >/dev/null 2>&1 wins=0