libnfc icon indicating copy to clipboard operation
libnfc copied to clipboard

Fixed memory leak in nfc_initiator_select_passive_target()

Open martin-vitek opened this issue 4 years ago • 4 comments

martin-vitek avatar Sep 26 '20 16:09 martin-vitek

I checked my app, which is using libnfc, with Valgrind and it discovered, that function nfc_initiator_select_passive_target() is leaking 12B.

https://github.com/nfc-tools/libnfc/blob/d9a04a54ffd99348cbbbf7c2e5e4747c2b8fe404/libnfc/nfc.c#L576-L591

Problem is in calling macro HAL before free(), because the macro is returning from nfc_initiator_select_passive_target(), so the free() afterwars is never called. And in case nm.nmt == NMT_ISO14443A, abtTmpInit isn't deallocated.

But as I'm looking at it now, my solution isn't good, because abtInit is used after my added free() by iso14443_cascade_uid(). And it is used as abtInit, which is pointing to deallocated address, also by macro HAL and initiator_select_passive_target.

martin-vitek avatar Sep 27 '20 08:09 martin-vitek

Ah, I see… thanks you for the context, I did not checked in that area. Returning from a macro is never a good idea… We can either do some big refactoring to avoid this macro completely, or a short-term easier fix would be to call the macro from another internal function, save the returned value, free memory and return the saved value.

smortex avatar Sep 27 '20 21:09 smortex

Yeah, reworking the HAL macro would be a good idea, but as a much easier fix, I simply replaced malloc() with alloca(), so the memory is allocated on the stack and automatically deallocated when exiting the function. As the size of the allocated memory is small, use of alloca() should be fine.

martin-vitek avatar Oct 16 '20 12:10 martin-vitek

Hum :thinking: I am concerned about portability here… I don't know if many people use the libnfc code in embedded systems, but they might not have an alloca() function available at hand. @nfc-tools/core though?

smortex avatar Oct 19 '20 17:10 smortex