give every remaining shared global in std a verdict, and fix the racy ones - #626
Merged
Conversation
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.
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.
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.
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.
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.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
phase three of the concurrency audit: a verdict for every remaining shared
mutable global in
std/, and a fix for the three that were actually racy.what was racy
the tls config registry. 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 handles
were unlocked. the file's session stores were guarded in an earlier pass; this
half was left on the assumption that configs are startup state. they are not:
std.net.httpbuilds a fresh client config for every https request, so one taskallocates a handle and inserts while another is mid-handshake reading the same
maps and a third is removing from them in
Config.close(). two tasks readingthe counter together get the same handle and share a registry slot, which is how
a handshake picks up another config's certificate and key. everything now goes
through four small accessors that hold one mutex for a single map operation;
pem parsing, certificate verification and the user-supplied hooks all take a
copy out and run off the lock. the new case dies in the allocator on the unfixed
tree.
std.log's global settings.min_level,sink_pathandoutput_formatare read by every task through
log.root()and written byset_level,set_format,set_fileandclear_file, which docs/logging.md presents withno ordering requirement. the sink path is a string, and reassigning it releases
what a reader has just loaded — the new case segfaults on every run without the
lock.
root()now reads the three together; the file append and the stderrwrite stay off the lock.
std.testing's tally.record_passandrecord_failbumped bare counters,so assertions made from spawned tasks lost results, which means a failure that
never gets reported. the lock covers the counters and is released before the
line is printed.
the verdicts
std.metricsmetrics_mu; the snapshot render holds it but does no i/ostd.argsparsereturnsstd.tracetrace_active,sampler_mode,sampler_ratioobs.startbefore the first span, and plain words holding no referencestd.tracesink_spans,sink_activestd.db.mysql/std.db.postgrespools,pins,preparedstd.net.grpcservethat then blocks in its accept loopstd.net.http2.serverstream_handler_fnstd.uuidlast_v7_ms,last_v7_counternext_v7_counteris the only accessstd.obsstarted,runninginitidempotentstd.prometheus/std.net.http2.servermutonly because that is how a global holding a constructed value is declaredstd.net.tlsstd.logstd.testingeach reason now sits in a comment at the declaration so the next pass does not
re-derive it.
left as decisions
three findings are calls to make rather than bugs to fix, so they went into
docs/limitations.mdbeside what the runtime pass left, with the evidence:std.metricsis correct and does not scale. one mutex covers all thirteenregistries, and aggregate throughput falls as tasks are added — 7.4M
increments a second at one task, 3.7M at two, 2.4M at eight on the 2-core box.
sharding, per-series locks and atomics each give something up.
std.net.tlsconfigs are never closed andstd.net.httpbuilds one per httpsrequest, so a 219 KB copy of the system root bundle accumulates per call. two
hundred configs held 44 MB. fixing it means deciding who owns the config.
std.argsis safe by a convention nothing enforces.what ordering the fixes rely on
nothing new. the grpc and http/2 handler slots keep their existing
one-per-process constraint, which is already documented at both declarations.
the tls listener's config lookup now reads a handle out under the lock instead
of holding a reference into the map, and uses a negative sentinel so a config
closed mid-handshake fails the way a missing one already does.
tested
make testandPITH_GREEN=0 make test— 99 examples, 287 regression casesthrough both compilers, all invalid-input and cli and ir-contract checks
make verify-green-corpusandmake verify-osthread-corpus— 287 each, afterthe last change
make run-examples,make docsite-check, 75 colocated std test files throughthe self-hosted compiler
examples/grpc_chat,examples/grpc_reflect,examples/web_observabilityrun repeatedly against their expected output
make leak-checkandmake memcheck— the tls accessors allocate on thehandshake path, so both were worth running; flat and clean
allocator, the log one segfaults every run