Conversation
…yout into_presigned_request_ptr built the header key and value arrays by pushing into a Vec and leaking it with mem::forget, while presigned_request_free rebuilt them with Vec::from_raw_parts(ptr, len, len). A pushed Vec usually has capacity greater than len (1-3 headers leave capacity 4), so the arrays were deallocated with the wrong layout, which is undefined behavior. Reuse utils::string_pairs and release_string_pairs, which store the arrays as exact-length boxed slices, and release_c_string for method and uri. Headers are now validated before any string is allocated, so a header value that is not visible ASCII no longer leaks the strings built so far. The FFI struct layout is unchanged, so the C# side needs no update.
The error message, operator info fields, and entry paths each reimplemented the null check and CString::from_raw that utils::release_c_string already provides. Route them through the shared helper so every owned C string in the FFI payloads is released the same way.
- Replace validate_non_zero_u64/u32/usize with one generic validate_positive_integer; the three bodies and error messages were identical. - Replace parse_usize and parse_u64 with a generic parse_number; the error message still names the target type. - Build stat metadata payloads through into_metadata_ptr like write and copy already do.
…l path The Task-returning SubmitAsyncOperation overloads duplicated the Task<bool> overloads line for line, since both register an AsyncState<bool>. Forward them to the Task<bool> overloads instead. This adds no delegate or closure allocation per call.
Each *_option_build and *_option_free pair repeated the same collect, box, and release steps, differing only in the parser and payload type. Move those steps into generic build_options and free_options helpers. Every exported function keeps its own name, signature, and safety contract, and each build function still names the parser it uses, so the FFI surface is unchanged.
Every input and output stream function repeated the same null check and unsafe cast of the opaque stream pointer. Add require_input_stream and require_output_stream next to the stream types, mirroring require_op_handle, so each function validates its handle in one line. The null-pointer errors and their order relative to the other argument checks are unchanged; the free functions keep treating null as a no-op.
TryTakeAsyncState only forwarded to AsyncStateRegistry.TryTake, which already carries the NotNullWhen annotation. Call the registry directly, as OnReadResultRetained already does, and drop the wrapper.
github-actions
Bot
added
bindings/dotnet
releases-note/refactor
This branch has not been deployed
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
1 participant
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.
Which issue does this PR close?
None.
Rationale for this change
The .NET binding repeats the same FFI plumbing in several places, such as C string release, option payload build and free, and stream handle validation. This PR consolidates the copies that share the same contract.
While doing so, I found a memory bug in
src/presign.rs. It built the header arrays by pushing into aVecbut freed them withVec::from_raw_parts(ptr, len, len). A pushedVecusually has more capacity than length, so the arrays were freed with the wrong layout, which is undefined behavior. It also leaked the strings already built when a header value was not visible ASCII. A temporary layout-checking allocator confirmed both problems before the fix and neither after it.What changes are included in this PR?
Each commit is one self-contained change:
utils::string_pairsandrelease_string_pairs, which store exact-length boxed slices, and validate every header before allocating anything. The FFI struct layout is unchanged.utils::release_c_string.validate_non_zero_u64,validate_non_zero_u32, andvalidate_non_zero_usizeinto a genericvalidate_non_zero, andparse_usizeandparse_u64intoparse_number. Stat now builds its metadata payload throughinto_metadata_ptr, like write and copy.Task-returningSubmitAsyncOperationoverloads to theTask<bool>overloads. This adds no delegate or closure allocation per call.*_option_buildand*_option_freepairs through genericbuild_optionsandfree_optionshelpers. Every exported function keeps its own name, signature, and safety contract, and each build function still names the parser it uses.require_input_streamandrequire_output_stream, mirroringrequire_op_handle, instead of repeating the null check and cast in each stream function.TryTakeAsyncState, which only forwarded toAsyncStateRegistry.TryTake.I tested the changes with
dotnet teston the memory service and on MinIO.Are there any user-facing changes?
None.
Breaking changes
AI Usage Statement