Skip to content

r.geomorphon: fix uninitialized memory access in main loop#6924

Open
sumitchintanwar wants to merge 2 commits intoOSGeo:mainfrom
sumitchintanwar:fix-r-geomorphon-uninitialized
Open

r.geomorphon: fix uninitialized memory access in main loop#6924
sumitchintanwar wants to merge 2 commits intoOSGeo:mainfrom
sumitchintanwar:fix-r-geomorphon-uninitialized

Conversation

@sumitchintanwar
Copy link
Contributor

@sumitchintanwar sumitchintanwar commented Jan 19, 2026

Summary

This PR fixes memory safety issues in r.geomorphon that caused Valgrind to report
“Conditional jump or move depends on uninitialised value(s)”, primarily for border and NULL cells.
Fixes #5770.

Problem (Before)

  • Distance and threshold variables were used without explicit initialization.
  • Border and NULL cells used continue, skipping pattern computation.
  • Output buffers later accessed an uninitialized pattern pointer, causing:
    • Undefined behavior
    • Thousands of Valgrind warnings
    • Potential crashes or invalid output

Fix (After)

  • Explicitly initialize all distance and threshold variables.
  • Replace continue logic with a clear if / else structure:
    • Border and NULL cells explicitly write NULL values to outputs.
    • Pattern-dependent logic runs only when a valid pattern is computed.
  • Ensure pattern is never dereferenced unless initialized.

Verification

  • Before: ~2550 Valgrind errors
  • After: 0 Valgrind errors
  • Correct number of border NULL cells (384 for a 50×50 raster)
  • Landform classification unchanged

Notes

This PR does not change the geomorphon algorithm.
It strictly improves memory safety and robustness.

This fixes 2550 Valgrind errors by initializing distance variables and refactoring the main loop into an if/else structure to protect border cells.
Copy link
Member

@wenzeslaus wenzeslaus left a comment

Choose a reason for hiding this comment

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

Thank you for this PR. This is generally in the right direction. There is couple of issues, mainly this changes a lot of lines without a clear relation to the original Valgrind issue.

Related to the size of the change. The code block change would be easier to review with a full test in place backing up the results. There are two regression tests in place for this tool, but they are limited in terms of what they are testing. Would you be willing to contribute new tests for this tool? Ideally, in a separate PR which would be merged before this one if it runs.

I see there are three things combined here, the Valgrind-related fix, the flow clarification, and all the other fixes (now inits, but possibly changes of scope).

Separating these and having better tests in place will make it easier to review.

Comment on lines 551 to 555
}

if (opt_output[o_ternary]->answer)
((CELL *)rasters[o_ternary].buffer)[col] =
determine_ternary(pattern->pattern);
Copy link
Member

Choose a reason for hiding this comment

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

Is this change necessary for the fix of the Valgrid issue? I agree that the change should be made, but perhaps separately from the Valgrid fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wenzeslaus What changes specifically?

@sumitchintanwar
Copy link
Contributor Author

Thank you for this PR. This is generally in the right direction. There is couple of issues, mainly this changes a lot of lines without a clear relation to the original Valgrind issue.

Related to the size of the change. The code block change would be easier to review with a full test in place backing up the results. There are two regression tests in place for this tool, but they are limited in terms of what they are testing. Would you be willing to contribute new tests for this tool? Ideally, in a separate PR which would be merged before this one if it runs.

I see there are three things combined here, the Valgrind-related fix, the flow clarification, and all the other fixes (now inits, but possibly changes of scope).

Separating these and having better tests in place will make it easier to review.

Thank you for this PR. This is generally in the right direction. There is couple of issues, mainly this changes a lot of lines without a clear relation to the original Valgrind issue.

Related to the size of the change. The code block change would be easier to review with a full test in place backing up the results. There are two regression tests in place for this tool, but they are limited in terms of what they are testing. Would you be willing to contribute new tests for this tool? Ideally, in a separate PR which would be merged before this one if it runs.

I see there are three things combined here, the Valgrind-related fix, the flow clarification, and all the other fixes (now inits, but possibly changes of scope).

Separating these and having better tests in place will make it easier to review.

@wenzeslaus Thanks for the feeback. Yes, I’m willing to contribute new tests for this tool. I’ll prepare a separate PR to improve test coverage and submit it first, then split the Valgrind fix and other changes into focused PRs as suggested.

@sumitchintanwar
Copy link
Contributor Author

sumitchintanwar commented Jan 27, 2026

@wenzeslaus The tests are added in PR #6981. I’d appreciate your review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C Related code is in C module raster Related to raster data processing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants