Skip to content

lib/arraystats: re-enable "Discont" algorithm #5529

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 3 commits into from
Apr 23, 2025

Conversation

nilason
Copy link
Contributor

@nilason nilason commented Apr 11, 2025

Addresses heap buffer overflow in previously disabled 'Discont' algorithm.

The immediate cause was too small allocation of variable num in AS_class_discont(), which was fixed by an increase of its size (broadly mirroring the original Fortran code).

This fix was complemented by some code related improvements:

  • Code clean up and restructure of AS_class_discont()
  • Re-enable "dis" algorithm in v.class and d.vect.thematic
  • Pass classbreaks array by pointer to pointer to enable in- and output.

For this PR I had the original Fortran code at my disposal.

See: https://lists.osgeo.org/pipermail/grass-dev/2008-July/038951.html

Fixes: #5486

@nilason nilason added this to the 8.5.0 milestone Apr 11, 2025
@github-actions github-actions bot added vector Related to vector data processing C Related code is in C libraries module display labels Apr 11, 2025
@nilason nilason force-pushed the fix_arraystats_discont branch from 7d89f25 to 19ed7ef Compare April 12, 2025 12:00
@nilason
Copy link
Contributor Author

nilason commented Apr 12, 2025

I have compared the results of the updated/fixed "discontinuities" algorithm against the results from the original Fortran code and they are consistent. I don't have the knowledge to evaluate the algorithm as such but it works as intended (with reference to the original author). So, this part will probably be difficult to review.

However, I'd be happy to receive review on the general workings of v.class and d.vect.thematic (and of course the code itself), so to not end up with any regression.

@mlennert A big thank you for your assistance, which was crucial for this fix.

@nilason nilason force-pushed the fix_arraystats_discont branch from 56393d5 to 5cb06bd Compare April 13, 2025 12:22
Addresses heap buffer overflow in previously disabled 'Discont'
algorithm.

The immediate cause was too small allocation of variable `num` in
`AS_class_discont()`, which was fixed by an increase of its size
(broadly mirroring the original Fortran code).

This fix was complemented by some code related improvements:

- Code clean up and restructure of `AS_class_discont()`
- Re-enable "dis" algorithm in v.class and d.vect.thematic
- Pass `classbreaks` array by pointer to pointer to enable in- and
  output.

See: https://lists.osgeo.org/pipermail/grass-dev/2008-July/038951.html

Fixes: OSGeo#5486
@nilason nilason force-pushed the fix_arraystats_discont branch from 5cb06bd to 7d42ec3 Compare April 13, 2025 13:30
@echoix
Copy link
Member

echoix commented Apr 14, 2025

Was the Fortran source available publicly? If so, can you link it here for perennity?

echoix
echoix previously approved these changes Apr 14, 2025
Copy link
Member

@echoix echoix left a comment

Choose a reason for hiding this comment

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

Approving here in order to not block you, if it never gets reviewed thoroughly. I don't see how I could find something wrong with the process you used to fix that old time issue.

@nilason
Copy link
Contributor Author

nilason commented Apr 17, 2025

Was the Fortran source available publicly?

To my knowledge, it is not.

@nilason
Copy link
Contributor Author

nilason commented Apr 22, 2025

If there are no objections I'll go ahead and merge this soon. I'm fairy confident this will not break anything, so let it be tested live.

@nilason
Copy link
Contributor Author

nilason commented Apr 23, 2025

This will need a renewed approval, I did some (foremost) stylistic changes.

@echoix
Copy link
Member

echoix commented Apr 23, 2025

What does const in C guarantee? I remember reading that is doesn't prevent to still change the data given as input, but it still gives the expected usage.

Are the changes in the function signature insignificant for the public API/ABI? Does a note need to be prepared for the next release, or these changes are "source-compatible", ie work without changes to code using it, just rebuild?

@nilason
Copy link
Contributor Author

nilason commented Apr 23, 2025

What does const in C guarantee? I remember reading that is doesn't prevent to still change the data given as input, but it still gives the expected usage.

It informs both compiler and user (see const type). Compiler may enforce, and even optimise.

Are the changes in the function signature insignificant for the public API/ABI? Does a note need to be prepared for the next release, or these changes are "source-compatible", ie work without changes to code using it, just rebuild?

API is practically the same, in the unlikely case the library is used in any addon it shouldn’t be any issue.

@nilason nilason merged commit 4564a09 into OSGeo:main Apr 23, 2025
28 checks passed
@nilason
Copy link
Contributor Author

nilason commented Apr 23, 2025

Thanks Edouard!

@nilason nilason deleted the fix_arraystats_discont branch May 9, 2025 12:45
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 display libraries module vector Related to vector data processing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] lib/arraystats: heap buffer overflow in disabled 'Discont' algorithm
2 participants