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

Memory leak fix in curl bindings #1856

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

Conversation

trnzk
Copy link

@trnzk trnzk commented Nov 2, 2024

No description provided.

Copy link
Member

@dimensionscape dimensionscape left a comment

Choose a reason for hiding this comment

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

Yeah this makes sense. Thanks!

@trnzk
Copy link
Author

trnzk commented Nov 2, 2024

Issue 2313 might be related to this.

@barisyild
Copy link
Contributor

barisyild commented Nov 13, 2024

Issue 2313 might be related to this.

@trnzk Unfortunately there is a leak in the png implementation :(

I took this image from my 5 minute game session.

The functions that cause leakage are as follows;

png_write_rows (png_ptr, &row, 1);

png_structp png_ptr = png_create_write_struct (PNG_LIBPNG_VER_STRING, NULL, user_error_fn, user_warning_fn);

png_infop info_ptr = png_create_info_struct (png_ptr);

image

@barisyild
Copy link
Contributor

Issue 2313 might be related to this.

@trnzk Unfortunately there is a leak in the png implementation :(

I took this image from my 5 minute game session.

The functions that cause leakage are as follows;

png_write_rows (png_ptr, &row, 1);

png_structp png_ptr = png_create_write_struct (PNG_LIBPNG_VER_STRING, NULL, user_error_fn, user_warning_fn);

png_infop info_ptr = png_create_info_struct (png_ptr);

image

Sorry for misinformation, this leak is related to png encode, probably curl fix will fix the leak mentioned.

@barisyild
Copy link
Contributor

barisyild commented Nov 15, 2024

_ No description provided. _

My tests still confirm a memory leak.

lime::header_callback(void*, unsigned long, unsigned long, void*)
chop_write
Curl_client_write
Curl_http_readwrite_headers
readwrite_data
Curl_readwrite
multi_runsingle
curl_multi_perform
lime::lime_curl_multi_perform(_value*)
lime::net::curl::CURLMulti_obj::perform()

@barisyild
Copy link
Contributor

_ No description provided. _

My tests still confirm a memory leak.

lime::header_callback(void*, unsigned long, unsigned long, void*) chop_write Curl_client_write Curl_http_readwrite_headers readwrite_data Curl_readwrite multi_runsingle curl_multi_perform lime::lime_curl_multi_perform(_value*) lime::net::curl::CURLMulti_obj::perform()

Please remove following lines to fix memory leak.

values->clear ();

values->clear ();

@trnzk
Copy link
Author

trnzk commented Nov 15, 2024

Removing these lines will change the purpose of the function; it would be more appropriate to use the free_header_values function before 'clear'. I don’t have the opportunity to test it right now, but if you try it this way and let me know the result, I’d appreciate it.

@barisyild
Copy link
Contributor

Removing these lines will change the purpose of the function; it would be more appropriate to use the free_header_values function before 'clear'. I don’t have the opportunity to test it right now, but if you try it this way and let me know the result, I’d appreciate it.

Changing these lines does not affect the purpose or behavior of the function, in fact it creates the behavior it should have.

If you empty the vector, the free_header_values function is useless because it always tries to freeing the empty vector.

also writing free under alloc_string doesn't work because alloc_string is just casting and the same pointer will still be used.

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.

4 participants