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

feat: add JSON dumping functionality to DartDumper #17

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

nitanmarcel
Copy link

No description provided.

@nitanmarcel nitanmarcel changed the title feat: add JSON dumping functionality to DartDumper [Draft] feat: add JSON dumping functionality to DartDumper Nov 9, 2023
@nitanmarcel nitanmarcel marked this pull request as draft November 9, 2023 18:45
@nitanmarcel
Copy link
Author

Converted to draft until I find time to add the structs to the json output

@nitanmarcel nitanmarcel changed the title [Draft] feat: add JSON dumping functionality to DartDumper feat: add JSON dumping functionality to DartDumper Nov 9, 2023
@nitanmarcel nitanmarcel marked this pull request as ready for review November 9, 2023 21:05
@nitanmarcel nitanmarcel marked this pull request as ready for review November 11, 2023 09:45
@nitanmarcel
Copy link
Author

After many commits I think this is ready. Waiting for a review from you @worawit

@worawit
Copy link
Owner

worawit commented Nov 16, 2023

  1. It requires a "nlohmann-json3" library. No instruction or script to add the library dependency when building on Windows and macOS. For this case, it can be added to this project directly with single header file. But I prefer no additional library if possible.
  2. Is dumping assembly as json really needed?
  3. Some Dart register name should not be replaced without context. For example, CSREG_DART_WB_* names are useful only code that calling write barrier stub.
  4. Dumping assembly as a whole json object uses too much memory. The system might run out of memory when dumping a large flutter app. For this case, only stream mode should be used.
  5. More Dart function information might be extracted in a future. I don't want to maintain this feature yet. So it might be a while before I merge this pull request.

@nitanmarcel
Copy link
Author

nitanmarcel commented Nov 16, 2023

  1. It requires a "nlohmann-json3" library. No instruction or script to add the library dependency when building on Windows and macOS. For this case, it can be added to this project directly with single header file. But I prefer no additional library if possible.
  2. Is dumping assembly as json really needed?
  3. Some Dart register name should not be replaced without context. For example, CSREG_DART_WB_* names are useful only code that calling write barrier stub.
  4. Dumping assembly as a whole json object uses too much memory. The system might run out of memory when dumping a large flutter app. For this case, only stream mode should be used.
  5. More Dart function information might be extracted in a future. I don't want to maintain this feature yet. So it might be a while before I merge this pull request.

I think the json could be made manually by adding strings but no idea yet if that would create any weird issues that the json library could automatically handle, using it in as an header sounds like a more optimal solution

No idea if the assembly json is really needed, I just saw it being used by your ida script. But you raise a good objection with the memory usage. Will remove and if it's needed in the future I suppose the app can just output them to stdout to be parsed.

About maintaining, I suppose that if the same methods are used to extract the functions metadata won't cause any trouble maintaining it since it will use the output that's already been extracted (same way the idea script generator works).

Will see when I'm free to do the point 1 and remove the assembly json and handle the other feedback.

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.

2 participants