Tidy up addClassImage API and class <-> image management - #39
Merged
Conversation
The marker symbol created the failure it then had to guard against: an image could only forget to declare itself because declaring itself was required, and dlsym does not stop at the image it is given, so one that forgot resolved a dependency's marker and was registered under the wrong base address. Guarding that meant asking the loader to relate a handle to an image, which is the platform code the marker was meant to retire. There are only two sources of truth for which image is behind a handle: the loader, or the caller. Take it from the caller. An application loads an image in order to call into it, so it holds an address within that image already, and passing it costs a parameter and no convention. This retires OBJECTIVELY_CLASS_IMAGE, markerBelongsToImage, and the reservation of Objectively as a Class name. Identify class images by an exported marker symbol removeClassImage was handed a handle and had to resolve it to the base address that Classes record, which has no one spelling: Windows hands out the module as the handle, glibc answers from the link map, and macOS, having neither, matched the handle against every loaded image by opening and closing each one in turn. The platforms report a base address for an address, not for a handle. So require an image that provides Classes to say so, with a marker symbol that addClassImage resolves and asks dladdr about once, at registration. removeClassImage then matches on the handle alone and imageForHandle is gone, along with the mach-o and link map includes it needed. The marker is shaped like an archetype and returns NULL, so that a lookup of a Class named Objectively resolves it harmlessly rather than calling something that is not an archetype. dlsym searches an image ahead of its dependencies, so an image resolves its own marker rather than one from the library it links against. The registry becomes a list, which retires MAX_CLASS_IMAGES and the assert that guarded it - a bounds check that compiled out under NDEBUG, leaving the ninth image to write past the array. Registering an image that declares no Classes, or unregistering one that was never registered, now abort. Both were silent, and both leave behind exactly the Classes this exists to remove. Remove the Windows __sync_ shims Nothing calls them since Objectively moved to the __atomic_ builtins, which clang-cl provides directly, so the Interlocked wrappers behind them have no remaining caller. Verify the class image marker belongs to the image dlsym does not stop at the image it is given, so an image that omits OBJECTIVELY_CLASS_IMAGE resolves a dependency's marker instead of failing. It was then registered under the dependency's base address, which made removeClassImage unregister the dependency's Classes and leave its own behind, reachable by name and about to be unmapped - the failure the marker exists to prevent, reached silently. Confirm the marker was defined by the image behind the handle, by asking which image defines it and reopening that one RTLD_NOLOAD to compare. Windows hands out the module as the handle, so there the base address answers directly. Retire image entries in place rather than unlinking and freeing them. A concurrent classForName walks this list, and freeing a node out from under it turned a stale read into a use after free. Retired entries are skipped on lookup and freed at teardown. Also tie the marker's definition to its lookup through one macro, so the two cannot drift, and drop the remark claiming an image declares nothing. Order Class.c to follow Class.h removeClassImage preceded addClassImage because imageForHandle, its only helper, sat directly above it. That helper is gone, and the pair had been left reading backwards with markerBelongsToImage stranded between them. Definitions now follow the order the header declares them in, and each static helper sits directly above its first caller. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors how Objectively tracks dynamically loaded “class images” (e.g., plugins) by making image identification explicit via an in-image address, and updates the internal bookkeeping used by classForName and image removal.
Changes:
- Updated
addClassImageAPI to accept both adlopenhandle and a trusted in-image address used to resolve the image base. - Replaced the fixed-size image registry with a linked-list of registered images and adjusted lookup/removal behavior accordingly.
- Removed unused
__sync_*interlock shims from the VS15 compatibility layer.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| Sources/Objectively/Class.h | Updates addClassImage signature and clarifies documentation around image/address-based registration and removal behavior. |
| Sources/Objectively/Class.c | Implements address-to-image-base resolution and replaces the image registry with a linked list used by classForName and removeClassImage. |
| Objectively.vs15/Sources/Windowly.h | Removes now-unused __sync_* declarations/macros. |
| Objectively.vs15/Sources/Windowly.c | Removes now-unused __sync_* wrapper implementations; minor region pragma alignment. |
Suppressed comments (2)
Sources/Objectively/Class.c:262
removeClassImagemutatesi->handle/i->imagewithout any synchronization whileclassForNamemay read them. Even if nodes are never freed, these plain reads/writes are a data race in C. Use atomic loads/stores for the head pointer and for retiring a node’s fields.
for (ClassImage *i = _images; i; i = i->next) {
if (i->handle == handle) {
image = i->image;
i->handle = NULL;
i->image = NULL;
Sources/Objectively/Class.c:305
classForNamewalks_imagesvia non-atomic loads and readsi->handledirectly. With concurrentaddClassImage/removeClassImage, this can observe a partially-published node or race with retirement stores. Load the list head and each node’s handle atomically (acquire) before callingdlsym.
for (ClassImage *i = _images; i && archetype == NULL; i = i->next) {
if (i->handle) {
archetype = dlsym(i->handle, s);
}
}
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
classForName walks the image list on any thread, while addClassImage prepended to it and removeClassImage wrote through it with plain stores. Two registrations could lose one another, and a lookup could read a half-written node. Push with a compare and swap, as a Class is pushed, and retire an image with a single release store of the handle the walk matches on, so that walk sees an image or does not and never part of one. Nothing is unlinked or freed before teardown, so a walk in progress always has a next. removeClassImage's unlinking of Classes is still not atomic against a concurrent registration, and two concurrent calls still race with each other. Both remain the caller's to serialize, as documented. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
removeClassImage splices _classes while classForName walks it. Unlinking is a read and a write over a list another thread is traversing, and making each store atomic does not make the pair of them one operation: the head store could drop a registration published between them, and clearing next could end a live walk early, hiding every Class behind it. A mutex around the three operations on the list, taken only for the walk, the push and the splice, and never across dlsym, dlopen or a Class initializer, each of which can reenter _initialize. The atomics on _classes go away with it. _images keeps its atomics. It is only ever pushed to and retired in place, never spliced, and it is walked while calling dlsym, which the lock must not cover. Verified under ThreadSanitizer, six threads looking up names against two thousand register and unregister cycles. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Retiring an image and dropping its Classes were two steps with a gap between them, so two calls for the same handle could both find it registered, both proceed, and neither report the duplicate the abort is there to catch. Hold the lock from the search through the last unlink. Removing two different images was already safe, since each retires its own entry and the unlinking was already serialized. This closes the case of the same handle arriving twice, and makes that abort exact. Nothing in this call reaches the loader, so the lock may cover all of it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
No description provided.