fix: Add memory lifetime warnings to ReadOnlySpan properties - #406
Merged
CurtHagenlocher merged 2 commits intoAug 18, 2026
Merged
Conversation
adamreeve
reviewed
Aug 17, 2026
Contributor
|
I've removed "closes #397" from your PR description. I think we want to keep that issue open to explore other solutions. |
adamreeve
approved these changes
Aug 18, 2026
CurtHagenlocher
added a commit
that referenced
this pull request
Aug 22, 2026
…417) ### Rationale for this change The repository was split roughly in half on byte order mark usage — 272 of 539 `.cs` files carried a UTF-8 BOM. Editors and tools that save without one silently strip it, which shows up as spurious first-line changes in otherwise unrelated pull requests. #406 is a recent example: five of its six files had a BOM removed from line 1, unrelated to the change itself. Nothing enforced either convention, so the churn could go in both directions indefinitely. ### What changes are included in this PR? Three commits. **1. Strip the BOM** from all tracked `.cs`, `.csproj`, and `.json` files (275 files). Every change in that commit is exactly one line, at line 1, differing only by the removed `EF BB BF` bytes — verified with `git diff --numstat` (all `1/1`) and by confirming every removed line matches its added line once the BOM is dropped. `examples/Examples.sln` and `src/Apache.Arrow/Properties/Resources.resx` are deliberately left alone, since Visual Studio rewrites both with a BOM and stripping them would just churn back. **2. Enforce it** so this does not drift again: - `charset = utf-8` under `[*.cs]` in `.editorconfig`. The existing `format` pre-commit hook runs `dotnet format`, which honors `charset`, so this is enforced by the `Lint` CI job as-is. (`*.{csproj,props,targets}` already declared this; two `.csproj` files were quietly in violation.) - The upstream `fix-byte-order-marker` hook, to cover what `dotnet format` cannot see: files outside the solution such as `launchSettings.json`, and anything under the `src/Apache.Arrow/Flatbuf/FlatBuffers/` directory that the `format` hook excludes. Contributors who run `pre-commit install` get the check locally as well. **3. Fix a malformed ASF license header** in `Time32Array.cs` and `Time64Array.cs`, whose first line read `// Licensed to the Apache Software Foundation (ASF) under one or moreDate32Array`. This is the one content change in the PR, so it is isolated in its own commit. The typo is pre-existing — a copy-paste slip from the `Date32Array` template in 71dc961 (ARROW-16660, #13279, June 2022). It survived four years precisely because the BOM kept line 1 out of everyone's diffs; removing the BOM surfaced it immediately. These were the only two files in the repository with a malformed header line. ### Are these changes tested? Yes. - `dotnet build Apache.Arrow.sln -c Release` succeeds with 0 warnings and 0 errors. 38 `.cs` files already lacked a BOM while containing non-ASCII bytes before this PR, so Roslyn's UTF-8 default was already being exercised; removing the remaining BOMs does not change how any file decodes. - `dotnet format --verify-no-changes` exits 0 on the resulting tree, and exits 2 with `error CHARSET: Fix file encoding.` when a BOM is reintroduced. - `fix-byte-order-marker` passes on the resulting tree, and fails when a BOM is reintroduced into `launchSettings.json` or `FlatBufferBuilder.cs` — the two blind spots `dotnet format` misses. The two excluded files keep their BOMs. - Apart from the two header lines in commit 3, every file in this PR was confirmed byte-identical to `main` once the leading BOM is removed. ### Are there any user-facing changes? No. This is an encoding-only change to source files, plus a comment typo fix; no API, behavior, or build output is affected. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) <[email protected]>
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
3 participants
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.
What's Changed
Adds explicit XML documentation remarks (
<remarks>) to properties and methods that return aReadOnlySpanover managed or unmanaged memory across Apache Arrow array and buffer classes.Motivation & Relation to PR #393
When an
ArrowBufferorArrayis backed by unmanaged memory (e.g., allocated via a customMemoryAllocatororNativeMemoryManager), extracting aReadOnlySpaninto a local variable and subsequently disposing the parent buffer/array can lead to use-after-free memory access issues.While PR #393 explores structural lifecycle/pinning mechanisms, this PR provides an immediate, non-breaking developer safety improvement:
<remarks>warning callouts toArrowBuffer.Span,PrimitiveArray.Values,BooleanArray.Values, andBinaryArray.GetBytes. These surface as visible safety notes in IDE tooltips and IntelliSense (Visual Studio, VS Code, Rider) without breaking builds or requiring API breaking changes underTreatWarningsAsErrors=true.PoisonMemoryAllocatorand unit testTestNativeMemoryManagerUseAfterFreetoApache.Arrow.Teststo verify memory poisoning on buffer release.Relates to #397 and #393.