Skip to content

Enable ASAN on CI and fix a null-termination bug#246

Merged
urschrei merged 2 commits into
georust:mainfrom
yutannihilation:ci/sanitizer
Feb 4, 2026
Merged

Enable ASAN on CI and fix a null-termination bug#246
urschrei merged 2 commits into
georust:mainfrom
yutannihilation:ci/sanitizer

Conversation

@yutannihilation
Copy link
Copy Markdown
Contributor

  • I agree to follow the project's code of conduct.
  • I added an entry to the project's change log file if knowledge of this change could be valuable to users.
    • Usually called CHANGES.md or CHANGELOG.md
    • Prefix changelog entries for breaking changes with "BREAKING: "

During investigating the Windows build, I found there's a memory corruption bug. This pull request is to prove it is a real bug in other OSes than Windows as well. Note that I'm not an expert of ASAN (and C++ in general), so I'm not sure if these compiler flags are exactly the best for our purpose.

This is the result when I ran this on my forked repo:

https://github.com/yutannihilation/proj/actions/runs/21554286154/job/62107945847#step:5:129

I also confirmed 1c9bc45 fixes this error.

@urschrei
Copy link
Copy Markdown
Member

urschrei commented Feb 1, 2026

I'm happy for this to merge – I don't know a huge amount about ASAN but the setup seems reasonable:

  • using the bundled option to ensure a source compilation with ASAN
  • Have to use a Nightly for -Z sanitizer=address

And the fix itself seems good – we already do something similar in proj_create_crs_to_crs_from_pj and proj_as_projjson

(A small nit: please squash these commits before we merge)

@weiznich @lnicola @michaelkirk does anyone else want to weigh in?

@yutannihilation yutannihilation changed the title [PoC] Enable ASAN on CI Enable ASAN on CI and fix a null-termination bug Feb 1, 2026
@yutannihilation
Copy link
Copy Markdown
Contributor Author

Thanks, I squashed the commits.

Copy link
Copy Markdown
Contributor

@weiznich weiznich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's reasonable to apply asan this way. That's what I also do for other projects so 👍 from me

@urschrei urschrei added this pull request to the merge queue Feb 4, 2026
Merged via the queue into georust:main with commit 81f2a34 Feb 4, 2026
29 checks passed
@yutannihilation yutannihilation deleted the ci/sanitizer branch February 4, 2026 20:25
@michaelkirk michaelkirk mentioned this pull request Feb 5, 2026
2 tasks
github-merge-queue Bot pushed a commit that referenced this pull request Feb 6, 2026
- [x] I agree to follow the project's [code of
conduct](https://github.com/georust/.github/blob/main/CODE_OF_CONDUCT.md).
- [x] I added an entry to the project's change log file if knowledge of
this change could be valuable to users.
  - Usually called `CHANGES.md` or `CHANGELOG.md`
  - Prefix changelog entries for breaking changes with "BREAKING: "
---

Follow up to #246 

LLM was used to help with docs.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants