Skip to content
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

Fix appender code that might not initialize memory that might point at huge allocations #9084

Merged
merged 2 commits into from
Nov 14, 2024

Conversation

schveiguy
Copy link
Member

Not going to reproduce all the junk I did in the bug report. See the report.

pointers in unallocated space, thereby pinning unused memory.
@dlang-bot
Copy link
Contributor

Thanks for your pull request, @schveiguy!

Bugzilla references

Auto-close Bugzilla Severity Description
24856 normal std.array.Appender.ensureAddable can create stale memory references

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#9084"

@schveiguy
Copy link
Member Author

switching to draft, need to add a test.

@schveiguy schveiguy changed the title Fix appender code that might not initialize memory that might point at huge allocations Draft: Fix appender code that might not initialize memory that might point at huge allocations Nov 14, 2024
@schveiguy schveiguy marked this pull request as draft November 14, 2024 03:59
@schveiguy schveiguy changed the title Draft: Fix appender code that might not initialize memory that might point at huge allocations Fix appender code that might not initialize memory that might point at huge allocations Nov 14, 2024
@schveiguy
Copy link
Member Author

well, I guess there isn't any sane testing harnesses I can incorporate here, and I don't want to add one for this. So it will have to go in with existing tests.

@schveiguy schveiguy marked this pull request as ready for review November 14, 2024 04:12
@jmdavis
Copy link
Member

jmdavis commented Nov 14, 2024

well, I guess there isn't any sane testing harnesses I can incorporate here, and I don't want to add one for this. So it will have to go in with existing tests.

Wouldn't a test to make sure that the elements were zeroed out suffice?

@LightBender
Copy link
Contributor

LightBender commented Nov 14, 2024

I'm inclined to agree with @jmdavis. Given the fragile nature of the proof-test and that it requires a whole-program testing framework that does not currently exist in Phobos, I would think that a test that ensures that the desired "correct state" is achieved should be sufficient for our purposes. Such a test would prevent regressions which is mostly what we're after.

@schveiguy
Copy link
Member Author

I can try and add a unittest that attempts to cause the problem to happen. Let's see if I can get it to fail with the original code though.

@schveiguy
Copy link
Member Author

schveiguy commented Nov 14, 2024

OK, a test is added. I added a printout for the log in case the test isn't properly run. FWIW, when I tested locally without the fix, the test failed.

Copy link
Contributor

@thewilsonator thewilsonator left a comment

Choose a reason for hiding this comment

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

otherwise looks good

std/array.d Outdated Show resolved Hide resolved
@thewilsonator thewilsonator merged commit 567b375 into dlang:stable Nov 14, 2024
10 checks passed
@schveiguy schveiguy deleted the fixappender branch November 15, 2024 01:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants