Decode /proc/mounts octal escapes in parseMounts - #129
Conversation
The kernel encodes space, tab, newline, and backslash as octal escapes (\040, \011, \012, \134) in /proc/mounts, since those characters would otherwise be ambiguous in its whitespace-separated format. parseMounts used the raw whitespace-split fields verbatim, so a mountpoint or device path containing one of these characters (e.g. a USB drive labeled "My Drive", mounted at /media/pi/My\040Drive) never matched its real path. Add unescapeMountField to decode the four escapes proc(5) documents for this format, applied to both the device and mountpoint fields. Any other backslash sequence is left untouched.
There was a problem hiding this comment.
Reviewed against the project's Go/security/API conventions. go build ./..., go vet ./..., go test ./... -race -cover (internal/collector 91.1%) and golangci-lint run (0 issues) all pass on a4b2bc9.
The decoder logic itself is correct — I checked the bounds arithmetic (i+3 < len(s) is exactly the condition under which s[i+1:i+4] is in range, including an escape ending the string), the i += 3 / loop-i++ advance, and the left-to-right single pass that makes \134 self-consistent. No API shape change, no new dependency, no exec/privilege surface touched.
Two findings inline, both in the test/consistency layer rather than the decoder. The table-driven restructuring is required by docs/TESTS.md and should be addressed before merge.
fstype is mangled by the kernel with the same octal escapes as device
and mountpoint (e.g. a fuse subtype derived from user input can be
"fuse.My\040FS"), but only the other two fields were being decoded.
Apply unescapeMountField to fstype as well, since it's used for the
excludedFSType lookup and surfaced verbatim in /api/v1/metrics and the
dashboard.
Collapse the three separate escape-decoding test functions into one
table-driven TestParseMounts_DecodesOctalEscapes per docs/TESTS.md, and
add a case pinning that the single left-to-right decoding pass doesn't
re-scan bytes it already emitted: an escaped backslash ("\134")
immediately followed by digits that look like another escape ("040")
must decode to a literal "\040", not a space.
|
2 participants
📖 Description
parseMountsininternal/collector/disk.gosplit each/proc/mountslineon whitespace and used the resulting fields as
device/mountpoint/fstypeverbatim. The kernel encodes space, tab, newline, and backslash as octal
escapes (
\040,\011,\012,\134) in/proc/mounts, since thosecharacters would otherwise be ambiguous in its whitespace-separated format
(see
proc(5)). A mountpoint or device path containing one of these — e.g. aUSB drive labeled
My Drive, mounted at/media/pi/My\040Drive— was neverdecoded back to its real path, so it wouldn't match the actual filesystem
path elsewhere (an include/exclude list, the dashboard, the API).
This is a bug fix, not a breaking change: parsing of ordinary paths (the
overwhelming majority, with no escapes) is unaffected.
Adds
unescapeMountField, applied to both thedeviceandmountpointfields in
parseMounts. It only recognizes the four escapesproc(5)documents for this format; any other backslash sequence (or a lone trailing
backslash) is left untouched rather than misinterpreted.
🎫 Issues
Closes #128
👩💻 Reviewer Notes
The change is contained to
parseMountsand the newunescapeMountField/mountFieldEscapesininternal/collector/disk.go. Everything downstream(
Collect,dedupeMounts,collectDisk) is untouched — they alreadyoperate on
mountEntry.device/.mountpointas opaque strings.📑 Test Plan
Added to
internal/collector/disk_test.go, alongside the existingTestParseMounts:TestParseMounts_DecodesOctalEscapes— a mountpoint fixture with all fourescapes (
\040,\011,\012,\134) decodes to the literalspace/tab/newline/backslash characters.
TestParseMounts_DecodesOctalEscapesInDevice— the same escape (\040)in the device field (e.g. a
/dev/disk/by-label/...path) is decoded too.TestParseMounts_LeavesUnknownBackslashSequencesUntouched— a backslashsequence that isn't one of the four real escapes, and a lone trailing
backslash, are passed through unchanged (regression guard against
misinterpreting arbitrary
\NNN-shaped text, and against an out-of-rangeslice on a short trailing backslash).
Confirmed
TestParseMounts_DecodesOctalEscapesandTestParseMounts_DecodesOctalEscapesInDevicefail against the pre-fix code(raw
\040etc. observed in the parsed fields) and pass after the fix.Ran
go build ./...,go vet ./...,go test ./... -race -cover(allpackages pass,
internal/collectorat 91.1%), andgolangci-lint run(0issues).
This isn't verifiable against a real escaped mountpoint on physical Pi
hardware in this environment, but the fixtures reproduce the exact
/proc/mountsencoding documented inproc(5)and observed by the kernelfor such paths.
✅ Checklist
General
go test ./... -race -coverpasses locally).go vet ./...andgolangci-lint runare clean.ARCHITECTURE.mdif this changes a documented design decision. (not applicable — internal parsing detail, no documented design decision changes)REST API / configuration / packaging
docs/API.mdto reflect a REST API change. (not applicable — no API shape change; mountpoint/device strings are now correctly decoded, not restructured)/api/v1/...response shapes, or a new API version (/api/v2/...) was introduced instead.README.md/packaging/pimonitor.example.yamlto reflect a new or changed configuration option. (not applicable — no new configuration)packaging/install.shor the systemd units if this changes installation/packaging, and kept the unprivileged/privileged service split intact (seeSECURITY.md). (not applicable)⏭ Next Steps
None — this closes out the follow-up noted in #109.