rz_core_cmd replacement with API

[caribpa]

The current state of rz_core_cmd vs the API in the code, as mentioned by @ret2libc in 0.2.0 Priorities is:

remove most uses of rz_core_cmd* call whenever possible by using the proper API instead. Again, this is a quite long task but we have already achieved some good results. This will allow us to refactor the code with more confidence as we move, because if you change something you get a compiler warning instead of realizing just after X days/months that a command called through rz_core_cmd* has changed its syntax/does not work anymore.

According to the current search results for rz_core_cmd, there are 18 files that are still using rz_core_cmd:

• librz/core/agraph.c Refactor Graph mode (librz/core/agraph.c) to use API instead of commands #397
• librz/core/cmd_interpret.c
• librz/core/cmd_help.c
• librz/core/rtr.c
• librz/core/cmd.c
• librz/core/vmenus.c Refactor Visual Menus /librz/core/tui/vmenus* to use API instead of commands #491
• librz/core/cdebug.c
• test/unit/test_autocmplt.c
• librz/socket/rzpipe.c - A string
• librz/core/visual.c
• librz/core/cmd_search.c
• ibrz/core/cfile.c
• librz/core/task.c
• librz/core/core.c
• librz/core/cmd_debug.c
• librz/debug/debug.c - A comment
• librz/include/rz_core.h
• librz/core/panels.c Refactor Panels mode (librz/core/tui/panels.c) to use API instead of commands #383
• librz/core/cmd_print.c - A comment, probably not relevant

From these files, I think the ones with higher priorities are the "base" ones, such as librz/core/core.c and librz/core/cmd.c.

Honestly, I have no clue of the changes that need to be performed for using the API instead of rz_core_cmd (I'll investigate), but I would make these two "base" files a priority for 0.2.0 and leave the others for another milestone.

[officialcjunior]

Honestly, I have no clue of the changes that need to be performed for using the API instead of rz_core_cmd (I'll investigate), but I would make these two "base" files a priority for 0.2.0 and leave the others for another milestone.

Hey, I'm hoping to make this a bit clearer. Firstly, this is mainly done to:

• To reduce the overhead, code cruft, and code breakage.
  Calling rz_core_cmd is similar to running the whole command directly again. Also, making an API means, we can just call it whenever we need it on panels or Visual modes, reducing the overhead and increasing the simplicity. Moreover, this increases code quality.

• Will reduce the work when someone converts the commands into newshell.
  If we have an API for a particular command, we can simply call that API from the command handler making the conversion very easier.

The workflow for solving this would be something like: (Quoting @XVilka's message from Rizin Dev)

1. See rz_core_cmd[f0]("bla ...") call
   Find a rz_core_cmd() call and see what kind of a command it is calling.

2. Go check to the corresponding librz/core/cmd_*.c what exact functions are called.
   Which means, check out the actual code that is triggered by that command.

3. Depending on how much code is under that command either directly substitute with the same functions that are called from the same command, or introduce one or more additional functions that combine those logic and reuse them in both command implementation and in place of that rz_core_cmd*() calls.

You can also see the issues related to this pinned on the corresponding project: https://github.com/rizinorg/rizin/projects/5
To check out previous commits see the linked Pull Requests from the Issues from the corresponding Project's page.

[caribpa]

Lovely!

This is well-written enough to be used for the new release/changelog explanation (first two points), as well as to be included in a document regarding how-to-contribute to the rz_core_cmd to API conversion (next three points with @XVilka quotes).

Excellent work 🏆

[karliss]

Firstly, this is mainly done to:

Third major benefit is preventing command injection vulnerabilities mostly in situations where arguments are not properly escaped like rz_core_cmdf("cmd %s", arg). There have been plenty of those before.

[XVilka]

Please note, that some of the rz_core_cmd[0f_] calls are legitimate, e.g. in librz/core/visual.c or librz/core/panels.c when they are used for calling commands defined in the configuration variables, e.g.:

	case RZ_CONS_KEY_F1:
		cmd = rz_config_get(core->config, "key.f1");
		if (cmd && *cmd) {
			ch = rz_core_cmd0(core, cmd);
		} else {
			visual_help(core);
		}
		break;
	case RZ_CONS_KEY_F2:
		cmd = rz_config_get(core->config, "key.f2");
		if (cmd && *cmd) {
			ch = rz_core_cmd0(core, cmd);
		} else {
			visual_breakpoint(core);
		}
		break;

But when #384 is done, ideally it should be only called once, and the same for Visual, Panels, and Visual Menus modes.

[XVilka]

No, it isn't, see also commands that are taken from the config variables like I listed above.

[ret2libc]

In general, all places where you have no clue what is the command(s) that is going to be executed and the command comes from the user are ok to use rz_core_cmd. When, instead, you know that you have to execute af, for example, it's better to just call the right API for it.

[caribpa]

I see, somehow I didn't process this when they are used for calling commands defined in the configuration variables when I first read it 🙂

Then, regarding the : command, what's its state? does it use (or shall use) the API, or rz_core_cmd?

[ret2libc]

Then, regarding the : command, what's its state? does it use (or shall use) the API, or rz_core_cmd?

I guess you are referring to : in visual mode. Of course in that case it shall use rz_core_cmd, because that is like a prompt, with commands given by the user... The programmar has no way to know the command that the user is going to type.

[caribpa]

Makes sense, thanks for clarifying 🙂

[XVilka]

It should become a core priority for 0.3.0 - hopefully we will be able to eradicate all unnecessary such calls.