search

swami / libinstpatch

Instrument file software library.
Other
20 stars 6 forks source link

Crash on cleaning up a DLS reader after trying to read a invalid file #48

Closed PhysSong closed 4 years ago

PhysSong commented 4 years ago

Original report: https://github.com/LMMS/lmms/issues/5483 In the function ipatch_dls_nullify_fixups: https://github.com/swami/libinstpatch/blob/a1af56bb064279f305aedf062597b700c2811f79/libinstpatch/IpatchDLSReader.c#L531-L534 ipatch_container_init_iter may fail and inst_iter will be invalid in that case. However, the function is using inst_iter without checking for errors. In the reported case, FluidSynth tried to load a directory as a DLS file. However, it may happen for corrupted files as well.

jjceresa commented 4 years ago

However, the function is using inst_iter without checking for errors.

Ok, please, just to clarify the issue you observed: 1) we are here in the context of ipatch_dls_reader_finalize() calling ipatch_dls_nullify_fixups() that crashes because the file read by IpatchDlSReader is corrupted. Is is right ?. 2) this issue occurs when using Fluidsynthmaking use of libinstpatch support. Is is right ?

PhysSong commented 4 years ago

Both of them are right.

derselbst commented 4 years ago

We need to check the return value of ipatch_container_init_iter() everywhere. Sounds fun.

@jjceresa Do you want to do that? Otherwise I would start with that on the upcoming weekend.

jjceresa commented 4 years ago

We need to check the return value of ipatch_container_init_iter() everywhere.

I guess we must call g_return_val_if_fail() or g_return_val_fail() should be preferable rather that check and return silently, isn't it ?

Do you want to do that?

Yes I will do, in the whole library.

derselbst commented 4 years ago

I don't think it's necessary to wrap ipatch_container_init_iter() in an g_return_val_if_fail(), because if ipatch_container_init_iter() fails, it will have already told us, like:

CRITICAL **: 17:13:11.946: ipatch_container_init_iter: assertion 'IPATCH_IS_CONTAINER(container)' failed

So there is no need to add g_return_val_if_fail() to callers of ipatch_container_init_iter.

jjceresa commented 4 years ago

Ok.

jjceresa commented 4 years ago

@PhysSong , A fix to this issue is done in branch iter (PR #49). Please, it would be great if you could test this with the relevant corrupted file.

derselbst commented 4 years ago

Thank you @PhysSong for the report and @jjceresa for the fix!