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

lib/diglib: fix integer overflow #2735

Merged
merged 2 commits into from
Jan 9, 2023
Merged

lib/diglib: fix integer overflow #2735

merged 2 commits into from
Jan 9, 2023

Conversation

metzm
Copy link
Contributor

@metzm metzm commented Jan 5, 2023

This PR fixes an integer overflow when writing out spatial index files (sidx) larger than 2GB. With the integer overflow, the resultant file size is underestimated, a wrong size for writing out file offsets is used and the spatial index file becomes unreadable. Other potentially large vector files (coordinates, topology, category index) are not affected.

@metzm metzm added bug Something isn't working backport_needed vector Related to vector data processing C Related code is in C labels Jan 5, 2023
@metzm metzm added this to the 8.2.1 milestone Jan 5, 2023
@metzm metzm requested a review from nilason January 5, 2023 17:28
Copy link
Contributor

@nilason nilason left a comment

Choose a reason for hiding this comment

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

Looks good to me!

FYI, this is listed among another 100+ similar overflow issues in the CodeQL CI check.
Out of curiosity: I assume you hit upon this as a bug (segfault or similar) running some code, was it difficult to the track this down?

@metzm
Copy link
Contributor Author

metzm commented Jan 5, 2023

FYI, this is listed among another 100+ similar overflow issues in the CodeQL CI check.

An integer overflow can happen whenever two integer values are added, subtracted, or multiplied, right? It is practically impossible to cast the operation to the next larger integer type. One exception in GRASS is, we should use grass_int64 for the number of raster cells = rows * columns.

Out of curiosity: I assume you hit upon this as a bug (segfault or similar) running some code, was it difficult to the track this down?

The error message came from G_fseek(), reason being an invalid offset argument. Once I got a sample vector to reproduce this error, it was relatively easy to trace down the bug.

@nilason
Copy link
Contributor

nilason commented Jan 5, 2023

FYI, this is listed among another 100+ similar overflow issues in the CodeQL CI check.

An integer overflow can happen whenever two integer values are added, subtracted, or multiplied, right? It is practically impossible to cast the operation to the next larger integer type. In GRASS, we should do this for the number of raster cells = rows * columns.

The issues reported by CodeQL are arithmetics (multiplication) on smaller types which result is stored to a larger type. This reflects the programmers expectations, e.g.:

size_t size;
int a, b, c;

size = a * b; // in this case multiplication precedes the (implicit) cast and its result may be overflowed

size = (size_t)a * b; // the cast precedes the multiplication, no problems

c = a * b; // the programmer don't expect larger result, no problems

This is why the CodeQL issues only are numbered in the hundreds (and not thousands).

Out of curiosity: I assume you hit upon this as a bug (segfault or similar) running some code, was it difficult to the track this down?

The error message came from G_fseek(), reason being an invalid offset argument. Once I got a sample vector to reproduce this error, it was relatively easy to trace down the bug.

Good to hear. Once I, or someone else, manage to fix those relatively simply fixed CodeQL issues, another potential similar bug to this will be avoided.

@metzm metzm merged commit 4e4e38f into OSGeo:main Jan 9, 2023
@metzm metzm deleted the vector_lfs branch January 9, 2023 16:12
@metzm metzm mentioned this pull request Jan 9, 2023
metzm added a commit to metzm/grass that referenced this pull request Jan 9, 2023
metzm added a commit that referenced this pull request Jan 9, 2023
ninsbl pushed a commit to ninsbl/grass that referenced this pull request Feb 17, 2023
* fix integer type for estimating file size, avoid compiler warnings
@neteler neteler changed the title diglib: fix integer overflow lib/diglib: fix integer overflow Jun 6, 2023
neteler pushed a commit to nilason/grass that referenced this pull request Nov 7, 2023
* fix integer type for estimating file size, avoid compiler warnings
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working C Related code is in C vector Related to vector data processing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants