Skip to content

Fix "one before begin of" pointer formation #237

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 11, 2025

Conversation

SoapZA
Copy link
Collaborator

@SoapZA SoapZA commented Apr 14, 2025

Fixes #233

* Forming a pointer "one before" the start of a range is UB:
  https://devblogs.microsoft.com/oldnewthing/20211112-00/?p=105908
* GCC 13's UBSAN detects this now:
  https://gcc.gnu.org/cgit/gcc/commit?id=28896b38fabce8
* By using a (signed) index, we can avoid forming invalid pointers.

Fixes #233
@SoapZA
Copy link
Collaborator Author

SoapZA commented Apr 14, 2025

Copy link
Contributor

@blawrence-ont blawrence-ont left a comment

Choose a reason for hiding this comment

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

LGTM, but are the changes necessarily required in myersCalcEditDistanceNW()? I don't immediately see where the pointer arithmetic can break down in that function.

Also I don't think that me approving this PR does anything really...

@SoapZA
Copy link
Collaborator Author

SoapZA commented Apr 14, 2025

I just did it for consistency, but yes, that function didn't fail. Using indices is a bit debug friendlier on operator[] if this code is ever switched to use std::vector

@Martinsos
Copy link
Owner

@SoapZA thanks for this -> I will have to take some time to properly study this stuff, but I will do it! Hopefully in the next weeks.
In the meantime, any additional context helps. @SoapZA , how confident are you about the changes done in this PR -> are you confident they don't change behaviour in any way, or performance?

@SoapZA
Copy link
Collaborator Author

SoapZA commented Apr 14, 2025

Logically, I'm pretty sure these don't have an impact, since changing a pointer + offset to just offset and then subscripting on the pointer doesn't change the addressing logic (and makes moving to unique_ptr<int[]> in future much easier).

Performance-wise, I haven't checked, but since the compiler can see the pointer[index] logic everywhere, it will likely optimize this away, is my guess.

@Martinsos Martinsos merged commit 05adf6b into Martinsos:master May 11, 2025
10 checks passed
@Martinsos
Copy link
Owner

Thanks @SoapZA , merged!

@Martinsos
Copy link
Owner

@SoapZA would you be interested in being added to Edlib as collaborator? You have contributed quite a bit in the last years and seem to know C/C++ quite well + you are active in the field of bioinformatics, would be great to have you involved. I have been the only maintainer from the very start and it would be good to add more people. No obligations or anything, it would just make it easier for you to contribute if you wish, and be able to do things even if I am occupied or something.

@SoapZA SoapZA deleted the fix-233 branch May 13, 2025 09:54
@SoapZA
Copy link
Collaborator Author

SoapZA commented May 13, 2025

@Martinsos that would be great!

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.

Test failures under GCC13&14
3 participants