Repository navigation
fix: technical review findings across library, backends, app, mcp and ci - #111
Merged
Merged
Conversation
Library and backends: - Linux: decode /proc names and map paths without failing on invalid UTF-8 - bind scan and listing generators to the process object (ClosedProcess after close) - validate search arguments at call time, not on the first next() - reject floats in unsigned writers and bool pids; range-check NumPy integers - fix duplicate regex matches across chunks, overlap past region end and unaligned pointers straddling chunks; keep pointer widths apart in intersect - refuse pointer-chain hops outside the 64-bit space (ValueError and OSError) - Windows: drop short reads, retry module snapshots, grow mapped-file buffer - macOS: restore max protection after writes, report failed close, walk fixes - NumPy paths for the pointer map and integer NOT_EXACT scans App: - free finished workers and closed dialogs; no auto-refresh after cancel/error - linear promotion and refine; frozen inline edits; dead-target inline edits - pointer scan stop message, backpressure and unresolved compare rows MCP: - protect the whole launch chain; per-instance approvals for shared runtimes - unreadable names always ask; more password managers denied, sensitive flag - opt-in --confirm-writes; response size budget; huge regions scanned in pieces - bufflength "0" sentinel, normalized value_type, locked scan lookup CI and packaging: - least-privilege permissions, split build/publish with a green-CI gate - CodeQL, scoped and retried pip-audit gate, per-platform mypy, wider macOS matrix - psutil <8, PySide6 <7, refreshed tests extra, release docs in CONTRIBUTING
github-actions
Bot
added
docs
|
You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool. What Enabling Code Scanning Means:
For more information about GitHub Code Scanning, check out the documentation. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #111 +/- ##
===========================================
- Coverage 92.15% 77.41% -14.75%
===========================================
Files 41 64 +23
Lines 4231 9186 +4955
===========================================
+ Hits 3899 7111 +3212
- Misses 332 2075 +1743
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
- mcp: read /proc/<pid>/stat as bytes, so a non-UTF-8 comm no longer ends the ancestor walk and leaves the MCP client unprotected - mcp: pin a shared-runtime approval to the start identity shown in the prompt; refuse the attach if the pid was reused while it was open - mcp: apply the response value budget only to str/bytes scans and cut hex to an even length, so truncated values still parse - mcp: echo value_type through one canonical_value_type helper - app: drop the unused _drain_pending_writes; tests use the run() path
- app: resolve pointer-scan rows with their stored base again; only Compare rows whose module is not loaded are flagged and refused (rebasing every row onto first_module_bases picked the wrong image for duplicate names) - mcp: re-check the policy after attaching on a remembered approval, so a per-instance grant cannot be used by a process that reused the pid - backends: always sort scan regions; a MemoryRegionSnapshot is a mutable list and sorting an already sorted one is a linear pass - win32: pause briefly between ERROR_BAD_LENGTH module snapshot retries - scanning: drop the progress_callback plumbing no caller passes - tests: avoid reference cycles through Qt objects, which let the cyclic GC free them off the GUI thread (worker segfault on CI)
- re-check a remembered approval off the event loop, and say whether the start time was unreadable or the pid was reused - do not remember a shared-runtime approval whose start time could not be read before the prompt - procinfo: reuse the Linux backend's start-time parser - macos: the bitness probe lists modules without computing image sizes
This was referenced Oct 12, 2026
JeanExtreme002
added a commit
that referenced
this pull request
Oct 12, 2026
The informational app-coverage step added in #111 uploaded PyMemoryEditor/app to Codecov, which merges every upload into one project total: ~4.5k app lines at ~62% next to ~4.6k library and MCP lines at ~93% took the badge from 92% to 77%. The library and MCP coverage did not change. Drop that step and .coveragerc-app, and list the app under codecov.yml's ignore, as the CI coverage gate (.coveragerc-lib) already leaves it out.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why is this PR necessary, what does it do?
A full technical review of the project (library, backends, MCP server, app, CI) produced a list of findings. Each one was re-verified against the code before changing anything; the ones that turned out not to be problems were left alone (see Notes). Public defaults are unchanged: where a finding could only be fully fixed by changing a default, this PR adds an opt-in safeguard and documents the risk instead. Pure refactors (deduplicating the backends, splitting large classes) are out of scope.
Library (
process/,util/)write_uint*accepted floats and truncated them (write_uint(a, 3.7)wrote 3) whilewrite_intraisedTypeError; unsigned writers now useoperator.index. NumPy integers out of range were silently wrapped; they are range-checked now.ProcessInfo.pid = Trueopened pid 1; bools are rejected.search_by_value/search_by_value_between/search_by_patternare called (newprepare_search_target), not on the firstnext()of the returned generator.intersect_pointer_pathsmerged 32- and 64-bit paths with the same recipe; it keys onptr_sizetoo (JSON format unchanged).resolve_pointer_chain: ctypes masks addresses to 64 bits, soNULL + negative offsetread a wrapped address. A hop outside[0, 2**64)now raises an error that is bothValueErrorandOSError(what the failed read used to raise), so existingexcept OSErrorkeeps working.NOT_EXACTscans. Debug logs where a pointer-map chunk or the fixed-width check was skipped silently.Backends (
win32/,linux/,macos/)/proc/*/comm, taskcommand/proc/<pid>/mapswere read in strict text mode, so one process with a non-UTF-8 name (any process can set one withprctl) brokeOpenProcess(name=...)for everyone, and a non-UTF-8 mapped path broke every scan of that target. They are decoded with replacement characters now.p1.close(), Windows can hand the same handle value to the nextOpenProcess, so a kept generator could read another process. Newprocess/_guard.pyre-checks before every resume and raisesClosedProcess.ReadProcessMemorykept a zero-filled tail that could match a scan for 0 (now skipped like on Linux/macOS);get_modulesretries (with a short pause) onERROR_BAD_LENGTHand raisesProcessIDNotExistsErrorfor an exited target;GetMappedFileNameWgrows its buffer instead of truncating; unusedsystem_information/_HOST_IS_64BITremoved.VM_PROT_COPYleftWRITEin a page's maximum protection after a write; the restore lowers it again.close()reports a failed port release (as Windows/Linux do);get_modulesraisesProcessIDNotExistsErrorfor an exited target; the region walk no longer wraps the 64-bit cursor or stops at a zero-size region; load commands are parsed once in_static_image_ranges.App (
app/)parent=self, nodeleteLater), keeping every scan's address copies alive; closed tool dialogs had noWA_DeleteOnClose. Both are freed now.rows_readyhad no backpressure; Compare walked a saved path's previous-run base when its module is not loaded, and the row could still be sent to the cheat table — such rows are shown unresolved and refused when acted on.snapshot_readydisconnected on shutdown; swallowed shutdown exceptions are logged at debug.MCP (
mcp/)getpid()/getppid(); underuvxthe parent isuv, so the MCP client was attachable. Newmcp/procinfo.pywalks the ancestor chain (bounded, cycle- and pid-reuse-aware) and the policy protects all of it.python3/node/ shells approved every process with that name; for shared runtimes it now covers that instance only (approval_scopein the result), pinned to the start time shown in the prompt — if the pid is reused while the prompt is open, the attach is refused. Unreadable process names skipped the denylist; they always ask now. More password managers are denied; browsers and AI clients are flaggedsensitiveand warned about in the prompt (not denied).--confirm-writes: the server elicits before eachwrite_value(fails closed). Off by default so clients that honourrequiresUserInteractionare not asked twice; the risk of the default is documented.str/bytesscans share a byte budget (additive*_truncatedfields; hex is cut to an even length); tool descriptions mark memory contents as untrusted. Huge regions are scanned in pieces so the deadline holds.bufflength="0"errored instead of meaning "default";read_value/write_valueechoed the rawvalue_type;find_scanread without the store lock; HTTP help text claimed remote hosting.CI and packaging
python-package.ymlgets top-levelpermissions: contents: read.publish.ymlis split into build (noid-token) and publish jobs, and requires a successful non-PRPython Packagerun on the release commit before uploading.pip-auditaudits only the extras' frozen closure (not its own dependencies), retries transient failures, and blocks outside pull requests.check_untyped_defs; macOS matrix covers 3.10, 3.12 and 3.14; informational app coverage.psutil>=5.9,<8(7.x verified),PySide6>=6.5,<7,testsextra refreshed; Hypothesisciprofile;make publishguarded bycheck-release-tag.User-visible changes (for the release notes)
close()raisesClosedProcess.write_uint*rejects floats and numeric strings;OpenProcess(pid=True)is rejected.ValueError(also anOSError).get_modules()on an exited target raiseProcessIDNotExistsError; macOSclose()can returnFalse.--confirm-writes; truncation fields andapproval_scope/sensitive/write_confirmation.Checklist (complete all items):
References:
No references to be shared.
Notes:
--platformdarwin/linux/win32),sphinx -W,python -m buildand actionlint are clean. MCP suite also passes onmcp2.0.0, so themcp>=2,<3floor stays.PyMemoryEditorError(it would change whatexcept PyMemoryEditorErrorcatches and abort pointer scans on the first unreadable page); shrinking the Linux region struct's 4 KiB inline path (public field); returning the valid prefix of a partial read (needs a coordinated driver + 3-backend change); memoising dead ends in the reverse pointer walk (not provably result-identical); moving Compare / region snapshot off the UI thread (needs new workers).mcpfloor,_set_busybypass. The Linux freeze-after-exit concern is already covered by the pidfd binding from v3.2.0.