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/vector/Vlib: fix segfault introduced with #5038 #5109

Merged
merged 1 commit into from
Feb 13, 2025

Conversation

metzm
Copy link
Contributor

@metzm metzm commented Feb 13, 2025

Fixing a small resource leak in #5038 did not test if there is actually some memory allocated, introducing a segmentation fault. This PR fixes #5038.

@metzm metzm added bug Something isn't working vector Related to vector data processing C Related code is in C labels Feb 13, 2025
@metzm metzm added this to the 8.5.0 milestone Feb 13, 2025
@metzm metzm requested a review from nilason February 13, 2025 15:55
@neteler
Copy link
Member

neteler commented Feb 13, 2025

(@ShubhamDesai please have a look)

@wenzeslaus
Copy link
Member

I don't see this in Coverity Scan or CodeQL, so it seems neither of them caught it. (Coverity Scan reports additional resource leak issues at the same location and null pointer dereferences at other places.) I'm guessing you have some code which triggered it. Do you want to create a test? CI tests seem to be a crucial part of the mix even with all the static analysis.

@ShubhamDesai
Copy link
Contributor

ShubhamDesai commented Feb 13, 2025

(@ShubhamDesai please have a look)

actually there were two lines for the check (muliple times whenever the memory is allocated)

 Fi = Vect_get_field(Map, tfield);
    if (Fi == NULL)
        G_fatal_error(_("Database connection not defined for layer %d"),
                      tfield);

this makes sure Fi is not null

if (db_get_column(driver, Fi->table, afcol, &Column) != DB_OK)
            G_fatal_error(_("Column <%s> not found in table <%s>"), afcol,
                          Fi->table);

also this would have made ensured Column is not null

there were checks for both so in any case the memory would be allocated otherwise program would exit. So i directly used free without check

But in anyways adding additional checks is good.

@metzm
Copy link
Contributor Author

metzm commented Feb 13, 2025

@ShubhamDesai These tests are only performed if (afcol != NULL) thus unconditional calling of Vect_destroy_field_info(Fi); and db_free_column(Column); can lead to segmentation faults.

You must check if a resource (memory) is really used before freeing it.

@metzm metzm merged commit 3913be0 into OSGeo:main Feb 13, 2025
26 of 27 checks passed
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 libraries vector Related to vector data processing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants