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

Replace RZ_ASM_NOPLUGINS and RZ_BIN_NOPLUGINS with more general RZ_NOPLUGINS #3919

Merged
merged 1 commit into from
Oct 16, 2023

Conversation

Rot127
Copy link
Member

@Rot127 Rot127 commented Oct 12, 2023

Your checklist for this pull request

  • I've read the guidelines for contributing to this repository
  • I made sure to follow the project's coding style
  • I've documented or updated the documentation of every function and struct this PR changes. If not so I've explained why.
  • I've added tests that prove my fix is effective or that my feature works (if possible)
  • I've updated the rizin book with the relevant information (if needed)

Detailed description

...

Test plan

All green (since the variable is used for all tests, stuff will explode if something is wrong).

Closing issues

...

@XVilka
Copy link
Member

XVilka commented Oct 13, 2023

@ret2libc take a look at this one please, and if you don't have any objections - merge.

Copy link
Member

@ret2libc ret2libc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mh, If the others are ok i'm ok, though I would have gone with the opposite solution of keeping one NOPLUGIN for each kind of plugins.

librz/main/rz-asm.c Outdated Show resolved Hide resolved
@Rot127
Copy link
Member Author

Rot127 commented Oct 13, 2023

@ret2libc

though I would have gone with the opposite solution of keeping one NOPLUGIN for each kind of plugins.

We can do this as well. Though, if we want to have it properly done, we would need to:

  • Add checks for RZ_NOPLUGIN where ever the other NOPLUGINS are.
  • Define somewhere what a bin plugin and what an asm plugin is.

Idk, is it worth the trade? We can also open an issue about it for a better plugin loading module.
This is maybe even a nice task for beginners with C knowledge.

Something where each plugin needs to set its category and we have a simply call to enable a certain plugin

load_plugin(plugin) { if (do_not_load_plugin_group(X) return; .... }

@ret2libc
Copy link
Member

I'm not sure I follow you.

Define somewhere what a bin plugin and what an asm plugin is.

What do you mean? We already have separate bin plugins and separate asm plugins.

@Rot127
Copy link
Member Author

Rot127 commented Oct 13, 2023

What do you mean? We already have separate bin plugins and separate asm plugins.

Sorry, I was not clear with this.

The flag BIN/ASM_NOPLUGINS suggests that only ASM/BIN plugins don't get loaded. But when the flag is checked and is set, no plugin is loaded at all (at the location of the env check).
Also there are no checks for RZ_HASH_NOPLUGINS or RZ_ANALYSIS_NOPLUGINS, although plugins of this type are loaded as well.

A more general problem is, plugin libraries get loaded via rz_lib_opendir(). But this loads always all libraries from this directory (at least this is what the doc string says).
Though the plugin is only usable if the handler was added via rz_lib_add_handler().

This appears inconsistent to me and hard to manage, because there is no single module which does the plugin loading.
RZ_NOPLUGINS (or RZ_XXX_NOPLUGINS) should be only checked at a single location in the code base IMHO. So if for example ASM plugins should be loaded, a single plugin load handler is called, it checks if RZ_ASM_NOPLUGINS or RZ_NOPLUGINS is set, loads only the needed libs and adds only the needed handlers.

@ret2libc
Copy link
Member

I understand now, thanks for the explanation! If so, yes then it makes sense how you did it :) Thanks for the patience :D

@XVilka XVilka merged commit 58ee80d into rizinorg:dev Oct 16, 2023
43 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants