Unmatched malloc/free in `emulator_delete`

[paladin-t]

In the latest revision in function emulator_delete, file_data_delete is called to release ROM data.

binjgb/src/emulator.c

Lines 5050 to 5056 in 1e67a75

	void emulator_delete(Emulator* e) {
	if (e) {
	xfree(e->audio_buffer.data);
	file_data_delete(&e->file_data);
	xfree(e);
	}
	}

The data is created outside the emulator, but it's released inside it, which causes unmatched malloc/free. There should be a way that allows a programmer to indicate whether the memory of ROM data is managed by emulator or its caller.

[binji]

It's a bit weird I suppose, but the way it currently works is that the ROM data ownership is passed to the emulator. Are you using this in some way where you want to retain ownership of the ROM data after the emulator is destroyed?

[paladin-t]

The application layer is written in C++. The emulator per se and the ROM are both retained by a wrapper class. In particular the ROM data is created and owned as a member field like std::shared_ptr<Bytes> rom = std::shared_ptr<Bytes>(new Bytes(...)). They will be both unused and released at a same time; so the problem is not a ROM lives longer than an emulator instance, it is just I hope that ROM is released consistently with it's creating.

[binji]

Unfortunately that's not a valid strategy with the most recent changes, since the emulator may resize the rom data (see e.g.

binjgb/src/emulator.c

Line 1196 in 1e67a75

file_data_resize(&e->file_data, max_file_size);

). You really should assume that when you call emulator_new() that ownership of the ROM data has been passed to Emulator.

[paladin-t]

Ok. Then I think it's better if there were another initialization workflow that duplicates input ROM data inside emulator. In short, an emulator who is in charge of freeing should be also in charge of allocating the space for ROM data.

[binji]

At least for the ways I use emulator_new(), it works best to pass ownership. The calling code has no need for the ROM data after it's been loaded. And similarly, it doesn't make sense for the emulator itself to allocate the space since it doesn't know how much it will need; the ROM data could be coming from a file on disk, or generated automatically in memory, or anywhere else.

If you want your calling code to retain ownership the best way is to make a copy yourself to give to the emulator, rather than having an additional option in EmulatorInit to do this.

[paladin-t]

I see. I'm ok with that.