geeqie
geeqie copied to clipboard
Possible double-free and use-after-free in `FileData::file_data_free_ci`
This double-free relies on refcount state. If a FileData
has two or more references when FileData::file_data_free_ci
is called, then everything will be fine. However, if the FileData
has 1 reference when the method is called, while holding a FileDataChangeInfo
, a multiple memory errors will occur.
The codepath for the issue
- Here's
FileData::file_data_free_ci
- You can see that on line 1630, it calls
FileData::file_data_planned_change_remove
- On line 1612,
file_data_planned_change_remove
calls::file_data_unref
on theFileData
. If our refcount is 1, that will end up triggeringfile_data_free
- On line 730,
file_data_free
callsFileData::file_data_change_info_free
-
file_data_change_info_free
callsg_free
onfd->change->source
,fd->change->dest
, andfd->change
before settingfd->change
tonullptr
. - Function control eventually returns back to
FileData::file_data_free_ci
- First and foremost, you can see that
file_data_free_ci
had copied thefd->change
pointer into a local calledfdci
before it calledfile_data_planned_change_remove
. Thus, after we return from that function,fd->change == nullptr
, butfdci
retains the former value of that variable (which was alreadyg_free
d in step 5) - So when we attempt to dereference that pointer on line 1632, that is a use-after-free of
fdci
. - Also, because
fd
will have beeng_free
d at this point as well, passingfd
tofile_data_disable_grouping
will also cause use-after-free errors in that function. - Then if we make it to lines 1634–1637, we repeat
g_free
onfdci->source
,fdci->dest
, andfdci
. Each of those is a double-free. - Finally, on line 1639, we attempt
fd->change = nullptr
whenfd
has already beeng_free
d. This is a use-after-free
Possible mitigations
Because this function body relies on the FileData
continuing to exist, the easiest mitigation would be to file_data_ref
the fd
before the function starts, and to file_data_unref
the fd
before returning (which may trigger the fd
to be freed). Then, we should re-cache fd->change
after file_data_planned_change_remove
returns, and re-check whether it might be null.
Because there are multiple return points, this would be a lot easier with an RAII FileDataRef
object.