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

Fixed runtime crash #58

Merged
merged 1 commit into from
Jan 30, 2025
Merged

Fixed runtime crash #58

merged 1 commit into from
Jan 30, 2025

Conversation

aamirglb
Copy link
Contributor

Instead of manually freeing the memory allocated by libpostal_expand_address(), use libpostal_expansion_array_destroy() to free the memory.

This is how example program from libpostal frees the memory. For more details, please refer to this issue:
openvenues/libpostal#682

Instead of manually freeing the memory allocated by `libpostal_expand_address()`, use `libpostal_expansion_array_destroy()` to free the memory.

This is how example program from libpostal frees the memory.
For more details, please refer to this issue:
openvenues/libpostal#682
@missinglink
Copy link
Contributor

missinglink commented Jan 29, 2025

Thanks @aamirglb, I'm happy to merge this.

Could you please explain to me (as a non-C developer) what is the difference between what was there before:

392ac3a

and this?

https://github.com/openvenues/libpostal/blob/9e5af6b044295f147e21f8586d1c79a1d0c8914e/src/expand.c#L1650-L1655

ref:

https://github.com/openvenues/libpostal/blob/9e5af6b044295f147e21f8586d1c79a1d0c8914e/src/libpostal.c#L62

The both look the same to me:

loop {
  free(expansions[i]);
}
free(expansions);

@albarrentine
Copy link
Contributor

Fundamentally the two shouldn’t be different unless the implementation of new() in V8 is accessing the memory after it returns.

This is cleaner/preferred though and we may at some point change the implementation to use either a custom allocator or a cstring_array which stores all the strings continuously in memory and requires 2 frees and incurs potentially less memory fragmentation for tiny strings. Overall better to use the API function.

@aamirglb
Copy link
Contributor Author

@missinglink In my particular case, I compiled the libpostal using MSYS2-MinGW64, which uses g++ compiler. And I'm trying to use the generated DLL on Windows OS with VC++ compiler.

Using the libpostal dll with Pelias Interpolation Service causes the interpolation service to crash. I suspect, the main issue could be difference in runtime between MSYS2 and VC++, but I'm not sure. It required a more detailed debugging.

While debugging interpolation service crash, I noticed that the crash happens when expand.cc tries to free the memory. After looking at the example programs provided by libpostal, I noticed this API which frees the allocated memory and updated expand.cc to use the API. After this update, Interpolation service works without any issue.

Also it is not a good practice to manually free a pointer returned by a library unless the library documentation explicitly recommend it.

cc: @albarrentine

@albarrentine albarrentine merged commit c2bd952 into openvenues:master Jan 30, 2025
10 checks passed
missinglink added a commit that referenced this pull request Jan 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants