Conversation
✅ Deploy Preview for urunc canceled.
|
Anand-240
force-pushed
the
fix/mountinfo-octal-unescape
branch
from
July 31, 2026 12:38
8429133 to
2386752
Compare
Contributor
|
Hello @Anand-240 , can you bring this up-to-date with the main branch? |
The kernel octal-escapes spaces, tabs, newlines and backslashes in the root and mount point fields of /proc/self/mountinfo. getMountInfo was comparing the raw, still-escaped mount point field against the caller's real path, so any bind-mount source containing one of those characters never matched and getBlockVolumes silently skipped it instead of attaching it as a block device. Switch to moby/sys/mountinfo.GetMounts, which already decodes the escaping, instead of hand-splitting the mountinfo lines. The matching logic is split into findMountInfo so it can be exercised with synthetic mountinfo entries in a test. Fixes: urunc-dev#867 Signed-off-by: Anand-240 <anandprakashsrivastava68@gmail.com>
Anand-240
force-pushed
the
fix/mountinfo-octal-unescape
branch
from
September 16, 2026 14:00
2386752 to
e6133ac
Compare
✅ Deploy Preview for urunc canceled.
|
Contributor
Author
|
rebased onto main, no conflicts. CI should run now. |
cmainas
reviewed
Sep 29, 2026
cmainas
left a comment
cmainas
left a comment
Contributor
There was a problem hiding this comment.
Thank you @Anand-240 for the rebase. Overall the PR looks good and I would like to see this get merged mostly because it replaces the custom logic of reading /pros/self/mountinfo. However, the comments need to reflect the actual code and some old comments need to get updated too.
Comment on lines
+60
to
+64
| // | ||
| // We rely on moby/sys/mountinfo to parse /proc/self/mountinfo instead of | ||
| // splitting the raw lines ourselves, because the kernel octal-escapes | ||
| // spaces, tabs, newlines and backslashes in the root and mount point | ||
| // fields (see proc(5)), and mountinfo.GetMounts() already decodes them. |
Contributor
There was a problem hiding this comment.
No reason for this comment.
Comment on lines
+74
to
+77
| // findMountInfo scans already-parsed mountinfo entries for the one mounted | ||
| // at path. It is split out from getMountInfo so the matching logic can be | ||
| // unit tested against synthetic mounts, without depending on the real | ||
| // /proc/self/mountinfo of the process running the test. |
Contributor
There was a problem hiding this comment.
Please keep the comments aligned with the actual functionality of a function and not for unit tests or others.
| // at path. It is split out from getMountInfo so the matching logic can be | ||
| // unit tested against synthetic mounts, without depending on the real | ||
| // /proc/self/mountinfo of the process running the test. | ||
| func findMountInfo(mounts []*mountinfo.Info, path string) (types.BlockDevParams, error) { |
Contributor
There was a problem hiding this comment.
Please use a different name for mounts (for example mountInfo).
| blockDev.Source = m.Source | ||
| blockDev.FsType = m.FSType | ||
| blockDev.MountPoint = path | ||
| // Keep the mount VFS options (field 6 of mountinfo) |
Contributor
There was a problem hiding this comment.
We need to update this comment. It is stale with these changes.
This branch has not been deployed
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
2 participants
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.
Description
getMountInfo()inpkg/unikontainers/block.gofigures out whether a bind-mount source is itself a mount point by scanning/proc/self/mountinfoand comparing the mount point field against the given path with a plain string comparison (preDash[4] == path).The kernel octal-escapes spaces, tabs, newlines and backslashes in the root and mount point fields of that file (e.g. a space becomes
\040). The old code read those fields straight off the file and never unescaped them, so a mount whose path contained any of those characters could never match,getMountInforeturnedErrMountpoint, andgetBlockVolumes()silently skipped attaching it as a block device with no error surfaced anywhere.This switches
getMountInfoto usemoby/sys/mountinfo.GetMounts, which is already a dependency and already used elsewhere in this file (mountinfo.MountedinrestoreBlockVolumes). That library decodes the escaping while parsing, so the comparison works correctly. The actual matching logic is pulled out intofindMountInfo(mounts []*mountinfo.Info, path string)so it can be tested against synthetic mount entries instead of depending on the real/proc/self/mountinfoof whatever process runs the tests.Related issues
How was this tested?
go build ./...andgo vet ./...on Linux (cross-compiled from macOS, then verified inside agolangcontainer since the affected code andunix/loop-device syscalls it depends on are Linux-only).go test ./pkg/unikontainers/...in a Linux container: existingTestGetBlockDevicestill passes (it exercisesgetMountInfo("/proc")against the real mountinfo of the test process).TestFindMountInfoEscapedPath, which builds a synthetic mountinfo line with a mount point escaped the way the kernel would escape it (space, tab, backslash), parses it withmountinfo.GetMountsFromReader, and checksfindMountInfomatches it against the real, unescaped path. This reproduces the bug and fails on the old code path, passes with the fix.TestCopyFile,TestMoveFile) are pre-existing and unrelated. They fail identically on unmodifiedmainwhen run as root inside the container (permission-denied checks don't trigger for root).golangci-lint/hypervisor e2e setup available in this environment, somake lintand the e2e suites were not run locally; flagging that here rather than checking those boxes.LLM usage
Claude (Anthropic, model: claude-sonnet-5) was used to investigate the bug, write the fix, and write this description. All changes were reviewed and tested by me before opening this PR, per the project's LLM policy.
Checklist
make lint). Not run locally, see note above.make test_ctr,make test_nerdctl,make test_docker,make test_crictl). Not run locally, see note above.