fix(app): prevent OverflowError from addresses above 2^63 in memory map - #77
Conversation
…n the Python side
…to visually compare
…er to visually compare
|
The "Lint PR title" check doesn't check case insensitive, even though github generates titles from branches automatically with casing. |
|
Hi, @cromachina! Thanks for contributing. There are some issues that must be fixed before merging this PR. I'll post the reviews here. |
JeanExtreme002
left a comment
There was a problem hiding this comment.
Good catch on the root cause. Verified locally: regular addresses (0x7ffd…) go through setData fine — only values above 2**63 overflow, which is exactly [vsyscall] at 0xffffffffff600000 on Linux x86-64. The Python-side payload is the right call, and it also covers pointer_scan_dialog.py:687, which had the same latent bug.
Three things before merge, inline. The first one is a crash, the other two are smaller.
One more: the checklist says tests were added, but there are none in the diff and _widgets.py has no test file yet. tests/app/ + QT_QPA_PLATFORM=offscreen already make this cheap to cover — a round trip with 0xffffffffff600000 and a sort over rows with a missing payload would pin both regressions.
…would be the default of __lt__
JeanExtreme002
left a comment
There was a problem hiding this comment.
Thanks for the quick turnaround — roles, type hints and the sort recursion are all sorted (verified: sorting a column with None payloads no longer crashes, and UserRole / UserRole + 1 stay distinct). Resolved those threads.
One new issue from the text() override, plus two smaller things, inline.
For the record on the docstring: the cap isn't really about unsigned support — a plain QStandardItem takes 0x7FFDABCD1234 fine and only breaks past 2**63, i.e. QVariant's qint64. Your 2**128 case shows the same edge. Also worth stating the constraint the workaround introduces: the payload never reaches the C++ model, so it must be read as item.data(role) and never model.data(index, role) — that path converts back to a QVariant and overflows again (confirmed locally).
JeanExtreme002
left a comment
There was a problem hiding this comment.
Adding applicable suggestions for the two open points above. Both verified locally: with them the app suite is 83 passed and flake8 is clean; putting the text() override back makes the new assertions fail.
The override returned str(self.data()), so text() stopped matching what the table displays (the raw payload instead of the formatted label, and 'None' for rows without one) and the __lt__ fallback compared 'None' to 'None' rather than the labels. Its except branch was unreachable too: dict.get() returns None, it never raises KeyError. QStandardItem.text() already returns the label the fallback wants. The NumericItem tests were only checking that nothing crashed. Assert the comparison results, and cover the two paths that weren't exercised: user roles staying distinct, and the C++ sort driving comparisons over a column that mixes payloads with None — the case that segfaulted, which comparing two items directly doesn't reach.
|
I went ahead and pushed the two suggestions to this branch (
Verified locally: 406 passed, 78 skipped, flake8 clean. Still open: the |
…ests The address columns were reaching into pointer_scan_dialog for its private _MONO, which points the dependency the wrong way: the memory map pulled in the whole pointer scan module for one string. Move the family to _widgets, next to the NumericItem those columns already import, and have all three dialogs read it from there. The memory map's size field was hardcoding the same stack, so it reads the constant now too. Move the NumericItem tests out of the smoke file into test_app_widgets.py. They were gated behind a pytest-qt skip they never needed — none of them uses qtbot, a QApplication is enough — so they now run wherever PySide6 is installed.
|
Pushed the last two as well (
406 passed, 78 skipped, flake8 clean. All review threads are resolved now — the only thing I left alone is the docstring wording, which is fine as is. |
The cap isn't about unsigned support — a plain QStandardItem takes 0x7FFDABCD1234 without complaint and only breaks past 2**63, which is QVariant's qint64 limit. Name the actual trigger ([vsyscall] at 0xffffffffff600000) and the constraint the workaround creates: the payload never reaches the C++ model, so reading it back through model.data() hits the same OverflowError.
|
Last one from me ( From my side this is ready to merge. |
Why is this PR necessary, what does it do?
Fixes an error that appears when viewing the memory map (and also potentially the module viewer). The error prevents the lists from being fully populated. The error can also potentially segfault the program, at least when shiboken stuff runs.
Also includes a minor cosmetic change to make comparing the addresses in a list easier (monospace font for the addresses).
Checklist (complete all items):
References:
Fixes #73
Notes:
No notes to be shared.