[go: nahoru, domu]

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

gp_{camera_folder,filesystem}_put_file leak memory on fd & handler input files #837

Open
RReverser opened this issue Sep 25, 2022 · 1 comment

Comments

@RReverser
Copy link
Contributor

gp_file_get_data_and_size docs state:

 * For regular CameraFiles, the pointer to data that is returned is
 * still owned by libgphoto2 and its lifetime is the same as the #file.
 *
 * For filedescriptor or handler based CameraFile types, the returned
 * data pointer is owned by the caller and needs to be free()d to avoid
 * memory leaks.

However, looking at all the usages of gp_file_get_data_and_size, I found that a lot of them - pretty much all of put_file_func implementations, as well as few others - just assume that file is a "regular" in-memory file and don't free the retrieved data if it's actually a GP_FILE_ACCESSTYPE_FD or GP_FILE_ACCESSTYPE_HANDLER kind of file.

Unless I'm missing something, this means that uploading files created from gp_file_new_from_fd or gp_file_new_from_handler via gp_{camera_folder,filesystem}_put_file will always leak memory.

I walked through the list of gp_file_get_data_and_size occurences in the codebase and tried to limit it to those that exhibit this issue (but I might have missed a few):

CHECK (gp_file_get_data_and_size (file, (const char **)&filedata,

gp_file_get_data_and_size (file, &data, &size);

CR (gp_file_get_data_and_size (file, &d, &d_len));

gp_file_get_data_and_size(file, &data, &datasize);

gp_file_get_data_and_size(file, &d, &len);

gp_file_get_data_and_size (file, &data, &size);

gp_file_get_data_and_size (file, &data, &size);

gp_file_get_data_and_size (file, &data, &size);

if (gp_file_get_data_and_size (file, (const char**)&filedata, &filesize) < GP_OK)

gp_file_get_data_and_size (file, (const char**)&object, &intsize);

CR (gp_file_get_data_and_size (file, &data, &size));

CHECK (gp_file_get_data_and_size (file, &data, &data_size));

CHECK (gp_file_get_data_and_size (file, &data, &size));

CHECK (gp_file_get_data_and_size (file, &data_file, &data_size));

gp_file_get_data_and_size

gp_file_get_data_and_size (file, &data_file, &data_size);

ret = gp_file_get_data_and_size (file, (const char **)&filedata,

CHECK (gp_file_get_data_and_size (file, (const char **)&filedata,

To be fair, I think this is an easy mistake to make for downstream consumers as well, since rules around ownership of the data returned from gp_file_get_data_and_size are confusing and depend on how the file was created. To makes matters worse, downstream consumers have no way of knowing how said file was created since CameraFile::accesstype is not exposed via getters, so users can't know who the data belongs to unless they were the ones to create the file too.

Aside from fixing those usages, I think it would be better to deprecate this function and instead provide one that always stores a data pointer internally, and consistently frees it on gp_file_free.

@msmeissn
Copy link
Contributor

I need to go over it when i have some quiet hours. :/
I cannot easily get rid of this old API and its memory ownage pattern is weird as you saw. Usually the library always "owns" the memory.

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

No branches or pull requests

2 participants