You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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.
The text was updated successfully, but these errors were encountered:
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.
gp_file_get_data_and_size
docs state:However, looking at all the usages of
gp_file_get_data_and_size
, I found that a lot of them - pretty much all ofput_file_func
implementations, as well as few others - just assume thatfile
is a "regular" in-memory file and don't free the retrieved data if it's actually aGP_FILE_ACCESSTYPE_FD
orGP_FILE_ACCESSTYPE_HANDLER
kind of file.Unless I'm missing something, this means that uploading files created from
gp_file_new_from_fd
orgp_file_new_from_handler
viagp_{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):libgphoto2/camlibs/ax203/library.c
Line 231 in cf154a3
libgphoto2/camlibs/canon/serial.c
Line 964 in cf154a3
libgphoto2/camlibs/fuji/library.c
Line 223 in cf154a3
libgphoto2/camlibs/kodak/dc210/library.c
Line 64 in cf154a3
libgphoto2/camlibs/konica/qm150.c
Line 521 in cf154a3
libgphoto2/camlibs/minolta/dimagev/upload.c
Line 54 in cf154a3
libgphoto2/camlibs/panasonic/dc1000.c
Line 442 in cf154a3
libgphoto2/camlibs/panasonic/dc1580.c
Line 563 in cf154a3
libgphoto2/camlibs/ptp2/library.c
Line 8339 in cf154a3
libgphoto2/camlibs/ptp2/library.c
Line 9078 in cf154a3
libgphoto2/camlibs/ricoh/library.c
Line 275 in cf154a3
libgphoto2/camlibs/sierra/library.c
Line 1443 in cf154a3
libgphoto2/camlibs/sierra/sierra.c
Line 601 in cf154a3
libgphoto2/camlibs/sierra/sierra.c
Line 754 in cf154a3
libgphoto2/camlibs/sonydscf55/sony.c
Line 785 in cf154a3
libgphoto2/camlibs/soundvision/soundvision.c
Line 400 in cf154a3
libgphoto2/camlibs/st2205/library.c
Line 304 in cf154a3
libgphoto2/camlibs/tp6801/library.c
Line 226 in cf154a3
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 fromgp_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 sinceCameraFile::accesstype
is not exposed via getters, so users can't know who thedata
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
.The text was updated successfully, but these errors were encountered: