Fix OOB read in samplePerlinBeta17Terrain and add Beta 1.7 regression…#164
Open
lzray-universe wants to merge 1 commit into
Open
Fix OOB read in samplePerlinBeta17Terrain and add Beta 1.7 regression…#164lzray-universe wants to merge 1 commit into
lzray-universe wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes an out-of-bounds permutation-table read in Beta 1.7 terrain Perlin sampling and adds Beta 1.7 biome checks intended to guard determinism and validity.
Changes:
- Wraps intermediate Perlin permutation indices to 8 bits in
samplePerlinBeta17Terrain(). - Adds Beta 1.7 determinism, regression-hash, and biome-ID validity checks to
tests.c. - Wires the new checks into the test executable’s
main().
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
noise.c |
Masks intermediate Beta 1.7 Perlin indices to keep table accesses in bounds. |
tests.c |
Adds Beta 1.7 biome determinism/regression/validity checks and invokes them from main(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+557
to
+561
| printf(" B1.7 regression hash: %08x\n", hash); | ||
|
|
||
| static volatile uint32_t vhash; | ||
| vhash = hash; | ||
| return 0; |
Comment on lines
+641
to
648
| printf("Testing Beta 1.7 biome determinism:\n"); | ||
| testBeta17BiomeDeterminism(); | ||
| printf("Testing Beta 1.7 biome regression:\n"); | ||
| testBeta17BiomeRegression(); | ||
| printf("Testing Beta 1.7 biome validity:\n"); | ||
| testBeta17BiomeValidity(); | ||
|
|
||
| return 0; |
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
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.
Summary
This PR fixes an out-of-bounds read in
samplePerlinBeta17Terrain()and adds some regression tests for Beta 1.7 biome determinism.The bug was caused by intermediate Perlin permutation indices being kept as
int, but not wrapped back to 8 bits. So values likeidx[i1] + i2could go past the valid range of the 257-entry permutation tabled[256 + 1].This was a pretty annoying bug because it could look like a random determinism issue, especially with different stack layouts / build modes.
Root cause
PerlinNoise::donly has valid indices[0, 256], whered[256]mirrorsd[0].But in
samplePerlinBeta17Terrain(), the intermediate indices were computed like this:Since
idx[...],i1,i2, andi3can all be in[0, 255], expressions such asidx[i1] + i2may reach510. Then later reads likeidx[a1],idx[a1+1],idx[b1], andidx[b1+1]can go beyond the end ofd.Other Perlin sampling paths already use
uint8_tintermediates, or otherwise get the same 8-bit wraparound behavior. So this issue seems specific tosamplePerlinBeta17Terrain().Fix
Mask the intermediate permutation indices with
& 0xff, matching the intended 8-bit wrapping:So all accesses now stay inside
[0, 256].About the seed=262 instability
The previously observed Beta 1.7 instability around
seed=262was not really a seperate uninitialized-variable bug. It was a symptom of this out-of-bounds read.I also checked the suspected locals:
climate[2]is filled bysampleBiomeNoiseBeta()before use.cols[2]is filled byprocessColumnNoise()before use.SeaLevelColumnNoiseBeta colNoiseis initialized bygenColumnNoise()before use.SurfaceNoiseBeta snbis initialized byinitSurfaceNoiseBeta()before use.Value-initializing these locals can make the result look fixed, but that only changes the nearby stack memory which the invalid read was accidentally looking at. So it hides the real problem...
Some evidences
I used an instrumented version of
samplePerlinBeta17Terrain()to check everyidx[...]access.Before the fix, the test triggered a lot of out-of-bounds reads. For example, with
seed=0, 1076 out of 1200 sampled calls accessed indices outside the valid range, and the observed indices reached at least 408.After the fix, the same instrumentation reports:
Tests
This also adds Beta 1.7 regression coverage:
testBeta17BiomeDeterminism()seed=262biome query multiple times at several scales.testBeta17BiomeRegression()testBeta17BiomeValidity()0..255range.