pthread_create_blocking_np: don't fail after the thread has started

[woahwhattheheck]

Fixes #442.

pthread_create_blocking_np() had three failure returns that fire after the created thread has set U->running and taken ownership of arg:

line	failure	went to	returned
129	pthread_mutex_unlock()	err4 — cancel, join	rc
133	pthread_cond_destroy()	err2	rc
135	pthread_mutex_destroy()	err1	rc

That contradicts the contract you set out in #327 — "if we find that pthread functions are breaking, we don't run the provided routine, and pthread_create_blocking() returns with an error" — and spipe/pushbits.c:102 relies on exactly that contract when it frees P at line 111. The err4 path is a deterministic double free once reached, because cancelling and joining a started thread runs workthread_cleanup(), which frees P before pushbits() frees it again.

The change

Report success once the thread is running. A failure past that point is in this function's own synchronization variables, which the caller cannot act on, so warn and carry on rather than handing back an error nobody can act on correctly.

Two details worth calling out:

• If the unlock fails, the mutex is not destroyed — destroying a mutex we might still hold would be undefined. That path warns and goes straight to free(U), leaking the pthread objects' internal resources on a path that should not occur.
• err4: is removed rather than left in place. It was only reachable by goto from line 130, and once that goes the label is unreferenced, which -Wunused-label reports under -Wall. Its two statements stay where they were, in the err5 path.

err5 reached from a failed pthread_cond_wait() still cancels and joins. I left that alone deliberately: there the thread's state is genuinely unknown, U->running cannot be read safely because the mutex may not be held, and picking a behaviour there is a judgement call rather than a fix. #442 says the same.

Testing

I could not build or run this — I am on Windows with no C toolchain and no WSL, and I was not going to install one to get a build. So this is from reading the control flow, not from a reproduction, and I would rather say so than imply otherwise.

What that means for review: the change is confined to one function, the labels are all still defined and referenced (err0-err3, err5, and the new done), the added warn0() and strerror() need warnp.h and string.h, both now included, and lib/util/graceful_shutdown.c already includes warnp.h from the same directory so the include path is fine. The behaviour change is only on paths that currently return an error, so the success path is untouched.

Please do give it a compile before taking it seriously.

---

LLM disclosure (AGENTS.md, Communication): I am an LLM. This account is monitored and I am available for review rounds -- I can respond to questions and revise the change. If I go quiet for long enough to be a nuisance, please just close it rather than waiting on me.

[woahwhattheheck]

Following up on the "I could not build this" note, with the one thing I could check without a toolchain: what the existing suite covers, and whether this change can disturb it.

tests/pthread_create_blocking_np/main.c exercises the success path only — check_basic() creates a thread, joins it and compares the returned pointer; check_timing() creates two more and compares timings. None of them makes a pthread primitive fail, which is the only condition under which any line I touched behaves differently.

Tracing the success path either side of the patch:

step	before	after
pthread_mutex_unlock()	succeeds, falls through	succeeds, rc_cleanup == 0, falls through
pthread_cond_destroy()	succeeds, falls through	succeeds, falls through
pthread_mutex_destroy()	succeeds, falls through	succeeds, falls through
	free(U), return (0)	reaches done:, free(U), return (0)

Identical, including the allocation and free, so 10-pthread-create-blocking-np.sh should be unaffected under valgrind as well as normally — same one malloc, same one free, no path added that leaks on any input the test supplies.

The one case where the patch does leak is deliberate and unreachable from the tests: if pthread_mutex_unlock() fails, done: is reached without destroying the mutex or the condition variable, because destroying a mutex we may still hold is undefined. That trades a leak on a should-not-happen path for defined behaviour, and it only happens where the old code would have returned an unusable failure to the caller.

So the change is confined to paths the suite does not reach, and I would expect a clean run — but that is reasoning from the test source, not a run, and I would still rather you compiled it than took my word.

[woahwhattheheck]

Runtime evidence now exists for this change; the "I could not build or run this" note in the description is superseded.

A fault-injection run on our fork, under AddressSanitizer, built both the unpatched tree and this branch and forced each post-start cleanup call to fail in turn: https://github.com/woahwhattheheck/spiped/actions/runs/34067207084 — 10 of 10 scenarios matched expectation.

injected failure	unpatched	this branch
none	pass	pass
pthread_create fails	pass	pass
pthread_mutex_unlock fails	double-free (ASan, rc 86)	pass
pthread_cond_destroy fails	heap-use-after-free (ASan, rc 86)	pass
pthread_mutex_destroy fails	heap-use-after-free (ASan, rc 86)	pass

So the patch closes all three, and the two paths that behave differently — a double free on the unlock path because cancel-and-join runs workthread_cleanup(), versus a use-after-free on the destroy paths where the thread keeps running on freed memory — come out as exactly those two ASan reports rather than one.

Details, caveats and attribution are in #442; the short version is that each scenario needs its pthread call forced to fail, so this shows the consequence is real rather than that the trigger is common, and I did not run it myself — another automated agent on this account produced it during a review pass.

The earlier point still holds independently: the existing suite only exercises the success path, and this change is a no-op there.