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

RzList to RzPVector shift in RzAnalysisSwitchOp #4024

Open
wants to merge 12 commits into
base: dev
Choose a base branch
from

Conversation

HN026
Copy link
Contributor

@HN026 HN026 commented Dec 7, 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

...

Closing issues

Partially addresses #3872

@XVilka

This comment was marked as resolved.

librz/core/canalysis.c Outdated Show resolved Hide resolved
librz/core/canalysis.c Outdated Show resolved Hide resolved
Copy link
Member

@XVilka XVilka left a comment

Choose a reason for hiding this comment

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

With ASAN you can see the reason why many tests fail:

[XX] db/abi/platforms/main_signatures MAIN_ABI : mips64-linux-gnueabi main
RZ_NOPLUGINS=1 /home/runner/bin/rizin -escr.utf8=0 -escr.color=0 -escr.interactive=0 -eflirt.sigdb.load.system=false -eflirt.sigdb.load.home=false -N -A -Qc 'e scr.utf8=false
s main
afs @ main
' bins/abi_bins/elf/platforms/mips64-linux-gnueabi-echo
-- stdout
--- expected
+++ actual
@@ -1,1 +1,0 @@
-int main(int argc, char **argv, char **envp);

-- stderr
[ ] Analyze all flags starting with sym. and entry0 (aa)
[
[x] Analyze all flags starting with sym. and entry0 (aa)

[ ] Analyze function calls
[
[x] Analyze function calls
[ ] find and analyze function preludes
[
[x] find and analyze function preludes
[ ] Analyze len bytes of instructions for references
[
[x] Analyze len bytes of instructions for references
[ ] Check for classes
[
[x] Check for classes

[x] Finding xrefs in noncode section with analysis.in=io.maps

[x] Analyze value pointers (aav)

[x] Value from 0x120000000 to 0x120001514 (aav)

[x] 0x120000000-0x120001514 in 0x120000000-0x120001514 (aav)
[ ] Emulate functions to find computed references
[../librz/include/rz_vector.h:270:28: runtime error: applying non-zero offset 18446744073709551600 to null pointer
AddressSanitizer:DEADLYSIGNAL
=================================================================
==64086==ERROR: AddressSanitizer: SEGV on unknown address (pc 0x7fe85f897f4e bp 0x7ffc03d82f50 sp 0x7ffc03d82120 T0)
==64086==The signal is caused by a READ memory access.
==64086==Hint: this fault was caused by a dereference of a high value address (see register values below).  Disassemble the provided pc to learn which register was used.
    #0 0x7fe85f897f4e in rz_pvector_at ../librz/include/rz_vector.h:270
    #1 0x7fe85f897f4e in get_next_i ../librz/core/canalysis.c:3310
    #2 0x7fe85f897f4e in rz_core_analysis_esil ../librz/core/canalysis.c:3685
    #3 0x7fe85f990bf2 in rz_core_analysis_esil_references_all_functions ../librz/core/cil.c:296
    #4 0x7fe85f8b81a0 in rz_core_analysis_everything ../librz/core/canalysis.c:4826
    #5 0x7fe85f8ba15f in rz_core_perform_auto_analysis ../librz/core/canalysis.c:6521
    #6 0x7fe869d0a528 in rz_main_rizin ../librz/main/rizin.c:1379
    #7 0x7fe869229d8f  (/lib/x86_64-linux-gnu/libc.so.6+0x29d8f) (BuildId: 203de0ae33b53fee1578b117cb4123e85d0534f0)
    #8 0x7fe869229e3f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x29e3f) (BuildId: 203de0ae33b53fee1578b117cb4123e85d0534f0)
    #9 0x56538e3ffb04 in _start (/home/runner/bin/rizin+0x2b04) (BuildId: 6cbc57462e9936c4a87c6b5769a61799e0a240d6)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV ../librz/include/rz_vector.h:270 in rz_pvector_at
==64086==ABORTING

-- exit status: 1

You can try to import this commit: 42a3fb7 cc @imbillow

You can try to send a PR with that change separately too @imbillow

XVilka and others added 3 commits December 9, 2023 09:24
* PDB: add module info parsing
* Refactor PDB component
* Refactor PDB/tpi
* Fix read_pascal_string
* Fix memory leaks
* Add test `Modules from PDB`
@github-actions github-actions bot removed the RzUtil label Dec 10, 2023
librz/core/canalysis.c Outdated Show resolved Hide resolved
librz/core/canalysis.c Outdated Show resolved Hide resolved
Copy link
Member

@wargio wargio left a comment

Choose a reason for hiding this comment

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

lgtm.

@wargio
Copy link
Member

wargio commented Dec 15, 2023

looks like a lot of stuff is broken.

@HN026
Copy link
Contributor Author

HN026 commented Dec 15, 2023

looks like a lot of stuff is broken.

Yes, the issue persists. I tried added additional checks but no help

@wargio
Copy link
Member

wargio commented Dec 15, 2023

looks like a lot of stuff is broken.

Yes, the issue persists. I tried added additional checks but no help

it is not because of the checks but because you changed the logic behind it. i suspect the main issue is related to how the new code works with the switch cases.

Comment on lines 380 to 381
if (bb->switch_op) {
RzList *unique_cases = rz_list_uniq(bb->switch_op->cases, casecmp);
outputs += rz_list_length(unique_cases);
rz_list_free(unique_cases);
if (bb->switch_op) {
Copy link
Member

Choose a reason for hiding this comment

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

looks like you also have dup this check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Argh, that's a mistake by my side. Apologies.

librz/core/canalysis.c Outdated Show resolved Hide resolved
librz/core/canalysis.c Outdated Show resolved Hide resolved
Copy link
Member

@wargio wargio left a comment

Choose a reason for hiding this comment

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

This PR is blocked till i refactor some code in dev.

@XVilka
Copy link
Member

XVilka commented Dec 18, 2023

@HN026 now that refactoring has landed, please rebase your PR (and fix corresponding conflicts)

@HN026
Copy link
Contributor Author

HN026 commented Dec 18, 2023

@HN026 now that refactoring has landed, please rebase your PR (and fix corresponding conflicts)

@XVilka Sure will do.

@wargio
Copy link
Member

wargio commented Dec 20, 2023

not yet. the code i want to refactor is not yet done. that PR was one of many

@HN026
Copy link
Contributor Author

HN026 commented Dec 20, 2023

not yet. the code i want to refactor is not yet done. that PR was one of many

Okay I've already done it once for now. Inform me when you're done.

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