From 483fb1961c4219a1c1dd761b98e70c28476e6c95 Mon Sep 17 00:00:00 2001 From: Kacy Fortner Date: Thu, 30 Jul 2026 15:45:03 +0000 Subject: [PATCH 1/6] guard the tls config registry with a lock MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit the dozen handle-keyed maps behind tls.Config — roots, certificate chains, private keys, alpn lists, client-ca settings, the two config hooks and the listener table — plus the counter that hands out config handles were plain module globals with no lock. the session stores in the same file were fixed earlier; the config registry was left, on the assumption that configs are built at startup. they are not. std.net.http builds a fresh client config for every https request, so a task can be allocating a handle and inserting while another is mid-handshake reading the same maps and a third is removing from them in Config.close(). two tasks that read the counter together get the same handle and share one registry slot, which is how a handshake picks up another config's certificate and key; two tasks writing the same map together corrupt it, which shows up as a double free. everything now goes through four small accessors that take one mutex for the length of a single map operation, generic over the value type because the registry is a dozen maps of eight types with the same access shape. nothing slow runs under the lock: pem parsing, certificate verification and the user-supplied verify/selector hooks all take a copy out first and run off it. the new case reproduces the corruption on the unfixed tree — two runs in four died in the allocator — and is stable on the fixed one under green, green with one worker, and the os-thread backend. --- std/net/tls.pith | 210 +++++++++++------- .../test_tls_concurrent_config_registry.pith | 167 ++++++++++++++ .../test_tls_concurrent_config_registry.txt | 3 + 3 files changed, 303 insertions(+), 77 deletions(-) create mode 100644 tests/cases/test_tls_concurrent_config_registry.pith create mode 100644 tests/expected/test_tls_concurrent_config_registry.txt diff --git a/std/net/tls.pith b/std/net/tls.pith index 655e76ec..76549fd0 100644 --- a/std/net/tls.pith +++ b/std/net/tls.pith @@ -16,7 +16,9 @@ import std.os.certs as os_certs import std.time as time from std.io import TcpStream -# handle allocators. valid handles are non-negative (error paths fail with a +# the config registry: a config is a plain Int handle, and everything it holds +# lives in these handle-keyed maps. handle allocators come first. valid handles +# are non-negative (error paths fail with a # Result, they don't hand back a config); the two ranges are kept apart — configs # from 0, connections from a high base — so a handle reads unambiguously in a # trace. they count up so every handle stays non-negative. (these start values @@ -35,6 +37,54 @@ mut native_server_client_ca_pem: Map[Int, String] := {} mut native_server_client_ca_optional: Map[Int, Bool] := {} mut native_listener_config: Map[Int, Int] := {} +# guards the whole config registry above, the two hook maps declared further +# down, and the handle counter. the registry is not startup-only state: an https +# client builds a fresh config for every request (see std.net.http), so one task +# is allocating a handle and inserting while another is mid-handshake reading +# the same maps and a third is removing from them in Config.close(). two tasks +# that read the counter together would also land on the same handle and share a +# registry slot, so a handshake could pick up another config's certificate and +# key. the lock only ever spans a single map operation — pem parsing, +# certificate verification, and the user-supplied config hooks all run off it. +mut native_config_mu := Mutex() + +# the registry accessors. each takes native_config_mu for one map operation and +# hands the value back to the caller, so nothing outside them touches a registry +# map. they are generic over the value type because the registry is a dozen maps +# of eight value types that all share this access shape. +fn config_has[V](values: Map[Int, V], handle: Int) -> Bool: + native_config_mu.lock() + found := values.contains_key(handle) + native_config_mu.unlock() + return found + +fn config_get[V](values: Map[Int, V], handle: Int, fallback: V) -> V: + native_config_mu.lock() + mut out := fallback + if values.contains_key(handle): + out = values[handle] + native_config_mu.unlock() + return out + +fn config_put[V](values: Map[Int, V], handle: Int, value: V): + native_config_mu.lock() + values.insert(handle, value) + native_config_mu.unlock() + +fn config_drop[V](values: Map[Int, V], handle: Int): + native_config_mu.lock() + if values.contains_key(handle): + values.remove(handle) + native_config_mu.unlock() + +# hand out the next config handle, one per caller. +fn next_config_handle() -> Int: + native_config_mu.lock() + handle := next_native_config_handle + next_native_config_handle = next_native_config_handle + 1 + native_config_mu.unlock() + return handle + # how many server handshakes have failed since the process started. a failed # handshake is normal traffic — probes, scanners, clients that hang up — so a # server drops it silently rather than logging a line an attacker could use to @@ -244,17 +294,22 @@ struct ServerResumptionDecision: pub struct Listener: pub handle: Int +# the config hooks, guarded by native_config_mu like the rest of the registry. mut native_config_verify_connection: Map[Int, fn(ConfigHookContext) -> ConfigHookResult] := {} mut native_server_config_selector: Map[Int, fn(ConfigHookContext) -> ConfigHookResult] := {} +# the value config_get falls back to when a config has no hook installed: it +# approves the connection and selects nothing, which is what "no hook" means. +fn no_config_hook(ctx: ConfigHookContext) -> ConfigHookResult: + return ConfigHookResult(Config(0), "") + fn native_config_handle(roots_pem: String) -> Config!: if roots_pem == "": fail "tls root certificates not found" # parse once here so config creation fails early for bad pem data. x509.parse_pem_chain(roots_pem)! - handle := next_native_config_handle - next_native_config_handle = next_native_config_handle + 1 - native_config_roots_pem.insert(handle, roots_pem) + handle := next_config_handle() + config_put(native_config_roots_pem, handle, roots_pem) return Config(handle) fn native_server_config_handle(cert_pem: String, key_pem: String) -> Config!: @@ -264,16 +319,14 @@ fn native_server_config_handle(cert_pem: String, key_pem: String) -> Config!: key := encoding.pem_decode(key_pem)! if key.kind != "PRIVATE KEY": fail "tls private key must be pkcs#8 pem" - handle := next_native_config_handle - next_native_config_handle = next_native_config_handle + 1 - native_server_cert_pem.insert(handle, cert_pem) - native_server_key_der.insert(handle, key.body) + handle := next_config_handle() + config_put(native_server_cert_pem, handle, cert_pem) + config_put(native_server_key_der, handle, key.body) return Config(handle) fn config_alpn_protocols(config: Config) -> List[String]: - if native_config_alpn.contains_key(config.handle): - return native_config_alpn[config.handle] - return [] + none_offered: List[String] := [] + return config_get(native_config_alpn, config.handle, none_offered) fn config_alpn_key(config: Config) -> String: protocols := config_alpn_protocols(config) @@ -285,21 +338,24 @@ fn config_session_cache_key(config: Config, server_name: String) -> String: return config.handle.to_string() + "|" + server_name + "|" + config_alpn_key(config) fn config_has_client_certificate(config: Config) -> Bool: - return native_client_cert_pem.contains_key(config.handle) and native_client_key_der.contains_key(config.handle) + return config_has(native_client_cert_pem, config.handle) and config_has(native_client_key_der, config.handle) fn config_requires_client_certificate(config: Config) -> Bool: - return native_server_client_ca_pem.contains_key(config.handle) and (not native_server_client_ca_optional.contains_key(config.handle) or not native_server_client_ca_optional[config.handle]) + # a config with client ca roots requires a certificate unless it was + # installed as optional; an absent flag reads as required. + return config_has(native_server_client_ca_pem, config.handle) and not config_get(native_server_client_ca_optional, config.handle, false) fn config_requests_client_certificate(config: Config) -> Bool: - return native_server_client_ca_pem.contains_key(config.handle) + return config_has(native_server_client_ca_pem, config.handle) fn config_resumption_enabled(config: Config) -> Bool: - return native_config_resumption_enabled.contains_key(config.handle) and native_config_resumption_enabled[config.handle] + return config_get(native_config_resumption_enabled, config.handle, false) fn native_config_client_cert_chain(config: Config) -> List[Bytes]!: - if not native_client_cert_pem.contains_key(config.handle): + cert_pem := config_get(native_client_cert_pem, config.handle, "") + if cert_pem == "": fail "native tls client certificate not found" - blocks := encoding.pem_decode_all(native_client_cert_pem[config.handle])! + blocks := encoding.pem_decode_all(cert_pem)! certs: List[Bytes] := [] for block in blocks: if block.kind == "CERTIFICATE": @@ -309,28 +365,31 @@ fn native_config_client_cert_chain(config: Config) -> List[Bytes]!: return certs fn native_client_private_key(config: Config) -> Bytes!: - if not native_client_key_der.contains_key(config.handle): + key := config_get(native_client_key_der, config.handle, bytes.empty()) + if key.len() == 0: fail "native tls client key not found" - return native_client_key_der[config.handle] + return key fn native_config_roots(config: Config) -> List[x509.Certificate]!: - if not native_config_roots_pem.contains_key(config.handle): + roots_pem := config_get(native_config_roots_pem, config.handle, "") + if roots_pem == "": fail "native tls client config not found" - return x509.parse_pem_chain(native_config_roots_pem[config.handle])! + return x509.parse_pem_chain(roots_pem)! fn is_native_config(config: Config) -> Bool: - return native_config_roots_pem.contains_key(config.handle) + return config_has(native_config_roots_pem, config.handle) fn is_native_server_config(config: Config) -> Bool: - return native_server_cert_pem.contains_key(config.handle) + return config_has(native_server_cert_pem, config.handle) fn is_native_conn(handle: Int) -> Bool: return native_conn_has(handle) fn native_server_certificate_chain(config: Config) -> List[Bytes]!: - if not native_server_cert_pem.contains_key(config.handle): + cert_pem := config_get(native_server_cert_pem, config.handle, "") + if cert_pem == "": fail "native tls server config not found" - blocks := encoding.pem_decode_all(native_server_cert_pem[config.handle])! + blocks := encoding.pem_decode_all(cert_pem)! certs: List[Bytes] := [] for block in blocks: if block.kind == "CERTIFICATE": @@ -340,9 +399,10 @@ fn native_server_certificate_chain(config: Config) -> List[Bytes]!: return certs fn native_server_private_key(config: Config) -> Bytes!: - if not native_server_key_der.contains_key(config.handle): + key := config_get(native_server_key_der, config.handle, bytes.empty()) + if key.len() == 0: fail "native tls server key not found" - return native_server_key_der[config.handle] + return key fn checked_read(result: Int!binary.BinaryError) -> Int!: if result.is_err: @@ -444,9 +504,10 @@ fn verify_certificate_chain(certs: List[x509.Certificate], config: Config, serve return true fn verify_client_certificate_chain(certs: List[x509.Certificate], config: Config) -> Bool!: - if not native_server_client_ca_pem.contains_key(config.handle): + ca_pem := config_get(native_server_client_ca_pem, config.handle, "") + if ca_pem == "": fail "native tls client ca config not found" - roots := x509.parse_pem_chain(native_server_client_ca_pem[config.handle])! + roots := x509.parse_pem_chain(ca_pem)! result := x509.verify_client_chain(certs, roots) if not result.ok: fail result.reason @@ -535,20 +596,25 @@ fn copy_client_hello_info(info: ClientHelloInfo) -> ClientHelloInfo: return ClientHelloInfo(info.server_name, copy_string_list(info.alpn_protocols)) fn run_verify_connection_hook(config: Config, state: ConnectionState) -> Bool!: - if not native_config_verify_connection.contains_key(config.handle): + if not config_has(native_config_verify_connection, config.handle): return true + hook := config_get(native_config_verify_connection, config.handle, no_config_hook) ctx := ConfigHookContext("verify_connection", copy_connection_state(state), empty_client_hello_info()) - result := native_config_verify_connection[config.handle](ctx) + # the hook is application code and may do anything, so it runs off the + # registry lock — config_get already copied the fn value out. + result := hook(ctx) if result.reason != "": fail result.reason return true fn select_server_config(config: Config, client_hello: tls13.ClientHello) -> Config!: - if not native_server_config_selector.contains_key(config.handle): + if not config_has(native_server_config_selector, config.handle): return config + selector := config_get(native_server_config_selector, config.handle, no_config_hook) info := ClientHelloInfo(client_hello.server_name, copy_string_list(client_hello.alpn_protocols)) ctx := ConfigHookContext("select_server_config", empty_connection_state(), copy_client_hello_info(info)) - result := native_server_config_selector[config.handle](ctx) + # application code again: called off the lock. + result := selector(ctx) selected := result.selected_config if not is_native_server_config(selected): fail "tls config selector must return a server config" @@ -1122,7 +1188,7 @@ pub fn server_config(cert_path: String, key_path: String) -> Config!: # install a server-side config selector that runs before the native handshake. pub fn with_config_selector(config: Config, chooser: fn(ClientHelloInfo) -> Config) -> Config: - native_server_config_selector.insert(config.handle, fn(ctx: ConfigHookContext) => ConfigHookResult(chooser(ctx.client_hello), "")) + config_put(native_server_config_selector, config.handle, fn(ctx: ConfigHookContext) => ConfigHookResult(chooser(ctx.client_hello), "")) return config # dial a tls server with system roots and host name verification. @@ -1153,7 +1219,7 @@ pub fn conn_from_handle(handle: Int) -> Conn: pub fn listen(host: String, port: Int, config: Config) -> Listener!: if is_native_server_config(config): fd := tcp_listen(host, port)! - native_listener_config.insert(fd, config.handle) + config_put(native_listener_config, fd, config.handle) return Listener(fd) if is_native_config(config): fail "tls.listen needs a server config, not a client config (use tls.server_config)" @@ -1215,10 +1281,9 @@ impl Config: if protocol != "": next_protocols.push(protocol) if next_protocols.len() == 0: - if native_config_alpn.contains_key(self.handle): - native_config_alpn.remove(self.handle) + config_drop(native_config_alpn, self.handle) return self - native_config_alpn.insert(self.handle, next_protocols) + config_put(native_config_alpn, self.handle, next_protocols) return self fn with_client_certificate(cert_path: String, key_path: String) -> Config!: @@ -1230,8 +1295,8 @@ impl Config: key := encoding.pem_decode(key_pem)! if key.kind != "PRIVATE KEY": fail "tls client private key must be pkcs#8 pem" - native_client_cert_pem.insert(self.handle, cert_pem) - native_client_key_der.insert(self.handle, key.body) + config_put(native_client_cert_pem, self.handle, cert_pem) + config_put(native_client_key_der, self.handle, key.body) return self fn require_client_ca_file(path: String) -> Config!: @@ -1239,9 +1304,8 @@ impl Config: roots := x509.parse_pem_chain(roots_pem)! if roots.len() == 0: fail "tls client ca bundle is empty" - native_server_client_ca_pem.insert(self.handle, roots_pem) - if native_server_client_ca_optional.contains_key(self.handle): - native_server_client_ca_optional.remove(self.handle) + config_put(native_server_client_ca_pem, self.handle, roots_pem) + config_drop(native_server_client_ca_optional, self.handle) return self fn request_client_ca_file(path: String) -> Config!: @@ -1249,41 +1313,30 @@ impl Config: roots := x509.parse_pem_chain(roots_pem)! if roots.len() == 0: fail "tls client ca bundle is empty" - native_server_client_ca_pem.insert(self.handle, roots_pem) - native_server_client_ca_optional.insert(self.handle, true) + config_put(native_server_client_ca_pem, self.handle, roots_pem) + config_put(native_server_client_ca_optional, self.handle, true) return self fn enable_session_resumption() -> Config: - native_config_resumption_enabled.insert(self.handle, true) + config_put(native_config_resumption_enabled, self.handle, true) return self fn with_verify_connection(verify: fn(ConnectionState) -> String) -> Config: - native_config_verify_connection.insert(self.handle, fn(ctx: ConfigHookContext) => ConfigHookResult(Config(0), verify(ctx.state))) + config_put(native_config_verify_connection, self.handle, fn(ctx: ConfigHookContext) => ConfigHookResult(Config(0), verify(ctx.state))) return self fn close(): - if native_config_roots_pem.contains_key(self.handle): - native_config_roots_pem.remove(self.handle) - if native_config_alpn.contains_key(self.handle): - native_config_alpn.remove(self.handle) - if native_config_resumption_enabled.contains_key(self.handle): - native_config_resumption_enabled.remove(self.handle) - if native_config_verify_connection.contains_key(self.handle): - native_config_verify_connection.remove(self.handle) - if native_server_config_selector.contains_key(self.handle): - native_server_config_selector.remove(self.handle) - if native_client_cert_pem.contains_key(self.handle): - native_client_cert_pem.remove(self.handle) - if native_client_key_der.contains_key(self.handle): - native_client_key_der.remove(self.handle) - if native_server_cert_pem.contains_key(self.handle): - native_server_cert_pem.remove(self.handle) - if native_server_key_der.contains_key(self.handle): - native_server_key_der.remove(self.handle) - if native_server_client_ca_pem.contains_key(self.handle): - native_server_client_ca_pem.remove(self.handle) - if native_server_client_ca_optional.contains_key(self.handle): - native_server_client_ca_optional.remove(self.handle) + config_drop(native_config_roots_pem, self.handle) + config_drop(native_config_alpn, self.handle) + config_drop(native_config_resumption_enabled, self.handle) + config_drop(native_config_verify_connection, self.handle) + config_drop(native_server_config_selector, self.handle) + config_drop(native_client_cert_pem, self.handle) + config_drop(native_client_key_der, self.handle) + config_drop(native_server_cert_pem, self.handle) + config_drop(native_server_key_der, self.handle) + config_drop(native_server_client_ca_pem, self.handle) + config_drop(native_server_client_ca_optional, self.handle) return impl Listener: @@ -1291,7 +1344,7 @@ impl Listener: return self.handshake(self.accept_socket()!)! fn accept_socket() -> Int!: - if native_listener_config.contains_key(self.handle): + if config_has(native_listener_config, self.handle): fd := tcp_accept(self.handle)! # bound the handshake (and the connection's reads) so a slow or # silent client can't pin a server task before it authenticates. @@ -1300,17 +1353,20 @@ impl Listener: fail "native tls listener not found" fn handshake(fd: Int) -> Conn!: - if not native_listener_config.contains_key(self.handle): + # config handles count up from 0, so a negative one means this listener + # has no config registered (it was never listened on, or it was closed). + config_handle := config_get(native_listener_config, self.handle, 0 - 1) + if config_handle < 0: note_failed_handshake(fd) fail "native tls listener not found" # the socket belongs to this call, so every error exit has to close it: # once the handshake fails there is no Conn for the caller to close. errdefer note_failed_handshake(fd) - return native_server_handshake(TcpStream(fd), Config(native_listener_config[self.handle]))! + return native_server_handshake(TcpStream(fd), Config(config_handle))! fn close(): - if native_listener_config.contains_key(self.handle): - native_listener_config.remove(self.handle) + if config_has(native_listener_config, self.handle): + config_drop(native_listener_config, self.handle) tcp_close(self.handle) return return @@ -1456,13 +1512,13 @@ test "tls config helpers track alpn, client auth, and resumption": assert_eq(config_alpn_protocols(client_cfg)[0], "pith.rpc") assert(config_has_client_certificate(client_cfg)) assert(config_resumption_enabled(client_cfg)) - assert(native_config_verify_connection.contains_key(client_cfg.handle)) + assert(config_has(native_config_verify_connection, client_cfg.handle)) assert_eq(select_alpn_protocol(server_cfg, ["pith.rpc", "http/1.1"])!, "pith.rpc") assert(select_alpn_protocol(server_cfg, ["http/1.1"]).is_err) assert(config_requests_client_certificate(server_cfg)) assert(config_requires_client_certificate(server_cfg)) - assert(native_config_verify_connection.contains_key(server_cfg.handle)) - assert(native_server_config_selector.contains_key(server_cfg.handle)) + assert(config_has(native_config_verify_connection, server_cfg.handle)) + assert(config_has(native_server_config_selector, server_cfg.handle)) optional_cfg := server_config("tests/live/fixtures/localhost.crt", "tests/live/fixtures/localhost.key")!.request_client_ca_file("tests/live/fixtures/localhost-ca.crt")! assert(config_requests_client_certificate(optional_cfg)) diff --git a/tests/cases/test_tls_concurrent_config_registry.pith b/tests/cases/test_tls_concurrent_config_registry.pith new file mode 100644 index 00000000..5261891e --- /dev/null +++ b/tests/cases/test_tls_concurrent_config_registry.pith @@ -0,0 +1,167 @@ +# the tls config registry — the dozen handle-keyed maps holding every config's +# roots, certificate, key, alpn list and hooks, plus the counter that hands out +# the handles — is shared by every task in the process. it is not startup-only +# state: an https client builds a fresh config for each request, and the config +# builders (with_alpn, enable_session_resumption, close) write to those maps +# whenever they are called. unguarded, two tasks reading the counter together +# got the same handle and shared one registry slot, which is how a handshake +# ends up with another config's certificate and key; two tasks writing the same +# map together corrupted it outright. +# +# this runs the writers and the readers at the same time. one set of tasks +# churns the builder maps as fast as it can; another set builds fresh configs +# and uses them for real handshakes against a tls echo server, whose own +# handshakes are meanwhile reading the server config out of the same maps. the +# assertions are the two things a torn registry breaks: every config gets a +# handle of its own, and every dial completes and echoes correctly. + +import std.net.tls as tls +import std.time as time + +CERT_PATH := "tests/live/fixtures/localhost.crt" +KEY_PATH := "tests/live/fixtures/localhost.key" +CA_PATH := "tests/live/fixtures/localhost-ca.crt" + +PORT := 50793 + +# the tasks hammering the builder maps, and how hard. +CHURNERS := 6 +ROUNDS := 60000 + +# the tasks doing real handshakes, and how many configs each builds. +DIALERS := 6 +CONFIGS_PER_DIALER := 3 + +fn serve_conn(listener_handle: Int, fd: Int): + conn_r := tls.listener_from_handle(listener_handle).handshake(fd) + if conn_r.is_err: + return + conn := conn_r.ok + msg_r := conn.read(64) + if msg_r.is_err: + conn.close() + return + conn.write_all("echo:" + msg_r.ok) catch 0 + conn.close() + +fn run_server(listener_handle: Int): + listener := tls.listener_from_handle(listener_handle) + while true: + fd_r := listener.accept_socket() + if fd_r.is_err: + return + spawn serve_conn(listener_handle, fd_r.ok) + +# write to the registry without pause: set an alpn list, set the resumption +# flag, drop the alpn list again. every round is three map operations on maps +# the handshakes below are reading. +fn churn(cfg: tls.Config, rounds: Int, done: Channel[Int]): + mut i := 0 + while i < rounds: + cfg.with_alpn(["h2", "http/1.1"]) + cfg.enable_session_resumption() + cfg.with_alpn([]) + i = i + 1 + done.send(rounds) + +fn dial_once(cfg: tls.Config, tag: String) -> Bool: + conn_r := tls.dial_with_config("127.0.0.1", PORT, "localhost", cfg) + if conn_r.is_err: + return false + conn := conn_r.ok + if conn.write_all(tag).is_err: + conn.close() + return false + got_r := conn.read(64) + conn.close() + if got_r.is_err: + return false + return got_r.ok == "echo:" + tag + +# a transient socket failure on a loaded box — a starved accept loop, a refused +# connect while the backlog drains — is retried, the same way the resumption +# case does it, so the exact-count output stays about the registry. +fn dial_with_retry(cfg: tls.Config, tag: String) -> Bool: + mut attempt := 0 + while attempt < 5: + if dial_once(cfg, tag): + return true + attempt = attempt + 1 + time.delay(20 * attempt) + return false + +# build a config, handshake with it, close it, over and over: creation and +# teardown race the churners' writes and each other's handle allocation. +fn dialer(id: Int, handles: Channel[Int], done: Channel[Int]): + mut ok := 0 + mut i := 0 + while i < CONFIGS_PER_DIALER: + built := tls.client_config_with_ca_file(CA_PATH) + if built.is_err: + i = i + 1 + continue + cfg := built.ok.with_alpn(["pith.rpc"]) + handles.send(cfg.handle) + if dial_with_retry(cfg, "d" + id.to_string() + "-" + i.to_string()): + ok = ok + 1 + cfg.close() + i = i + 1 + done.send(ok) + +fn main() -> Int!: + # bind before any client task starts, so a dial can never race the bind. + server_cfg := tls.server_config(CERT_PATH, KEY_PATH)!.with_alpn(["pith.rpc"]) + listener := tls.listen("127.0.0.1", PORT, server_cfg)! + spawn run_server(listener.handle) + + total_configs := DIALERS * CONFIGS_PER_DIALER + handles := Channel[Int](total_configs) + dialed := Channel[Int](DIALERS) + churned := Channel[Int](CHURNERS) + + # the churners get their own configs so their writes never disturb what a + # dialer or the server is about to read back — only the maps are shared. + churn_configs: List[tls.Config] := [] + mut made := 0 + while made < CHURNERS: + churn_configs.push(tls.client_config_with_ca_file(CA_PATH)!) + made = made + 1 + + mut started := 0 + while started < CHURNERS: + cfg := churn_configs[started] + spawn churn(cfg, ROUNDS, churned) + started = started + 1 + mut launched := 0 + while launched < DIALERS: + spawn dialer(launched, handles, dialed) + launched = launched + 1 + + mut echoed := 0 + mut finished := 0 + while finished < DIALERS: + echoed = echoed + dialed.recv().unwrap_or(0) + finished = finished + 1 + mut rounds := 0 + mut settled := 0 + while settled < CHURNERS: + rounds = rounds + churned.recv().unwrap_or(0) + settled = settled + 1 + + mut seen: Map[Int, Bool] := {} + mut duplicates := 0 + mut collected := 0 + while collected < total_configs: + handle := handles.recv().unwrap_or(0 - 1) + if seen.contains_key(handle): + duplicates = duplicates + 1 + seen.insert(handle, true) + collected = collected + 1 + + for cfg in churn_configs: + cfg.close() + + print("duplicate handles: {duplicates}") + print("echoed: {echoed}/{total_configs}") + print("churn rounds: {rounds}") + return 0 diff --git a/tests/expected/test_tls_concurrent_config_registry.txt b/tests/expected/test_tls_concurrent_config_registry.txt new file mode 100644 index 00000000..a3850d29 --- /dev/null +++ b/tests/expected/test_tls_concurrent_config_registry.txt @@ -0,0 +1,3 @@ +duplicate handles: 0 +echoed: 18/18 +churn rounds: 360000 From c75af49596bb2be1d0b67ee049a276daca09f415 Mon Sep 17 00:00:00 2001 From: Kacy Fortner Date: Thu, 30 Jul 2026 15:49:01 +0000 Subject: [PATCH 2/6] guard std.log's global settings min_level, sink_path and output_format are read by every task on its way through log.root(), and written by set_level, set_format, set_file and clear_file. those are public calls that docs/logging.md presents with no ordering requirement, and raising the log level or switching the sink on a running server is the reason they exist, so this is not startup-only state. the level and the format are words, but the sink path is a string, and reassigning it releases the value a reader may have just loaded. the new case segfaults on every run without the lock; the reason it needed a freshly built path per write is that a module constant's string is never freed, so a constant sink path hides the window entirely. root() now takes the three under one lock so a logger cannot mix a level from before a reconfiguration with a sink from after it. the lock is never held across the log write: emit_record works from a Logger that already carries its own copies, so the file append and the stderr write stay off it. --- std/log.pith | 29 ++++++- .../test_log_concurrent_reconfigure.pith | 86 +++++++++++++++++++ .../test_log_concurrent_reconfigure.txt | 3 + 3 files changed, 116 insertions(+), 2 deletions(-) create mode 100644 tests/cases/test_log_concurrent_reconfigure.pith create mode 100644 tests/expected/test_log_concurrent_reconfigure.txt diff --git a/std/log.pith b/std/log.pith index cdde5242..ced96379 100644 --- a/std/log.pith +++ b/std/log.pith @@ -60,6 +60,16 @@ pub struct Event: pub message: String pub fields: List[Field] +# the global logger's settings, and the lock over them. reconfiguration is not +# a startup-only step: set_level, set_format and set_file are public, documented +# as plain calls with no ordering requirement, and a server that raises its log +# level while it is serving is the obvious reason to have them. so these are +# written from one task while every other task is reading them on its way +# through root() — and sink_path is a string, whose reassignment releases the +# old value under a reader that has just loaded it. the lock covers the reads +# and the writes; it is never held across the log write itself, which happens +# from emit_record on a Logger that already carries its own copy. +mut settings_mu := Mutex() mut min_level: Int := LEVEL_DEBUG mut sink_path: String := "" mut output_format: Int := FORMAT_CONSOLE @@ -141,9 +151,16 @@ fn add_field(base: List[Field], item: Field) -> List[Field]: out.push(item) return out -# create a logger from the current global settings. +# create a logger from the current global settings. the three are read together +# so a logger never mixes a level from before a reconfiguration with a sink from +# after it. pub fn root() -> Logger: - return Logger(min_level, output_format, sink_path, [], "", "", "") + settings_mu.lock() + level := min_level + format := output_format + path := sink_path + settings_mu.unlock() + return Logger(level, format, path, [], "", "", "") # create a compact trace id suitable for logs and future propagation. pub fn new_trace_id() -> String!: @@ -161,11 +178,15 @@ pub fn set_level(level: Int): log_set_level(level) fn log_set_level(level: Int): + settings_mu.lock() min_level = level + settings_mu.unlock() # set the global output format. pub fn set_format(format: Int): + settings_mu.lock() output_format = format + settings_mu.unlock() # use console output for the global logger. pub fn set_console(): @@ -180,14 +201,18 @@ pub fn set_file(path: String): log_set_file(path) fn log_set_file(path: String): + settings_mu.lock() sink_path = path + settings_mu.unlock() # stop writing log output to a file sink. pub fn clear_file(): log_clear_file() fn log_clear_file(): + settings_mu.lock() sink_path = "" + settings_mu.unlock() impl Logger: fn with(fields: List[Field]) -> Logger: diff --git a/tests/cases/test_log_concurrent_reconfigure.pith b/tests/cases/test_log_concurrent_reconfigure.pith new file mode 100644 index 00000000..aee1beba --- /dev/null +++ b/tests/cases/test_log_concurrent_reconfigure.pith @@ -0,0 +1,86 @@ +# std.log's global settings — the minimum level, the output format and the file +# sink path — are read by every task on its way through log.root(), and written +# by set_level / set_format / set_file / clear_file, which are public calls with +# no documented ordering. so a server that raises its log level or switches its +# sink while it is serving has one task writing the settings under all the +# others reading them. the level and the format are words, but the sink path is +# a string: reassigning it releases the value a reader may have just loaded. +# +# the readers below check that every logger root() hands back carries a +# recognizable triple — a level that was actually set, a format that was +# actually set, and a sink path that is either empty or one the writers set. the +# writers hand each set_file a freshly built path rather than a module constant, +# because a constant's string is never freed and so never exposes the window. +# unguarded, this segfaults on every run. + +import std.log as log + +SINK_PATH := "/tmp/pith-log-reconfigure-sink.txt" + +READERS := 4 +WRITERS := 2 +READS := 50000 +WRITES := 50000 + +# flip every setting back and forth as fast as possible. +fn reconfigure(rounds: Int, done: Channel[Int]): + mut i := 0 + while i < rounds: + log.set_file(SINK_PATH + "." + i.to_string()) + log.set_level(log.warn_level()) + log.set_json() + log.clear_file() + log.set_level(log.debug_level()) + log.set_console() + i = i + 1 + done.send(rounds) + +# read the settings back through root() and count anything unrecognizable. +fn observe(rounds: Int, done: Channel[Int]): + mut bad := 0 + mut i := 0 + while i < rounds: + logger := log.root() + if logger.min_level != log.debug_level() and logger.min_level != log.warn_level(): + bad = bad + 1 + if logger.format != 0 and logger.format != 1: + bad = bad + 1 + if logger.sink_path != "" and not logger.sink_path.starts_with(SINK_PATH): + bad = bad + 1 + i = i + 1 + done.send(bad) + +fn main() -> Int: + written := Channel[Int](WRITERS) + observed := Channel[Int](READERS) + + mut w := 0 + while w < WRITERS: + spawn reconfigure(WRITES, written) + w = w + 1 + mut r := 0 + while r < READERS: + spawn observe(READS, observed) + r = r + 1 + + mut torn := 0 + mut seen := 0 + while seen < READERS: + torn = torn + observed.recv().unwrap_or(0 - 1) + seen = seen + 1 + mut rounds := 0 + mut settled := 0 + while settled < WRITERS: + rounds = rounds + written.recv().unwrap_or(0) + settled = settled + 1 + + # leave the process where it started so nothing downstream inherits a sink. + # nothing is ever logged here, so no sink file is created to clean up. + log.clear_file() + log.set_console() + log.set_level(log.debug_level()) + + print("torn reads: {torn}") + print("reads: {READERS * READS}") + print("reconfigurations: {rounds}") + return 0 diff --git a/tests/expected/test_log_concurrent_reconfigure.txt b/tests/expected/test_log_concurrent_reconfigure.txt new file mode 100644 index 00000000..09a65e6d --- /dev/null +++ b/tests/expected/test_log_concurrent_reconfigure.txt @@ -0,0 +1,3 @@ +torn reads: 0 +reads: 200000 +reconfigurations: 100000 From c7b393b917b592f17b521c4669659097946c334c Mon Sep 17 00:00:00 2001 From: Kacy Fortner Date: Thu, 30 Jul 2026 15:49:55 +0000 Subject: [PATCH 3/6] guard std.testing's pass/fail tally MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit record_pass and record_fail bumped bare counters, so two tasks asserting at once lost results — and a lost result means a failure that never gets reported. asserting from a spawned task is the normal way to test concurrent code, so this is a shape users will hit. the lock covers the counters and the silent flag and is released before the line is printed, so a slow terminal cannot serialize the tasks under test. done() reads the pair together for a summary that adds up. the regression lives beside the counters rather than in tests/cases because they are private to the module; it fails on every run without the lock. --- std/testing.pith | 62 +++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 56 insertions(+), 6 deletions(-) diff --git a/std/testing.pith b/std/testing.pith index dc84ed4b..cb8187e3 100644 --- a/std/testing.pith +++ b/std/testing.pith @@ -11,27 +11,44 @@ import std.fs as fs +# the tally, and the lock over it. an assertion made from a spawned task is a +# perfectly ordinary thing to write — testing concurrent code is what spawning +# in a test is for — and two tasks incrementing a bare counter lose results, +# which here means a failure that never gets reported. the lock covers the +# counters and the silent flag; the line is printed after it is released, so a +# slow terminal never serializes the tasks under test. +mut counts_mu := Mutex() mut pass_count: Int := 0 mut fail_count: Int := 0 mut silent: Bool := false fn reset_counts(): + counts_mu.lock() pass_count = 0 fail_count = 0 silent = false + counts_mu.unlock() fn silence_output(): + counts_mu.lock() silent = true + counts_mu.unlock() fn record_pass(name: String): + counts_mu.lock() pass_count = pass_count + 1 - if silent: + quiet := silent + counts_mu.unlock() + if quiet: return print(" ok " + name) fn record_fail(name: String, detail: String): + counts_mu.lock() fail_count = fail_count + 1 - if silent: + quiet := silent + counts_mu.unlock() + if quiet: return print(" FAIL " + name + " -- " + detail) @@ -144,12 +161,15 @@ pub fn with_temp_dir(prefix: String, run: fn(String) -> Bool) -> Bool: # print summary pub fn done(): - total := pass_count + fail_count + counts_mu.lock() + passed := pass_count + failed := fail_count + counts_mu.unlock() print("") - if fail_count == 0: - print(total.to_string() + " passed") + if failed == 0: + print((passed + failed).to_string() + " passed") else: - print(pass_count.to_string() + " passed, " + fail_count.to_string() + " failed") + print(passed.to_string() + " passed, " + failed.to_string() + " failed") test "testing helpers count passing checks": reset_counts() @@ -199,6 +219,36 @@ test "testing helpers inspect files and temp dirs": assert(pass_count == 1) assert(fail_count == 0) +# assert from a task, so the tally takes concurrent writes. the counters are +# private to this module, which is why this regression lives beside them rather +# than in tests/cases. +fn assert_from_task(rounds: Int) -> Int: + mut i := 0 + while i < rounds: + assert_eq(i, i) + assert_ne(i, i + 1) + i = i + 1 + return rounds * 2 + +test "the tally survives assertions made from several tasks at once": + reset_counts() + silence_output() + + per := 2000 + a := spawn assert_from_task(per) + b := spawn assert_from_task(per) + c := spawn assert_from_task(per) + d := spawn assert_from_task(per) + + mut expected := 0 + expected = expected + await a + expected = expected + await b + expected = expected + await c + expected = expected + await d + + assert(pass_count == expected) + assert(fail_count == 0) + # a payload enum for the compiler regression test below. the two payload # variants deliberately carry different heap types so the test can reuse # one binding name across arms. From be218ed3096c97e11c3f9e70f5c93bd2fe83e286 Mon Sep 17 00:00:00 2001 From: Kacy Fortner Date: Thu, 30 Jul 2026 15:53:03 +0000 Subject: [PATCH 4/6] record a verdict at every remaining shared global in std MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit comment-only. the concurrency audit reached a conclusion for each of these and the reasoning was going to be lost, so it now sits at the declaration instead: why the global is safe as it stands, not just that it looked safe. three of them were already correct and are now checked rather than assumed — metrics, where the point is that no map is read outside the lock and the snapshot render does no i/o; the db pool and pin registries, where the lock only covers the lookup; and obs, uuid and trace's span sink. the rest are safe because of when they are written. std.args is a setup-then-query parser with no path that mutates from the query side. the grpc dispatches and the http/2 streaming handler are assigned as the first statement of a serve call that then blocks in its accept loop, so they land before the listener exists. trace's on/off flag and sampler settings are startup config in plain words holding no reference, so a late change costs at most one span's decision. the two semaphores are `mut` only because that is how a global holding a constructed value is declared. --- std/args.pith | 8 ++++++++ std/db/mysql.pith | 10 +++++++++- std/db/postgres.pith | 6 ++++++ std/metrics.pith | 7 +++++++ std/net/grpc.pith | 13 +++++++++++++ std/net/http2/server.pith | 9 ++++++++- std/obs.pith | 6 +++++- std/prometheus.pith | 4 +++- std/trace.pith | 19 +++++++++++++++++++ std/uuid.pith | 2 ++ 10 files changed, 80 insertions(+), 4 deletions(-) diff --git a/std/args.pith b/std/args.pith index 6a8e654a..0517eb6c 100644 --- a/std/args.pith +++ b/std/args.pith @@ -15,6 +15,14 @@ # cross-module access in the current runtime # =============================================================== +# these are shared globals with no lock, and that is safe because the parser has +# exactly one shape: init, add_flag / add_option / add_positional, then parse, +# all from main before the program does any work. every function that writes is +# one of those setup calls, and every function that reads (get_flag, get_option, +# get_positional, has_errors, get_errors, help_text) only reads — there is no +# path in the module that mutates from the query side, so once parse returns the +# state is fixed for the life of the process. calling the setup half from a +# spawned task is not supported and would need a lock. mut app_name: String := "" mut app_desc: String := "" mut flags_data: String := "" diff --git a/std/db/mysql.pith b/std/db/mysql.pith index 76518524..9d3e2bf3 100644 --- a/std/db/mysql.pith +++ b/std/db/mysql.pith @@ -40,6 +40,9 @@ import std.time as time # for two reasons: a Db then carries only a plain handle, which sidesteps a # cross-module retain bug when a caller holds the handle; and one lock covers # every pool, so concurrent tasks borrow and return connections safely. +# every read of `pools` — open, the three acquire/release/discard helpers, and +# Db.close — takes registry_mu around the lookup and nothing else, so a slow +# connect or close happens off the lock. mut pools: List[mysql.Pool] := [] mut registry_mu := Mutex() @@ -117,12 +120,17 @@ struct Pin: mut broken: Bool mut finished: Bool +# every access to `pins` is one of the small helpers below, and each takes +# pins_mu around exactly one field read or write; the connection i/o that +# follows (release, discard) happens after the unlock. mut pins: List[Pin] := [] mut pins_mu := Mutex() # the server-side prepared statement behind each Stmt, indexed by the slot the # Stmt carries. it lives beside the pin registry (not inside Pin) so a Tx, which -# prepares nothing lasting, needs no placeholder statement. +# prepares nothing lasting, needs no placeholder statement — and it is guarded +# by pins_mu, the same lock, since register_prepared and prepared_stmt are its +# only two accesses. mut prepared: List[mysql.Stmt] := [] # pin a fresh connection from pool `pool_handle` and register it, returning the diff --git a/std/db/postgres.pith b/std/db/postgres.pith index f8e3ec48..cc74f56c 100644 --- a/std/db/postgres.pith +++ b/std/db/postgres.pith @@ -39,6 +39,9 @@ import std.time as time # for two reasons: a Db then carries only a plain handle, which sidesteps a # cross-module retain bug when a caller holds the handle; and one lock covers # every pool, so concurrent tasks borrow and return connections safely. +# every read of `pools` — open, the three acquire/release/discard helpers, and +# Db.close — takes registry_mu around the lookup and nothing else, so a slow +# connect or close happens off the lock. mut pools: List[postgres.Pool] := [] mut registry_mu := Mutex() @@ -117,6 +120,9 @@ struct Pin: mut broken: Bool mut finished: Bool +# every access to `pins` is one of the small helpers below, and each takes +# pins_mu around exactly one field read or write; the connection i/o that +# follows (release, discard) happens after the unlock. mut pins: List[Pin] := [] mut pins_mu := Mutex() diff --git a/std/metrics.pith b/std/metrics.pith index b8b3d539..c3611df3 100644 --- a/std/metrics.pith +++ b/std/metrics.pith @@ -33,6 +33,13 @@ mut histogram_bucket_defs: Map[String, List[Float]] := {} # guards all the metric maps so concurrent updates from separate threads do not # race. each public operation takes it around its whole read-modify-write; the # internal helpers (ensure_*, *_value, render_*) assume it is already held. +# +# every access to every map in this module goes through that rule — checked, not +# assumed: nothing reads a map outside the lock, which is the failure a +# write-guarded registry usually still has. holding it across a whole +# snapshot_text() render is deliberate and safe: rendering is string building +# with no i/o in it, so it cannot park a green task, and the scrape's socket +# write happens in std.prometheus after this module has handed the text back. mut metrics_mu := Mutex() # label support. a metric can have several series, one per distinct label set diff --git a/std/net/grpc.pith b/std/net/grpc.pith index d4cc8ac0..d1a2a9f3 100644 --- a/std/net/grpc.pith +++ b/std/net/grpc.pith @@ -1202,6 +1202,15 @@ EXPIRED_ON_ARRIVAL := "grpc: deadline expired before the handler ran" fn expired_response() -> http.HttpResponse: return http.response(200).content_type(GRPC_CONTENT_TYPE).with_trailers(grpc_status_trailers(GRPC_DEADLINE_EXCEEDED, EXPIRED_ON_ARRIVAL)) +# the process-wide unary dispatch, and the same for the body-level and streaming +# forms further down. all three are shared and unlocked, and safe because of +# where they are written: each is assigned as the first statement of the serve() +# call that installs it, and that call then blocks in the accept loop for the +# life of the process. so the write happens before the listener exists, let +# alone the first request task, and nothing writes them again. the cost is one +# dispatch per process — two concurrent serve() calls would fight over the slot, +# which is the same single-handler constraint std.net.http2.server documents for +# its streaming handler. mut grpc_dispatch_fn: fn(String, Bytes) -> Bytes!GrpcError := grpc_no_dispatch fn grpc_no_dispatch(path: String, request: Bytes) -> Bytes!GrpcError: @@ -1281,6 +1290,8 @@ pub fn serve_tls(host: String, port: Int, cert: String, key: String, dispatch: f # this, deframing per method. buffered means the whole request and reply pass # through memory — the right tool for bounded streams, not endless ones. +# installed by serve_body / serve_body_tls; see grpc_dispatch_fn above for why +# this needs no lock. mut grpc_body_dispatch_fn: fn(String, Bytes) -> Bytes!GrpcError := grpc_no_dispatch fn grpc_serve_body_handler(req: http.HttpRequestBytes) -> http.HttpResponse: @@ -1457,6 +1468,8 @@ impl GrpcStream: # the stream dispatch lives in a module global for the same reason the unary # dispatch does: a pith closure cannot capture a fn-value, so there is one # streaming grpc server per process. +# installed by the streaming serve calls; see grpc_dispatch_fn above for why +# this needs no lock. mut grpc_stream_dispatch_fn: fn(String, GrpcStream) -> Int!GrpcError := grpc_no_stream_dispatch fn grpc_no_stream_dispatch(path: String, call: GrpcStream) -> Int!GrpcError: diff --git a/std/net/http2/server.pith b/std/net/http2/server.pith index d0519baf..751cac40 100644 --- a/std/net/http2/server.pith +++ b/std/net/http2/server.pith @@ -95,7 +95,9 @@ MAX_CONNECTIONS := 512 # the in-flight permits, shared between the accept loops and the connection # tasks. a Semaphore is safe to share across threads; it lives as a module # global rather than a spawned-task argument (which the compiler cannot yet -# dispatch for a sync-primitive type). +# dispatch for a sync-primitive type). it is `mut` only because that is how a +# global holding a constructed value is declared — the binding itself is never +# reassigned, and the permit count is the semaphore's own synchronized state. mut connection_slots := Semaphore(MAX_CONNECTIONS) # how long a connection may sit without the socket making progress before the @@ -554,6 +556,11 @@ impl ServerStream: # fn(ServerStream) would make Server and ServerStream mutually recursive, which # the type system resolves in file order and so cannot express. the cost is one # streaming handler per process — the same shape grpc already documents. +# +# shared and unlocked, and safe for the same reason grpc's dispatch is: the +# assignment is the first statement of the listen_*_streaming call that installs +# it, and that call then blocks in the accept loop, so it lands before the +# listener exists and is never written again. mut stream_handler_fn: fn(ServerStream) -> Int := stream_handler_unset # the default streaming handler: refuse the stream, so a misconfigured server diff --git a/std/obs.pith b/std/obs.pith index 8bf94f13..231d12da 100644 --- a/std/obs.pith +++ b/std/obs.pith @@ -51,7 +51,11 @@ DEFAULT_SPAN_DELAY_MS := 5000 # module constant, because a module-level float constant is miscompiled to zero. DEFAULT_SAMPLER := "parentbased_always_on" -# guards the two flags below so init/shutdown and the loop agree on state. +# guards the two flags below so init/shutdown and the loop agree on state. every +# access goes through it: set_running and is_running for the loop, and the +# read-modify-writes in start and shutdown, which is what makes init idempotent +# rather than merely usually-idempotent. the flush calls are outside it, so the +# exporter's POST never runs under the lock. mut obs_mu := Mutex() # whether init has already run (init is idempotent). mut started := false diff --git a/std/prometheus.pith b/std/prometheus.pith index 1f097a06..a89a157c 100644 --- a/std/prometheus.pith +++ b/std/prometheus.pith @@ -30,7 +30,9 @@ MAX_CONCURRENT_SCRAPES := 64 # the in-flight permits, shared between the accept loop and the scrape tasks. a # Semaphore is safe to share across threads; it lives as a module global rather # than a spawned-task argument (which the compiler can't yet dispatch for a -# sync-primitive type). +# sync-primitive type). it is `mut` only because that is how a global holding a +# constructed value is declared — the binding itself is never reassigned, and +# the permit count is the semaphore's own synchronized state. mut scrape_slots := Semaphore(MAX_CONCURRENT_SCRAPES) # the /metrics response for the current metric snapshot. diff --git a/std/trace.pith b/std/trace.pith index 1581efa0..dd976f04 100644 --- a/std/trace.pith +++ b/std/trace.pith @@ -73,6 +73,12 @@ pub struct Link: # global on/off. off => start() returns a non-recording span and touches no # per-thread state, so the overhead of leaving trace calls in is negligible. +# +# shared and unlocked on purpose. std.obs.init sets it once at startup, before +# any task exists, and it is a machine word holding no reference, so a later +# flip cannot tear or free anything under a reader — the worst a racing task +# sees is the previous answer for one span, which is what turning tracing on or +# off mid-flight means anyway. mut trace_active := false # turn tracing on or off process-wide. std.obs.init sets this from the @@ -198,6 +204,13 @@ pub SAMPLER_PARENT_ALWAYS_OFF := 4 pub SAMPLER_PARENT_TRACEIDRATIO := 5 # the active sampler and its ratio (only used by the traceidratio modes). +# +# shared and unlocked for the same reason trace_active is: std.obs.start +# installs the sampler before it turns tracing on, so both are set before the +# first span, and both are plain words holding no reference. a task that reads +# the mode while a later set_sampler is halfway through can pair a new mode with +# an old ratio, which changes at most one span's sampling decision — and only +# for the traceidratio modes, which are the ones that consult the ratio at all. mut sampler_mode := SAMPLER_PARENT_ALWAYS_ON mut sampler_ratio := 1.0 @@ -409,6 +422,12 @@ impl Span: record_finished_span(self) # --- export buffer: ended spans wait here for the OTLP exporter to drain --- +# +# every span that ends writes here and the exporter drains from here, so unlike +# the two settings above this really is contended state. all three accesses +# (set_sink, record_finished_span, drain_finished_spans) take the lock, and the +# drain hands the list over and installs a fresh one rather than exporting under +# it, so the exporter's POST runs off the lock. mut sink_mu := Mutex() mut sink_spans: List[Span] := [] mut sink_active := false diff --git a/std/uuid.pith b/std/uuid.pith index 5b7342d5..72728f0c 100644 --- a/std/uuid.pith +++ b/std/uuid.pith @@ -22,6 +22,8 @@ mut last_v7_ms: Int := 0 mut last_v7_counter: Int := -1 # guards the v7 monotonic clock/counter so concurrent v7 generation from # separate threads stays race-free and monotonic within a millisecond. +# next_v7_counter is the only thing that touches either of them, and it takes +# the lock around the whole compare-and-advance. mut v7_mu := Mutex() fn uuid_error(message: String) -> UuidError: From 66319dc8e1cf3a9f5437e926918dc3d36303ca03 Mon Sep 17 00:00:00 2001 From: Kacy Fortner Date: Thu, 30 Jul 2026 16:00:27 +0000 Subject: [PATCH 5/6] record what the std globals audit found and left MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit three things the sweep turned up that are calls to make rather than bugs to fix: std.metrics is correct but serializes every counter, gauge and histogram write in the process on one lock (numbers included — aggregate throughput falls as tasks are added); std.net.tls configs are never closed and std.net.http builds one per https request, so a 219 KB copy of the system root bundle accumulates per call; and std.args is safe only because of a setup-then-query convention nothing enforces. each is written with the evidence and the options rather than a decision, next to the items the runtime pass left. --- docs/limitations.md | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/docs/limitations.md b/docs/limitations.md index 8d53e5ab..d8d75ee5 100644 --- a/docs/limitations.md +++ b/docs/limitations.md @@ -223,3 +223,40 @@ payload (the caller-side cascade does not yet treat a call argument as the borrow it now provably is), and extracting the same optional local twice is a rare use-after-free that needs a second-extraction check rather than the blanket retain that was tried and reverted for regressing the common single case. + +sweeping std's shared globals for the same class of bug turned up three things +that are questions of design rather than repairs, so they are recorded here +instead of decided. + +`std.metrics` is correct under concurrency and does not scale. one mutex covers +all thirteen registries, so every counter increment, gauge set and histogram +observation in the process serializes on it, and a metric written once per +request is the normal case. measured on the 2-core box, one task manages 7.4M +increments a second; two manage 3.7M between them, four 3.0M, eight 2.4M — the +aggregate falls as tasks are added, which is what a single lock looks like. the +absolute cost is still small next to a request, around 0.4 µs per increment at +eight tasks, so nothing is on fire. it is the shape that will not hold if a +process ever writes metrics faster than it serves requests. every way out costs +something. sharding by metric name spreads the contention but makes a coherent +snapshot harder to take. a lock per series adds a lookup to the hot path. +atomic counters are the obvious answer for a counter and no answer at all for a +histogram, which updates seven values as one unit. + +`std.net.tls` configs are never closed, and `std.net.http` builds one per https +request. `Config.close()` exists and removes the config's entries from the +registry, but nothing in std or the examples calls it, so every https request +adds another copy of the system root bundle — 219 KB here — to a map that only +grows. two hundred configs held 44 MB that closing them released. the fix is an +ownership decision rather than a patch: either the http client closes the config +it built, which means being sure no response still refers to it, or the default +client config becomes a process-wide value built once, which changes what +`tls.client_config()` returns. + +`std.args` is safe by convention, with nothing enforcing the convention. its ten +globals have no lock, which is fine given the parser's shape: init, then the +add_flag / add_option / add_positional calls, then parse, all from main, after +which everything left only reads. no path in the module mutates from the query +side, so the state really is fixed once parse returns. nothing stops a program +from calling the setup half from a spawned task, though, and there would be no +diagnostic if it did — just a torn string. a lock here is cheap. whether a cli +argument parser should carry one is the part worth an opinion. From 8d46b062c44f45f9cf928790fc07debbc56ef3d2 Mon Sep 17 00:00:00 2001 From: Kacy Fortner Date: Thu, 30 Jul 2026 17:18:37 +0000 Subject: [PATCH 6/6] keep the tls selector fallback off a real config handle MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit the no-hook fallback returned Config(0), and in a server-only program handle 0 is a real server config — so a selector removed between the presence check and the read would have been accepted as a valid selection instead of rejected. a negative handle no config can hold makes it fail the way the missing-selector path already does. --- std/net/tls.pith | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/std/net/tls.pith b/std/net/tls.pith index 76549fd0..93fb1218 100644 --- a/std/net/tls.pith +++ b/std/net/tls.pith @@ -299,9 +299,12 @@ mut native_config_verify_connection: Map[Int, fn(ConfigHookContext) -> ConfigHoo mut native_server_config_selector: Map[Int, fn(ConfigHookContext) -> ConfigHookResult] := {} # the value config_get falls back to when a config has no hook installed: it -# approves the connection and selects nothing, which is what "no hook" means. +# approves the connection and selects nothing. the callers check first and only +# reach this if the hook was removed in between, so the selected config is the +# negative handle no config can have rather than 0, which in a server-only +# program is a real server config the selector must not be able to land on. fn no_config_hook(ctx: ConfigHookContext) -> ConfigHookResult: - return ConfigHookResult(Config(0), "") + return ConfigHookResult(Config(0 - 1), "") fn native_config_handle(roots_pem: String) -> Config!: if roots_pem == "":