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

add a smali debugger #1137

Merged
merged 5 commits into from
Mar 28, 2021
Merged

add a smali debugger #1137

merged 5 commits into from
Mar 28, 2021

Conversation

busmaker
Copy link
Contributor

@busmaker busmaker commented Mar 18, 2021

#1136
Hi guys, this debugger works on android 7 , 8(thanks to @Surendrajat for testing), and 11,
but it can't get registers on 9 and 10 without debug info in the app.

And for Android 11, when a register is "zero value", ART won't change such register's value, so we can't set value to it.

Now it supports to modify fields of this object, surprise~

For debugging, you must have

  1. adb
  2. a debuggable app, or a rooted device/emulator that can make apps debuggable.

ps: this debugger will search ANDROID_HOME and PATH to look for adb.

Features:

  • single step
  • set breakpoint
  • modify register value

Short cuts:

  • f2 set/remove breakpoint
  • f7 step into
  • f8 step over
  • shift + f8 step out
  • f9 run

Demo
setting up an app to debug

debugger_present_wait

adb usage

debugger_present_adb

debugging

debugger_present_debug2

@lgtm-com
Copy link

lgtm-com bot commented Mar 18, 2021

This pull request introduces 4 alerts and fixes 1 when merging 0ec0c1d into 52412df - view on LGTM.com

new alerts:

  • 2 for Result of multiplication cast to wider type
  • 1 for Array index out of bounds
  • 1 for Continue statement that does not continue

fixed alerts:

  • 1 for Array index out of bounds

@Surendrajat
Copy link
Contributor

Surendrajat commented Mar 18, 2021

Great work @lbj-the-goat.

My observations:

  1. Everything works with Android 8.1 AVD. Will it be possible to get register values in android 9/10?
  2. Unchecking Show Dalvik Bytecode messes up breakpoints and all. Can it be disabled/hidden during debugging?
  3. Debugging related icons can be improved. Can we use these icons instead? :)

@busmaker
Copy link
Contributor Author

@Surendrajat

Everything works with Android 8.1 AVD. Will it be possible to get register values in android 9/10?

Thank you for testing for me, that's so great!
Android 9/10 have different way to handle getRegister request, they look into the debug info to check the type of register we requested, unless the app has the info otherwise they just reject directly.

Unchecking Show Dalvik Bytecode messes up breakpoints and all. Can it be disabled/hidden during debugging?

Okay, I will fix it

Debugging related icons can be improved. Can we use these icons instead? :)

Haha, I grabbed them online randomly.
I will check these icons out.
Thank you.

@skylot
Copy link
Owner

skylot commented Mar 18, 2021

@lbj-the-goat thanks!
Did you copy any code or resources from other projects (like ADB, JDWP classes and icons)?
If yes, please add corresponding licenses to NOTICE file in jadx root.

Also can you provide description how to regenerate JDWP class, link to jdwp.py script not working :(
Or maybe we can use a library with such code? Did you try to search it? I am really concerned about this big generated class :(

Can we use these icons instead? :)

@Surendrajat many icons I take from Eclipse JDT UI project (git mirror). Maybe these will fit better.

@busmaker
Copy link
Contributor Author

busmaker commented Mar 18, 2021

@skylot Relax, I wrote all the code myself including the jdwp-gen.py, and this poor script it was good, one day morning it just disappeared, I sincerely believe that it was accidentally deleted because I named it temp.py, and I realised this when I was trying to push it to github, it was too late. I can delete this line though.

And as for the icons I believe they don't need any primits and attributings, some of them are laying on my driver for a long time. But I think your suggestion

many icons I take from Eclipse JDT UI project (git mirror). Maybe these will fit better.

is better.

@busmaker
Copy link
Contributor Author

busmaker commented Mar 18, 2021

@skylot

maybe we can use a library with such code?

I had thinked of it, but don't know how to do it? Can you help me?

@skylot
Copy link
Owner

skylot commented Mar 18, 2021

maybe we can use a library with such code?

I mean maybe someone already created java library for this jdwp stuff :)

@busmaker
Copy link
Contributor Author

maybe we can use a library with such code?

I mean maybe someone already created java library for this jdwp stuff :)

I found a tutorial, I'll try to make it a library when this PR is ready to merge. :)

@Surendrajat
Copy link
Contributor

Surendrajat commented Mar 18, 2021

@lbj-the-goat I have few suggestions(possible improvements):

  1. Smali code is using parameter registers p0, p1,.. naming convention but debugger view is using normal ...,vN-2,vN-1,vN naming convention. Having p0,p1,.. really helps with keeping track of the input params.
    So can the last registers in debugger view be renamed to match the same (although I see the parameter name in parentheses there to help but still)?

  2. Since you're already searching for launcher activity and manifest also has package name, can the target app be started from Jadx itself using adb shell am start -n com.package.name/com.package.name.ActivityName? and can the target process be automatically found by searching the process list for package name?
    This will reduce the time taken by manually starting the app then selecting the process but more importantly, it will allow it to have a Restart Debug feature.
    What do you think?

@busmaker
Copy link
Contributor Author

@Surendrajat Good ideas, Let machine does its jobs is the right thing to do. :D

@Surendrajat
Copy link
Contributor

@lbj-the-goat cool. In that case, current (manual process selection) view can be shown if it fails to start activity or find the process.

@lgtm-com
Copy link

lgtm-com bot commented Mar 21, 2021

This pull request introduces 4 alerts and fixes 1 when merging 4890826 into 19572a6 - view on LGTM.com

new alerts:

  • 2 for Result of multiplication cast to wider type
  • 1 for Array index out of bounds
  • 1 for Continue statement that does not continue

fixed alerts:

  • 1 for Array index out of bounds

@Surendrajat
Copy link
Contributor

Surendrajat commented Mar 21, 2021

@lbj-the-goat tried the above commit(yeah, I was waiting for it :D) and here are my observations:

  1. Parameter registers names (p0,p1..) are shown as well as variable names 👍
  2. Can restart debug session from debugger itself 👍
  3. Getting few exceptions in logs (Android 8.1) maybe fixable if you want to check: https://del.dog/jadx.txt
  4. Works on Android 10 too but not able to get/set register values (expected)
  5. The main Debug icon from Toolbar is clickable even when there're a debug session already running and selecting debug again(while one in process), it messes up things. So can the Debug icon be disabled once a debug session is started?
  6. Can the Locate App thing be improved to automatically locate and select package name after connection is established(start adb server if not running)? and if the process is not running start it automatically even on first time? I think this way Locate App can be renamed to Debug App and user just have to hit that button(for confirmation) because app is automatically started and selected by Jadx.
  7. For restart debug, I think, it doesn't need the Locate App thingy as well because it knows what its restarting and it can seamlessly start a debug session without user interaction. Taking few seconds to restart the app and locating the process in background is very much acceptable :)

Overall, I love this feature and I really like how it's coming together. Thanks a lot!

@busmaker
Copy link
Contributor Author

@Surendrajat Hi, glad to know you were expecting this commit 😀

Getting few exceptions in logs (Android 8.1)

The Failed to read packet exception occurrs when Socket Connection was closed, may be the app exited or any othert reasons that could break this connect.
jdwp handshake failed may be the app was in the middle of handshake or being debugged, anyway it's occupied, it happens sometime, I haven't figured it out why, the best way to do is to kill the app and run again.

Works on Android 10 too but not able to get/set register values...

It takes debug_info to access and modify registers, so there's nothing I can do...

The main Debug icon from Toolbar is clickable...

May be a message box warning is better, sometimes people could pick the wrong app to debug, leave it clickable so we can restart again right away.👍

Can the Locate App thing be improved to automatically locate and select package ...

I was going to make it select package automatically, But some apps have quite large AndroidManifest.xml to decode, on a regular machine it could take seconds, if do it in background it may hurt user experience, I thought a Locate App button could make people feel more responsive, but a Launch App button seems more useful, so I will change it next commit😁

For restart debug, I think, it doesn't need the Locate App thingy as well because it knows ...

Yeah, that's because adb shell am doesn't return pid, so I have to get process list from adb again, sometimes, the app on the remote machine couldn't start up in time even thought I have set a delay for it, so when time's up the AdbDialog has to show up otherwise propel will wondering what the heck is going on. I added Locate App partly for this reason, so that as soon as the dialog showed up people can select the process they want.

I'll try to improve this problems anyway, Thanks again for your advices! 👍👍 Have a good day.

@busmaker
Copy link
Contributor Author

@skylot I've changed the icons to famfamfam's and made jdwp.java a library, Do you think it's ready to be merged?

@lgtm-com
Copy link

lgtm-com bot commented Mar 27, 2021

This pull request introduces 4 alerts and fixes 1 when merging df360b8 into 19572a6 - view on LGTM.com

new alerts:

  • 2 for Result of multiplication cast to wider type
  • 1 for Array index out of bounds
  • 1 for Continue statement that does not continue

fixed alerts:

  • 1 for Array index out of bounds

Copy link
Contributor

@Surendrajat Surendrajat left a comment

Choose a reason for hiding this comment

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

Tried a bit and everything looks good now. Nice work!

@skylot
Copy link
Owner

skylot commented Mar 27, 2021

@lbj-the-goat code look good, thanks.
One little bug: debug does not work for me on linux, on windows it is okay, but on linux it stuck on waiting jdwp handshake reply. Not sure why this happens. Can you check this?
Anyway, I think this PR is ready to merge. Any small issues we can fix in next PRs, also this one become so big that it is hard to review it on updates 😢

@Surendrajat
Copy link
Contributor

@skylot Not sure if it's something Linux specific because I am also using Linux(Ubuntu 18.04) and it works.

@busmaker
Copy link
Contributor Author

@skylot Sorry, I don't have a Linux machine.
On Windows, if it stuck at jdwp handshake stage that's because the remote app is occupied by another debugger, especially Android Studio, when it's running it beats other debuggers, I don't know what kind of black magic it used.

Anyway if it stuck, kill the remote app can end the waiting. May be we should use a thread to do the handshake job, so the ui doesn't freeze for this kind of problems.

@skylot
Copy link
Owner

skylot commented Mar 27, 2021

@lbj-the-goat thanks! It works now. Just running Android Studio in background breaks debug in jadx 😞
To fix hang up on reply wait it is possible to add socket timeout: socket.setSoTimeout(5000);, so it will throw exception if connect failed.

@lgtm-com
Copy link

lgtm-com bot commented Mar 28, 2021

This pull request introduces 4 alerts and fixes 1 when merging 253a4fe into 19572a6 - view on LGTM.com

new alerts:

  • 2 for Result of multiplication cast to wider type
  • 1 for Array index out of bounds
  • 1 for Continue statement that does not continue

fixed alerts:

  • 1 for Array index out of bounds

@skylot skylot merged commit 4705194 into skylot:master Mar 28, 2021
@bagipro
Copy link
Collaborator

bagipro commented Apr 12, 2021

@skylot @lbj-the-goat
Sorry, I didn't find my previous comment related to command injection. Yeah, you were right, that command will be executed on the device, but not the host computer, so it should be fine :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants