search

AFLplusplus / qemu-libafl-bridge

A patched QEMU that exposes an interface for LibAFL-based fuzzers
Other
57 stars 29 forks source link

Unset object to free memory space allocated (memory leak) #72

Closed DragonsAshes closed 4 months ago

DragonsAshes commented 4 months ago

Unset object to free memory space allocated (memory leak in fast snapshot implementation for libafl). This PR fix the memory leak. An issue about it has been created on LibAFL

rmalmain commented 4 months ago

Thank you for the PR, there is indeed a leak there. The fix looks good, but I think it would make more sense to use the qio_channel API to keep things consistent. I'll push a proposition soon with a modification in that direction.

rmalmain commented 4 months ago

Do you mind checking if this also solves the issue?

DragonsAshes commented 4 months ago

Do you mind checking if this also solves the issue?

I'll do that later today.

DragonsAshes commented 4 months ago

No this doesn't fix the memory leak. I feel like you are trying to do something with the data pointer of the structure. In our case, there is no allocation for this pointer cause we are using qio_channel_buffer_new_external() and hence we are just making it point to a location allocated before.

What need to be deallocated in our case is the QIOChannelBuffer structure. This is done by calling object_finalize(), which is called in object_unref() if the reference of the object is equal to 1. (Object_unref() is called in qemu_fclose() at the end of device_restore_all()). But in our case this reference has been incremented at the initialization of the object, but also in qemu_file_new_input(). So when we enter qemu_fclose(), the reference for the object is 2. Cause of this, we are leaving device_restore_all() without deallocating the QIOChannelBuffer.

rmalmain commented 4 months ago

No this doesn't fix the memory leak. I feel like you are trying to do something with the data pointer of the structure. In our case, there is no allocation for this pointer cause we are using qio_channel_buffer_new_external() and hence we are just making it point to a location allocated before.

It's not exactly the case. The changes you see in channel-buffer.c are unrelated to the original issue of the PR. It's just that I saw that ioc->data was not nulled in qio_channel_buffer_finalize after being free'd (it is done in qio_channel_buffer_close), so I just included it there for consistency.

What need to be deallocated in our case is the QIOChannelBuffer structure. This is done by calling object_finalize(), which is called in object_unref() if the reference of the object is equal to 1. (Object_unref() is called in qemu_fclose() at the end of device_restore_all()). But in our case this reference has been incremented at the initialization of the object, but also in qemu_file_new_input(). So when we enter qemu_fclose(), the reference for the object is 2. Cause of this, we are leaving device_restore_all() without deallocating the QIOChannelBuffer.

Yes, I know about this. What I didn't see when I wrote the external and writeback buffer channel implementation a while ago is that the call to qio_channel_close is not decreasing the reference counter (unlike for the implementation of qemu_fclose). I'm not sure it makes sense to reuse a closed channel since it cannot be reopened, though. In any case, let's keep your original implementation.