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

RReverser commented 2 years ago

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):

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 commented 2 years ago

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.

Sija commented 3 hours ago

@msmeissn Would you have time to revisit this?

gphoto / libgphoto2

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