The code should return errors instead of aborting/exitting

[disconnect3d]

Hey,

There are code paths when Unicorn calls exit() or abort() instead of returning an error, which is undesirable, especially if you consider the fact that the library is often included/pulled by various tools like GDB plugins.

One such path is when Unicorn fails to allocate a RWX memory mapping of 1GB. Instead of a graceful error return, we will get a "Could not allocate dynamic translator buffer" error print along with an exit:

https://github.com/unicorn-engine/unicorn/blob/56f3bdedb42d26bee1532cc01baf5eaf44a9aa23/qemu/accel/tcg/translate-all.c#L960-L963

Such a failure can happen for example if the user has low amount of RAM or uses a system with SELinux or custom kernel patches in place (like grsecurity) which disallows for W+X mappings. This results in a poor user experience for library users since the program that uses Unicorn crashes with a bogus error that does not say much. This is actually a case in Pwndbg currently which motivated me to create this issue.

The original "bail out and exit since allocation failed" may have been good but since the allocation may fail due to different reasons I don't think it is valid anymore.

On a long term, this should be fixed by refactoring the code so that the error can be handled by the library users. Additionally, we could also see if we can allocate with PROT_WRITE first and then changing the permissions to PROT_EXEC. However, I have seen systems that does not allow for that as well without explicit permissions.

On a short term, we could apply the following improvements: 1) The library could make a check whether RWX memory pages are supported during initialization and return appropriate error if its not 2) The errors displayed could be more explicit, detailing that a RWX memory allocation done by Unicorn failed which can either be OOM or inability to use RWX pages

This way, the library users would be able to handle this case and also would have an easier time tracking down the issue origins.

[wtdcode]

Thanks for your issue! I noticed this long time ago but lack time to improve it.

There are also a few tcg memory related issues on my TODO list and I will get them done asap.

[github-actions[bot]]

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 15 days.

[vrubleg]

It is still worthy to implement it one day in the future.

[github-actions[bot]]

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 15 days.

[vrubleg]

Still worthy to keep it.

[disconnect3d]

For what is worth, if we don't have a better solution or don't have time to implement one, we could:

1. Change exit to unicorn_abort() and initialize unicorn_abort = exit; at library load, exposing an API to change the unicorn_abort function
2. Instead of calling exit, sending a SIGSEGV or other signal to the process

:shrug:

[wtdcode]

I’m sorry to leave this long. Actually I have fixed this long time ago on dev branch but some backlog prevents me from releasing a new version. Let me try to do this in Feb.

[wtdcode]

This shall be fixed in 2.1.0, though we should have 2.0.2 =(

[wtdcode]

Oh, wait this one should be kept for returning a proper error.