New RZIL integration #1663

Heersin · 2021-09-13T01:54:52Z

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

New IL as an independent library which relies on rz_util only.
IL in core analysis are moved to core/cil.c. (Related pr of moving esil -> Reorganize esil directory #1360 )
Change the tech of collecting stats and trace in the old ESIL
Fix some bugs in the old pr New Rizin IL integration #1361 New Rizin IL integration #1451

Test plan

Should pass the old rizin tests.

Closing issues

closes #277

More Info
0. Use tree-like approach

This pr created for asan build to detect bug.
Only enabled in analysis_bf.c.
Register arena changes in other module (such as debug) are not covered in current new IL.

Heersin · 2021-09-13T02:18:11Z

The trace and stats techs of the old index approach replies on temporary value. Now it has been removed. We need to implement the trace/stats in the rzil. It aims to collect info reg/mem read/write.

Currently, the API of opcode evaluation is :
rzil_parse_root(RzILVM *vm, RzILOp *op);

it's a recursive function to evaluate the expression. so we may add an extra arg as Callback function
rzil_parse_root(RzILVM *vm, RzILOp *op, ParseCallbacks f);

the callback function ccould be

rzil_focus_trace (RzILOp *op)
 - mem read
 - mem write
 - reg read
 - reg write

etc.

but what I worry about is the possible header conflict.
Since our new trace info structure defined in higher level of rizin project while the rzil can only rely on rz_util

we may not be able to directly use the function and trace info struct in rz_analysis.h

Defined a temporary struct to store the collected info ?
Implement the similar parse/emulate function in the same level or higher one. As esil did.

That's two possible solutions I have for now. Do you have any idea ?

XVilka · 2021-09-13T05:45:29Z

@Heersin I think adding some new structure is a way to go. Not sure what @ret2libc @thestr4ng3r @wargio think.

wargio

looks good with these new changes.

wargio · 2021-09-13T08:20:39Z

librz/core/analysis_tp.c

 	core->dbg->trace = rz_debug_trace_new();
 	core->analysis->esil->trace = rz_analysis_esil_trace_new(core->analysis->esil);
+	core->analysis->rzil->trace = rz_analysis_rzil_trace_new(core->analysis, core->analysis->rzil);


if you have core->analysis why passing core->analysis->rzil ?

Yes it passes these 2 because I followed the ESIL api. We can only maintain one argument core->analysis.

Yes, lets skip and use just core->analysis for both.

wargio · 2021-09-13T08:23:54Z

librz/core/cil.c

+	ut8 code[32];
+	// analysis current data to trigger rzil_set_op_code
+	(void)rz_io_read_at_mapped(core->io, addr, code, sizeof(code));


since you are ignoring the rz_io_read_at_mapped return value, maybe it should be better to initialize code to 0 or any other value that represent NOP or invalid op.

I recommend not to ignore the value and do the function exit on the error

librz/il/definitions/bag.c

librz/il/definitions/bool.c

librz/il/definitions/effect.c

librz/il/theory_effect.c

librz/il/theory_init.c

librz/il/vm_layer.c

librz/include/rz_analysis.h

ret2libc · 2021-09-13T13:41:49Z

The trace and stats techs of the old index approach replies on temporary value. Now it has been removed. We need to implement the trace/stats in the rzil. It aims to collect info reg/mem read/write.

Currently, the API of opcode evaluation is :
rzil_parse_root(RzILVM *vm, RzILOp *op);

it's a recursive function to evaluate the expression. so we may add an extra arg as Callback function
rzil_parse_root(RzILVM *vm, RzILOp *op, ParseCallbacks f);

I'm not 100% sure I understood the problem, but I think having a callback that takes the ILOp as argument (maybe also the RzILVM as well) should be enough to support all kinds of traversal of the OPs without putting anything specific in the RzIL module.

XVilka

Overall in a good shape now. Only a few minor nitpicks left, and should be ready to merge. We can improve the implementation further after merging, IMHO.

librz/il/vm_layer.c

librz/il/rzil_vm.c

XVilka · 2021-09-24T04:10:10Z

@Heersin could you please rebase on the latest dev and fix some unaddressed comments - there is a conflict now?

librz/core/cmd_analysis.c

thestr4ng3r · 2021-09-27T11:13:04Z

librz/include/rz_il/rzil_opcodes.h

+ */
+struct rzil_op_int_t {
+	ut32 length; ///< s -- sort(type), length of bitvector
+	int value; ///< x -- value of bitvector


Shouldn't this rather just contain a single RzILBitVector and be called something like RzILOpBitVector?

thestr4ng3r · 2021-09-27T11:26:40Z

librz/include/rz_il/rzil_opcodes.h

+
+	// Memory
+	RZIL_OP_LOAD,
+	RZIL_OP_STORE,


Wouldn't RZIL_OP_LOAD actually be a bit vector op and RZIL_OP_STORE be an effect?

No, the operand and effect are separated in this case, see the original implementation:

http://binaryanalysisplatform.github.io/bap/api/odoc/bap-core-theory/Bap_core_theory/Theory/Effect/Sort/index.html

Interesting, where are the operands in this link?

librz/include/rz_il/rzil_opcodes.h

librz/analysis/rzil/rzil.c

ret2libc · 2021-09-27T21:30:55Z

There's a lot of work here, nice job. I don't want to be tooo picky here, as otherwise we'll never merge this. I think there are however just few more things to adjust, in particular the void * thing as mentioned also by @thestr4ng3r .

librz/analysis/rzil/rzil.c

librz/core/cil.c

librz/core/cmd_analysis.c

librz/il/vm_layer.c

librz/il/rzil_vm.c

librz/include/rz_il/rzil_vm.h

librz/il/rzil_vm.c

librz/include/rz_il/rzil_vm.h

librz/include/rz_il/definitions/value.h

XVilka · 2021-10-03T03:36:40Z

@Heersin could you please rebase your PR and address the feedback?

Heersin · 2021-10-05T09:43:25Z

@Heersin could you please rebase your PR and address the feedback?

Sure, I will review it tonight

XVilka

@Heersin you forgot 6 more feedback comments, please take a look again. Apart from that, it's good to merge, I think. cc @ret2libc @thestr4ng3r

* Fix db/cmd/esil tests and anlysis_bf bugs * Change BUILD_ header to RZ header * Fix macros and types * Remove unnecessary string and bit types and unused code * Add autolabeler * Various file name and function name changes * Fix broken test * Add unit test for il_definitions * Change the index behaviour of bitvector

- Fix some comments - Add ut8 ut16 convertions

* Rename functions * Add as_string and remove some unused debug functions * Add doxygen docs * Small fix * Fix header protector to corresponding name * Add null checks and RZ_NONNULL * Add more unit tests of rzil_vm, effect and mem * Minimal migration from index approach tree-like approach * Fix bug in new rzil op struct

Address some rename comments Add test for evaluation Address some old comments Fix tests

XVilka · 2021-10-09T08:11:46Z

Two ASAN failed tests are unrelated to this PR and happen on the latest dev as well:

[XX] TIMEOUT db/formats/elf/sections more than 65535 segments
RZ_NOPLUGINS=1 rizin -escr.utf8=0 -escr.color=0 -escr.interactive=0 -N -Qc iSS~? gzip://bins/elf/core/more-than-65535-segments.gz
-- stdout
--- expected
+++ actual
@@ -1,1 +1,0 @@
-131073

-- stderr
Cannot retrieve regstate on: AMD x86-64 architecture (not yet supported)

-- exit status: -1

[XX] TIMEOUT db/formats/mdmp mdmp 64bit - strings
RZ_NOPLUGINS=1 rizin -escr.utf8=0 -escr.color=0 -escr.interactive=0 -N -Qc 'iz~0x002e2fe1[1,2,3,4,5,6,7,8]
iz~0x002e2feb[1,2,3,4,5,6,7,8]
iz~0x002e2ff5[1,2,3,4,5,6,7,8]
iz~0x00330d85[1,2,3,4,5,6,7,8]
iz~0x003310c1[1,2,3,4,5,6,7,8]
iz~0x00161a37[1,2,3,4,5,6,7,8]
iz~0x00161c7f[1,2,3,4,5,6,7,8]
iz~0x00161dbf[1,2,3,4,5,6,7,8]
iz~0x001d9d45[1,2,3,4,5,6,7,8]
iz~0x001da0a5[1,2,3,4,5,6,7,8]
iz~0x003fad45[1,2,3,4,5,6,7,8]
iz~0x003fb0ad[1,2,3,4,5,6,7,8]
iz~0x00480857[1,2,3,4,5,6,7,8]
iz~0x004808c7[1,2,3,4,5,6,7,8]
iz~0x00499cf5[1,2,3,4,5,6,7,8]
iz~0x0049a045[1,2,3,4,5,6,7,8]
' bins/mdmp/hello64.dmp
-- stdout
--- expected
+++ actual
@@ -8,9 +8,3 @@
 0x00161dbf 0x7759e130 45 92 C:_Windows_System32_kernel32.dll utf16le \REGISTRY\USER\*\SOFTWARE\Classes\Wow6432Node
 0x001d9d45 0x776160b6 15 32 C:_Windows_System32_kernel32.dll utf16le VS_VERSION_INFO
 0x001da0a5 0x77616416 11 24 C:_Windows_System32_kernel32.dll utf16le VarFileInfo
-0x003fad45 0x7fefd5080b6 15 32 C:_Windows_System32_KERNELBASE.dll utf16le VS_VERSION_INFO
-0x003fb0ad 0x7fefd50841e 11 24 C:_Windows_System32_KERNELBASE.dll utf16le VarFileInfo
-0x00480857 0x7fefdb33bc8 4 10 C:_Windows_System32_msvcrt.dll utf16le PATH
-0x004808c7 0x7fefdb33c38 10 22 C:_Windows_System32_msvcrt.dll utf16le SystemRoot
-0x00499cf5 0x7fefdb4d066 15 32 C:_Windows_System32_msvcrt.dll utf16le VS_VERSION_INFO
-0x0049a045 0x7fefdb4d3b6 11 24 C:_Windows_System32_msvcrt.dll utf16le VarFileInfo

-- stderr
[WARN] Invalid or unsupported enumeration encountered 21
[WARN] Invalid or unsupported enumeration encountered 22
[INFO] Parsing data sections for large dumps can take time, please be patient (but if strings ain't your thing try with -z)!

-- exit status: -1

Heersin requested review from XVilka, ret2libc, thestr4ng3r and Basstorm September 13, 2021 01:54

github-actions bot added command RzAnalysis labels Sep 13, 2021

wargio requested changes Sep 13, 2021

View reviewed changes

Heersin force-pushed the asan_integrate_new_il branch from 32f7093 to f0fc2f0 Compare September 15, 2021 13:51

XVilka requested changes Sep 15, 2021

View reviewed changes

librz/il/vm_layer.c Show resolved Hide resolved

librz/il/rzil_vm.c Outdated Show resolved Hide resolved

librz/il/rzil_vm.c Outdated Show resolved Hide resolved

librz/il/rzil_vm.c Outdated Show resolved Hide resolved

librz/il/rzil_vm.c Outdated Show resolved Hide resolved

Heersin force-pushed the asan_integrate_new_il branch from f0fc2f0 to 6f1372a Compare September 16, 2021 16:18

XVilka approved these changes Sep 16, 2021

View reviewed changes

XVilka assigned ret2libc and thestr4ng3r Sep 24, 2021

Heersin force-pushed the asan_integrate_new_il branch from 6f1372a to 7da8ec3 Compare September 24, 2021 06:09

thestr4ng3r reviewed Sep 27, 2021

View reviewed changes

ret2libc reviewed Sep 27, 2021

View reviewed changes

librz/analysis/rzil/rzil.c Outdated Show resolved Hide resolved

ret2libc reviewed Sep 27, 2021

View reviewed changes

Heersin force-pushed the asan_integrate_new_il branch from 7da8ec3 to 343cc78 Compare October 8, 2021 13:19

XVilka requested changes Oct 8, 2021

View reviewed changes

Heersin added 3 commits October 9, 2021 15:20

* Roughly integrate with New IL

014ba15

Add some test about bitvector

86cf010

- Fix some comments - Add ut8 ut16 convertions

Heersin added 4 commits October 9, 2021 15:20

Add better error message

2368f7e

Add doxygen docs

cd0ead3

Address some comments

570cffc

Address some rename comments Add test for evaluation Address some old comments Fix tests

Heersin force-pushed the asan_integrate_new_il branch from 343cc78 to 570cffc Compare October 9, 2021 07:21

XVilka approved these changes Oct 9, 2021

View reviewed changes

Merge branch 'dev' into asan_integrate_new_il

0502c53

thestr4ng3r approved these changes Oct 9, 2021

View reviewed changes

XVilka merged commit e0d3d6b into dev Oct 9, 2021

chinggg mentioned this pull request Oct 12, 2021

Regression Bug: fail to build due to not token from librz/rz_il/rzil_opcodes.h rizinorg/rz-ghidra#258

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New RZIL integration #1663

New RZIL integration #1663

Heersin commented Sep 13, 2021

Heersin commented Sep 13, 2021 •

edited

Loading

XVilka commented Sep 13, 2021

wargio left a comment

wargio Sep 13, 2021

Heersin Sep 15, 2021

XVilka Sep 15, 2021

wargio Sep 13, 2021

XVilka Oct 8, 2021

ret2libc commented Sep 13, 2021

XVilka left a comment

XVilka commented Sep 24, 2021 •

edited

Loading

thestr4ng3r Sep 27, 2021

thestr4ng3r Sep 27, 2021

XVilka Sep 27, 2021

thestr4ng3r Sep 27, 2021

ret2libc commented Sep 27, 2021

XVilka commented Oct 3, 2021

Heersin commented Oct 5, 2021

XVilka left a comment

XVilka commented Oct 9, 2021

New RZIL integration #1663

New RZIL integration #1663

Conversation

Heersin commented Sep 13, 2021

Heersin commented Sep 13, 2021 • edited Loading

XVilka commented Sep 13, 2021

wargio 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ret2libc commented Sep 13, 2021

XVilka left a comment

Choose a reason for hiding this comment

XVilka commented Sep 24, 2021 • edited Loading

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 Sep 27, 2021

XVilka commented Oct 3, 2021

Heersin commented Oct 5, 2021

XVilka left a comment

Choose a reason for hiding this comment

XVilka commented Oct 9, 2021

Heersin commented Sep 13, 2021 •

edited

Loading

XVilka commented Sep 24, 2021 •

edited

Loading