Fixed memory leak in nfc_initiator_select_passive_target()

[martin-vitek]

No description provided.

[smortex]

This code looks convoluted, maybe I read it badly but I do not thing this fix improve the situation.

[smortex]

Here is now a use-after-free for abtInit (which is assigned the address of abtTmpInit which is freeed).

[smortex]

What are you trying to fix? A double-free error? If so, I feel like this should be the only free() after nfc_device_validate_modulation() returns successfuly.

[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.

libnfc/libnfc/nfc.c

Lines 576 to 591 in d9a04a5

	if (szInitData == 0) {
	// Provide default values, if any
	prepare_initiator_data(nm, &abtInit, &szInit);
	free(abtTmpInit);
	} else if (nm.nmt == NMT_ISO14443A) {
	abtInit = abtTmpInit;
	iso14443_cascade_uid(pbtInitData, szInitData, abtInit, &szInit);
	} else {
	abtInit = abtTmpInit;
	memcpy(abtInit, pbtInitData, szInitData);
	free(abtTmpInit);
	szInit = szInitData;
	}
	HAL(initiator_select_passive_target, pnd, nm, abtInit, szInit, pnt);
	free(abtTmpInit);

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.

[smortex]

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.

[martin-vitek]

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.

[smortex]

Hum 🤔 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?