Refactor `dm` #1182

MalhotraPulak · 2021-06-01T10:58:04Z

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 added tests that prove my fix is effective or that my feature works (if possible)
I've updated the documentation and the rizin book with the relevant information (if needed)

Detailed description
Plan:

Refactor dm to use RZ_API
Refactor dmh to use RZ_API
Make heap viewer in Cutter using the refactored commands

Currently:

Add more tests for the commands
Expose API to cutter

Test plan

CI is green

...

Closing issues

...

librz/core/cmd_debug.c

XVilka · 2021-06-01T11:46:55Z

I suggest to port that JSON PR on top of this one once it's finished.

librz/core/cmd_debug.c

librz/core/cmd_descs/cmd_debug.yaml

MalhotraPulak · 2021-06-02T07:19:16Z

@XVilka I found dmq and dmq. command in the codebase which were not mentioned in the help menu. I implemented that in the new shell. Can you tell what summary should I write for them?
Also, in the help menu there are dms and dms- command mentioned but not implemented anywhere. Should I add them to new shell as commands that do nothing or leave them?

XVilka · 2021-06-02T07:44:15Z

@PulakIIIT for

dms - remove them
dmq means quiet. Judging by the code it's the same as dm but with less verbose information. Just add RZ_OUTPUT_MODE_QUIET in dm YAML and call it from here.

Moreover, I checked the code of rz_debug_map_list(). It is wrong nowadays because:

Generally, when we have function ending in _list() it means it returns list of something, e.g. as RzList *. When the function prints something, we call it _print() instead.
Generally, we prefer to avoid RzCons usage or printing outside of the librz/core.

Thus, I suggest to do the following:

Add function rz_debug_map_print() in librz/core/cdebug.c
Make function rz_debug_map_list() return RzList *
Move all print helpers of old rz_debug_map_list() together with it into librz/core/cdebug.c
Move rz_debug_map_list_visual() with helpers also into librz/core/cdebug.c
Also I suggest to use RzOutputMode instead of char in that funciton like described in Refactor output modes to use enum #489

MalhotraPulak · 2021-06-02T13:45:57Z

@XVilka I have moved the function rz_debug_map_list and related functions from dmap.c to librz/core/cdebug.c and renamed rz_debug_map_list to rz_debug_map_print since it mostly focuses on printing. I also refactored rz_debug_map_print to use RzOutputMode. Since I renamed that function at the moment there is no function rz_debug_map_list which returns an RzList * as you mentioned in your first point. Is it necessary to have rz_debug_map_list in the codebase?

librz/core/cdebug.c

XVilka · 2021-06-03T06:27:47Z

Since I renamed that function at the moment there is no function rz_debug_map_list which returns an RzList * as you mentioned in your first point. Is it necessary to have rz_debug_map_list in the codebase?

Please add, you will need it for Cutter anyway.

librz/core/cdebug.c

XVilka · 2021-06-10T09:54:29Z

Please also rebase on top of the latest dev to resolve the conflicts.

librz/core/linux_heap_glibc.c

librz/core/cmd_debug.c

librz/core/cmd_descs/cmd_heap_glibc.yaml

librz/core/linux_heap_glibc.c

librz/core/cmd_debug.c

librz/core/cmd_descs/cmd_debug.yaml

librz/core/cmd_descs/cmd_heap_glibc.yaml

librz/core/cmd_descs/cmd_debug.yaml

librz/core/cmd_descs/cmd_heap_glibc.yaml

librz/include/rz_core.h

XVilka

Overall looks good, there are some minor changes required though before being ready to merge.

librz/core/cdebug.c

librz/core/cmd_debug.c

XVilka · 2021-06-16T04:42:11Z

librz/core/cmd_debug.c

+			/* TODO: do not spawn. use RzBin API */
+			if (sectname) {
+				char *sect = rz_str_escape(sectname);
+				res = rz_sys_cmd_strf("env RZ_BIN_PREFIX=\"%s\" rz-bin %s-B 0x%08" PFMT64x " -S \"%s\" | grep \"%s\"", name, mode, baddr, filesc, sect);


@thestr4ng3r could you please suggest a proper API as a substitute for this command?

this is not ok.

but we can change this in another PR

librz/core/linux_heap_glibc.c

librz/debug/dmap.c

librz/include/rz_core.h

librz/include/rz_heap_glibc.h

librz/core/cmd_debug.c

XVilka

LGTM, except if there is a quick API to substitute that rz-bin spawn. Lets see what @ret2libc and @yossizap think.

librz/core/cmd_descs/cmd_debug.yaml

librz/core/linux_heap_glibc.c

XVilka

Is this still "WIP"? Looks ready to me.

librz/core/cmd_debug.c

ret2libc · 2021-06-17T07:30:53Z

librz/core/cmd_debug.c

@@ -1443,376 +1357,462 @@ static bool get_bin_info(RzCore *core, const char *file, ut64 baseaddr, PJ *pj,
 	return true;
 }

-static int cmd_debug_map(RzCore *core, const char *input) {
+// dm


I'm tempted to remove these comments... They will likely become wrong at some point. I'd prefer to focus on #691 to correctly find the right handler.

librz/core/cmd_debug.c

ret2libc · 2021-06-17T07:33:28Z

librz/core/cmd_debug.c

+	rz_debug_map_sync(core->dbg); // update process memory maps
+	RzList *list = rz_debug_modules_list(core->dbg);
+	rz_list_foreach (list, iter, map) {
+		if ((!libname ||


very very long if. Not sure if it comes from old code, but a separate function with a meaningful name or something else would be better

Yeah this if comes from the old code indeed

librz/core/cmd_debug.c

ret2libc · 2021-06-17T07:42:15Z

Overall looks good anyway! Great work ;)

ret2libc

It looks good to me, but since you are touching a lot of debug code (which is not tested too much) please do a round of manual testing with a debugging session. I did it already a couple of versions ago, but better be sure ;)

yossizap

I am a bit late for this review so I'll just leave the comment regarding the API for a future PR. LGTM other than that.

yossizap · 2021-06-17T10:18:33Z

librz/include/rz_core.h

+RZ_API RzList *rz_heap_heap_chunks_list_32(RzCore *core, MallocState *main_arena, ut32 m_arena, ut32 m_state);
+RZ_API bool rz_heap_resolve_main_arena_32(RzCore *core, ut32 *m_arena);
+RZ_API bool rz_heap_update_main_arena_32(RzCore *core, ut32 m_arena, MallocState *main_arena);
+RZ_API RzList *rz_heap_tcache_list_32(RzCore *core, ut32 m_arena, MallocState *main_arena, bool main_thread_only);


Since we are planning to use the API here for Cutter's HeapWidget frontend the _32/_64 variations in the API should be made internal and replaced with wrapper functions that will use a bool/bitness var to differentiate architectures.

@PulakIIIT I think this can be done in a separate PR. Let's merge this one.

MalhotraPulak changed the title ~~Refactor dm~~ [WIP] Refactor dm Jun 1, 2021

github-actions bot added command RzCore labels Jun 1, 2021

XVilka requested changes Jun 1, 2021

View reviewed changes

librz/core/cmd_debug.c Outdated Show resolved Hide resolved

XVilka requested review from ret2libc and yossizap June 1, 2021 11:47

XVilka added shell refactor Refactoring requests labels Jun 1, 2021

XVilka requested changes Jun 1, 2021

View reviewed changes

librz/core/cmd_debug.c Outdated Show resolved Hide resolved

librz/core/cmd_debug.c Outdated Show resolved Hide resolved

XVilka requested changes Jun 2, 2021

View reviewed changes

librz/core/cmd_descs/cmd_debug.yaml Show resolved Hide resolved

XVilka requested changes Jun 3, 2021

View reviewed changes

librz/core/cdebug.c Outdated Show resolved Hide resolved

librz/core/cdebug.c Outdated Show resolved Hide resolved

XVilka requested changes Jun 4, 2021

View reviewed changes

librz/core/cdebug.c Outdated Show resolved Hide resolved

MalhotraPulak force-pushed the refactor-dm branch from e2fc3cf to 0feca77 Compare June 4, 2021 10:44

XVilka added this to the 0.3.0 milestone Jun 4, 2021

MalhotraPulak marked this pull request as ready for review June 10, 2021 06:28

MalhotraPulak requested review from kazarmy, thestr4ng3r and wargio as code owners June 10, 2021 06:28

MalhotraPulak force-pushed the refactor-dm branch from 3d7e227 to 1ef881e Compare June 10, 2021 11:11

ret2libc reviewed Jun 11, 2021

View reviewed changes

ret2libc reviewed Jun 14, 2021

View reviewed changes

XVilka requested changes Jun 16, 2021

View reviewed changes

librz/core/cmd_debug.c Outdated Show resolved Hide resolved

librz/core/cmd_debug.c Outdated Show resolved Hide resolved

MalhotraPulak added 7 commits June 16, 2021 15:15

Remove static keyword and commented code

2b36e2b

More documentation fixes

108c1d6

Use API instead of cmd

c24e648

Refactor dmm and dmm. command implementation

b208a65

minor changes

f46afbc

Made Heap API naming more consistent

24a0ed2

dbg->cb_printf -> rz_cons_printf

d199448

MalhotraPulak force-pushed the refactor-dm branch from fa1af3d to d199448 Compare June 16, 2021 09:46

XVilka approved these changes Jun 16, 2021

View reviewed changes

XVilka requested a review from ret2libc June 16, 2021 09:55

ret2libc reviewed Jun 16, 2021

View reviewed changes

librz/core/cmd_descs/cmd_debug.yaml Show resolved Hide resolved

librz/core/cmd_descs/cmd_debug.yaml Outdated Show resolved Hide resolved

librz/core/cmd_descs/cmd_debug.yaml Show resolved Hide resolved

librz/core/linux_heap_glibc.c Show resolved Hide resolved

MalhotraPulak added 4 commits June 16, 2021 16:42

use ARGV_STATE in dm

c5a2ddd

use ARGV_STATE in dmm

de285a0

remove fastbin option for dmhd for consistency

6d92a03

use ARGV_STATE in dmh

1cd104e

XVilka approved these changes Jun 16, 2021

View reviewed changes

MalhotraPulak changed the title ~~[WIP] Refactor dm~~ Refactor dm Jun 16, 2021

XVilka assigned ret2libc Jun 17, 2021

ret2libc reviewed Jun 17, 2021

View reviewed changes

MalhotraPulak added 2 commits June 17, 2021 13:30

Replaced repetitive code with macro

6d7ca8e

Use rz_cmd_state_output_array

fbc5b7e

XVilka requested a review from ret2libc June 17, 2021 09:15

Merge branch 'dev' into refactor-dm

dd4ed65

ret2libc approved these changes Jun 17, 2021

View reviewed changes

XVilka added the merge-when-green label Jun 17, 2021

yossizap approved these changes Jun 17, 2021

View reviewed changes

XVilka merged commit 6fabb6d into dev Jun 17, 2021

XVilka deleted the refactor-dm branch June 17, 2021 14:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor `dm` #1182

Refactor `dm` #1182

MalhotraPulak commented Jun 1, 2021 •

edited

Loading

XVilka commented Jun 1, 2021 •

edited

Loading

MalhotraPulak commented Jun 2, 2021 •

edited

Loading

XVilka commented Jun 2, 2021 •

edited

Loading

MalhotraPulak commented Jun 2, 2021 •

edited

Loading

XVilka commented Jun 3, 2021

XVilka commented Jun 10, 2021

XVilka left a comment

XVilka Jun 16, 2021

wargio Jun 17, 2021

wargio Jun 17, 2021

XVilka left a comment

XVilka left a comment

ret2libc Jun 17, 2021

ret2libc Jun 17, 2021

MalhotraPulak Jun 17, 2021

ret2libc commented Jun 17, 2021

ret2libc left a comment

yossizap left a comment

yossizap Jun 17, 2021

XVilka Jun 17, 2021

Refactor dm #1182

Refactor dm #1182

Conversation

MalhotraPulak commented Jun 1, 2021 • edited Loading

XVilka commented Jun 1, 2021 • edited Loading

MalhotraPulak commented Jun 2, 2021 • edited Loading

XVilka commented Jun 2, 2021 • edited Loading

MalhotraPulak commented Jun 2, 2021 • edited Loading

XVilka commented Jun 3, 2021

XVilka commented Jun 10, 2021

XVilka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XVilka left a comment

Choose a reason for hiding this comment

XVilka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ret2libc commented Jun 17, 2021

ret2libc left a comment

Choose a reason for hiding this comment

yossizap left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Refactor `dm` #1182

Refactor `dm` #1182

MalhotraPulak commented Jun 1, 2021 •

edited

Loading

XVilka commented Jun 1, 2021 •

edited

Loading

MalhotraPulak commented Jun 2, 2021 •

edited

Loading

XVilka commented Jun 2, 2021 •

edited

Loading

MalhotraPulak commented Jun 2, 2021 •

edited

Loading