Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
PhysSong opened this issue May 4, 2020 · 8 comments · Fixed by #49
Closed

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

PhysSong opened this issue May 4, 2020 · 8 comments · Fixed by #49
Labels
bug Something isn't working

Comments

@PhysSong
Copy link

PhysSong commented May 4, 2020

Original report: LMMS/lmms#5483
In the function ipatch_dls_nullify_fixups:

ipatch_container_init_iter((IpatchContainer *)(reader->dls), &inst_iter,
reader->is_gig ? IPATCH_TYPE_GIG_INST
: IPATCH_TYPE_DLS2_INST);
inst = ipatch_dls2_inst_first(&inst_iter);

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
Copy link
Contributor

jjceresa commented May 5, 2020

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 Fluidsynth making use of libinstpatch support. Is is right ?

@derselbst derselbst added the bug Something isn't working label May 5, 2020
@PhysSong
Copy link
Author

PhysSong commented May 5, 2020

Both of them are right.

@derselbst
Copy link
Member

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
Copy link
Contributor

jjceresa commented May 6, 2020

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
Copy link
Member

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
Copy link
Contributor

jjceresa commented May 6, 2020

Ok.

@jjceresa
Copy link
Contributor

jjceresa commented May 6, 2020

@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
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants