Fix scitoken_status_free: missing C-linkage symbol and status type confusion - #212
Open
djw8605 wants to merge 1 commit into
Open
Fix scitoken_status_free: missing C-linkage symbol and status type confusion#212djw8605 wants to merge 1 commit into
djw8605 wants to merge 1 commit into
Conversation
The header declares scitoken_status_free(SciTokenStatus *) inside extern "C", but the implementation defined an overload taking SciTokenStatus by value. C++ treats that as a distinct function with C++ linkage, so the library never emitted the C-linkage symbol at all: C consumers failed to link, and on Linux the mangled name is not matched by the scitoken* glob in the export map either. The implementation also deleted the handle as scitokens::AsyncStatus, but statuses returned by scitoken_deserialize_start are SciTokenAsyncStatus -- an unrelated type -- so freeing one would run the wrong destructor over unrelated memory (including unique_lock members interpreted from garbage). Fix by matching the declared signature (also nulling the caller's handle), wrapping the enforcer async statuses in SciTokenAsyncStatus so every SciTokenStatus handle exposed through the C API is the same concrete type (this also makes the scitoken_status_get_* accessors, which already cast to SciTokenAsyncStatus, correct for enforcer statuses), and rejecting a null status_out in scitoken_deserialize_start instead of dereferencing it. Adds a regression test that frees an async status and verifies the C-linkage symbol resolves. Co-Authored-By: Claude Fable 5 <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.
Problem
Three related bugs in the async-status C API:
The C symbol doesn't exist.
scitokens.hdeclaresvoid scitoken_status_free(SciTokenStatus *status)insideextern "C", butscitokens.cppdefinesscitoken_status_free(SciTokenStatus status)— a different parameter type. C++ treats that as a separate overload with C++ linkage, so the library only exports the mangled_Z20scitoken_status_freePv. Any C consumer using the public header gets an undefined symbol at link time. On Linux it's worse: the mangled name doesn't match thescitoken*glob inconfigs/export-symbols, so the function isn't exported in any form. (Verified withnmbefore/after.)Type confusion on free. The implementation deletes the handle as
scitokens::AsyncStatus*, but statuses returned byscitoken_deserialize_startareSciTokenAsyncStatus*— an unrelated type holding twounique_ptrs. Deleting through the wrong type runs~AsyncStatusover unrelated memory (itsstd::unique_lockmembers get destroyed reading garbage — potential unlock of a wild mutex pointer) and leaks the real members.Accessor type confusion for enforcer statuses.
scitoken_status_get_timeout_val/_get_read_fd_set/ etc. cast the handle toSciTokenAsyncStatus*, butenforcer_generate_acls_startreturned a rawAsyncStatus*— so using the select()-loop accessors with the enforcer async API read garbage.Fix
SciTokenAsyncStatus(start) and unwrap on continue, so everySciTokenStatushandle exposed through the C API is the same concrete type. This simultaneously fixes the free path and thescitoken_status_get_*accessors for enforcer statuses.status_outinscitoken_deserialize_startinstead of dereferencing it.Testing
nm libSciTokens.dylibnow showsT _scitoken_status_free(unmangled).ctestunit, env_config, and monitoring suites pass.🤖 Generated with Claude Code