Fix 22838 - BitArray.count reads beyond data

[JohanEngelen]

This fixes issue 22838
https://issues.dlang.org/show_bug.cgi?id=22838

All hail AddressSanitizer! ;-)

[dlang-bot]

Thanks for your pull request and interest in making D better, @JohanEngelen! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

• My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
• My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
• I have provided a detailed rationale explaining my changes
• New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

---

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close	Bugzilla	Severity	Description
✓	22838	critical	std.bitmanip.BitArray.count() reads beyond data when data size is integer size_t multiple

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "stable + phobos#8401"

[thewilsonator]

Please include the test case

[JohanEngelen]

Please include the test case

The test case is already there :) The bug is triggered by a unittest (see bug report). The invalid memory access is of course only found when enabling ASan.

[CyberShadow]

The test is already there. The failure is visible only when running with AddressSanitizer, which we don't do as part of our unit tests, so there is nothing to add.

Is it possible to write a unit test which purposefully clobbers respective memory so that the failure is visible outside AddressSanitizer, though?

[CyberShadow]

I think that was the only case where _ptr[fullWords] was accessed before checking that endBits is non-zero.

[CyberShadow]

Reopening the PR with the same branch as another confused the CI I think.

[JohanEngelen]

Is it possible to write a unit test which purposefully clobbers respective memory so that the failure is visible outside AddressSanitizer, though?

No. It is an invalid read, but the calculation of bit count still works, because of the masking with endMask.

Edit:

{
    size_t[3] buffer;
    BitArray b = BitArray(buffer[], 2 * size_t.sizeof * 8);

    b[] = true;
    b[0 .. 1] = true;
    buffer[2] = ~size_t(0); // these bits will be read, but masked by endMask
    b.flip(1);
    assert(b.count() == 2 * size_t.sizeof * 8 - 1); // works
}

[maxhaton]

All hail AddressSanitizer!

I started implementing it in dmd during the Christmas holiday but then I realized it was supposed to be a holiday and stopped.

[JohanEngelen]

CI errors are not due to this PR

[kinke]

[AFAIK, CI doesn't like direct branches on the upstream repo. That's mainly intended for bigger features targeting master, e.g., where druntime and compiler both need to be patched, and where one can use same-named branches for both original repos.]