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

Serialize seek history #3838

Merged
merged 3 commits into from
Sep 10, 2023
Merged

Conversation

laomaiweng
Copy link
Contributor

@laomaiweng laomaiweng commented Sep 8, 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

This PR implements seek history serialization/deserialization to/from the Rizin project file.

This is something that I feel is worth preserving across sessions, as seek history is often the logical path the user took to reach some location within the binary. I know in my case I often use it as a lifeline to go back up when I go down some rabbit hole in a binary, and having that lifeline lost if I close Rizin is annoying, hence the PR.

Since I'm new to this code base, there are a number of implementation details that I feel I should mention:

  • SDB format
    • the seek history items are serialized as SDB entries with the item index as the entry key, which means the entries
      • may not be ordered numerically by key in the SDB (I figured that doesn't really matter)
      • have to be sorted numerically upon load before parsing the items into the seek history
    • this could have been avoided by storing the items as a JSON array but that felt less legible
    • also using the SDB entry key for a numerical sort means the sort will behave badly if (for some reason, e.g. hand-crafted project file) some key isn't a number, but I figured if the project file is malformed it's not my problem as long as rizin doesn't crash
  • current offset
    • the current seek offset is saved as part of the seek history (with key 0 & "is_current": true), which is somewhat redundant with the core/offset entry (not fully redundant since the current seek history item also holds the cursor position which is otherwise not saved)
    • for now the offset of the current item in the seek history is just ignored upon load, I figured at some point in the future it would probably make sense to drop core/offset and use exclusively the offset from the current seek history item
  • I added a new vector API for swapping two items in a vector, but it's only used by the seek history deserialization code so if that's not enough users for it to be an API just yet I can remove it

Test plan

Green CI, incl. added unit tests (for seek history serialization & the new rz_vector_swap API) and integration tests (for the bumped project version).

Closing issues

N/A

@laomaiweng
Copy link
Contributor Author

Updated the db/cmd/project test to account for the project version bump.

librz/include/rz_core.h Outdated Show resolved Hide resolved
librz/util/vector.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.

Looks good, just some small details.

@laomaiweng laomaiweng force-pushed the serialize-seek-history branch from febce1b to 05cb958 Compare September 8, 2023 05:58
@laomaiweng
Copy link
Contributor Author

Fixed these, thanks for the review!

@laomaiweng laomaiweng force-pushed the serialize-seek-history branch from 05cb958 to 5af2fa1 Compare September 8, 2023 06:35
@laomaiweng
Copy link
Contributor Author

Update for the rz-bindgen lint check failure.

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

@laomaiweng
Copy link
Contributor Author

laomaiweng commented Sep 8, 2023

CI fails on MSVC because it doesn't support VLAs (in rz_vector_swap), can I #ifdef my way around it and use alloca instead on MSVC only ?

if (child->type != RZ_JSON_INTEGER) {
break;
}
seek_item.cursor = child->num.s_value;
Copy link
Member

Choose a reason for hiding this comment

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

why s_value instead of u_value here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because cursor in RzCoreSeekItem is an int, so I assumed it may be negative sometimes?

librz/include/rz_vector.h Outdated Show resolved Hide resolved
@laomaiweng laomaiweng force-pushed the serialize-seek-history branch from 5af2fa1 to 4b855eb Compare September 8, 2023 16:20
@laomaiweng
Copy link
Contributor Author

Update for typo & MSVC build failure.

Copy link
Member

@thestr4ng3r thestr4ng3r left a comment

Choose a reason for hiding this comment

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

Overall a sensible addition and good code!

librz/core/meson.build Outdated Show resolved Hide resolved
test/prj/v15-seek-history.rzdb Outdated Show resolved Hide resolved
librz/util/vector.c Outdated Show resolved Hide resolved
@laomaiweng laomaiweng force-pushed the serialize-seek-history branch from 4b855eb to 0172df8 Compare September 10, 2023 07:06
@laomaiweng
Copy link
Contributor Author

laomaiweng commented Sep 10, 2023

Updated with the suggested changes, thanks for the review!
I realize I forgot to even include the rationale for the change in the PR, I'll edit it to add that.

@laomaiweng
Copy link
Contributor Author

Not sure what's wrong with the failed db/formats/dmp/dmp test, there's only been a single failure in CI and I can't reproduce locally. Perhaps a network error? It seems to be fetching a PDB.

@XVilka XVilka merged commit 700c7d8 into rizinorg:dev Sep 10, 2023
43 checks passed
@laomaiweng laomaiweng deleted the serialize-seek-history branch September 10, 2023 13:37
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.

5 participants