search

rizinorg / rizin

UNIX-like reverse engineering framework and command-line toolset.
https://rizin.re
GNU Lesser General Public License v3.0
2.64k stars 352 forks source link

Check for `NULL` cases in `RZ_NEW*` macros. #3704

Open Rot127 opened 1 year ago

Rot127 commented 1 year ago

Is your feature request related to a problem? Please describe.

Because it is so convenient we use the RZ_NEW* macros quite a lot. But I see rarely that a check for the NULL case is added.

Describe the solution you'd like

We should add this check into the macro. Although it is pretty rare, that malloc etc. return NULL, it would still be good practice to take care of it.

We can also distinguish the ENOMEM case from the size = 0 case.

Describe alternatives you've considered

None.

Additional context

None

ret2libc commented 1 year ago

The problem is: what do you do in the macro when RZ_NEW returns NULL?

Rot127 commented 1 year ago

Try to exit as gracefully as possible. If there is not enough memory to allocate our tiny structs anymore, it is very unlikely that anything will work from there on. So either we call some kind of wait_until_some_mem_is_free function or just save the project and exit (maybe allocate some memory on start of Rizin to deal with these kind of things). My point is to implement an emergency exit, which saves the current project state, before everything goes sideways.

ret2libc commented 1 year ago

If you don't have memory left, it's hard to save projects as well, AFAIK. This is a big change overall. I'm not against it, but it would mean a bit shift in how the rizin codebase works.

Also, we need to be careful in some cases, like when allocating memory for some data structures in binaries. I'd prefer rizin to not load all the sections of a binary but still allow me to do something rather than just exiting because the RZ_NEW for the sections failed.

Rot127 commented 1 year ago

If you don't have memory left, it's hard to save projects as well, AFAIK.

Yeah. This is probably true. Would need quite a lot of work.

Thinking about it a second time, Rizin is not so terribly important, that it cannot just exit. On the other hand, why not go to sleep for X seconds and retry again? Just as a quick solution.

I'd prefer rizin to not load all the sections of a binary

This would tab into a more general discussion how to save resources, right? We could consider this from now on, when reviewing new PRs.

ret2libc commented 1 year ago

What would sleeping accomplish? Rizin does not use threading for the most part so if you sleep there's nothing else that can free the used space.

XVilka commented 1 year ago

I am against overengineering in such a simple case. It's better to invest time into making saving and loading data/sessions more resilient, it will require less changes as well than this idea.

The issue in hand could be by creating new macro - RZ_NEW_CHECK() or something like that. So that users of this new macro will know what to expect. We can slowly then substitute places where it's necessary, step by step.

XVilka commented 1 year ago

It's better to invest time and energy into: