Remove SMB file sharing for Windows #4879
Conversation
WalkthroughAdds a vendored 9P implementation and a new fs9p server, replaces Windows SMB flow with 9P (daemon listeners, VM mount path, drive tags), removes SMB/MSI and Windows-specific shared-dir credentials, and simplifies shared-dir configuration and driver helper code. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant Daemon as CRC Daemon
participant FS as fs9p Server
participant VM as Guest VM
User->>Daemon: start daemon (Windows)
Daemon->>FS: New9pServer(listener TCP/GatewayIP:Plan9TcpPort or vsock)
Daemon->>FS: Start() (async) -> ErrChan monitored
FS-->>Daemon: async serve / startup error via ErrChan
Daemon-->>User: daemon ready
User->>Daemon: start VM
Daemon->>VM: boot (shared-dir config type=9p, tags dirN)
VM->>FS: mount 9p over vsock
alt vsock unavailable
VM->>FS: mount 9p over TCP (GatewayIP:564)
end
VM-->>Daemon: mount result
sequenceDiagram
autonumber
participant VMClient as VM 9P client
participant Proto as p9/proto
participant DirFS as p9.Dir (filesystem)
participant Server as fs9p Server
VMClient->>Proto: Tversion(msize)
Proto->>VMClient: Rversion(msize)
VMClient->>Proto: Tattach
Proto->>DirFS: Attach(user, aname)
DirFS-->>Proto: Attachment
Proto-->>VMClient: Rattach
VMClient->>Proto: Topen / Tread / Twrite
Proto->>DirFS: Open/Read/Write/Stat...
DirFS-->>Proto: results
Proto-->>VMClient: Ropen / Rread / Rwrite
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (10)
💤 Files with no reviewable changes (7)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧠 Learnings (1)📓 Common learnings🧬 Code graph analysis (1)pkg/crc/machine/start.go (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 23
🧹 Nitpick comments (25)
vendor/github.com/DeedleFake/p9/addr_other.go (2)
11-16: Doc comment misleading for non-Unix build tagThis file is compiled on non-Linux/non-Darwin platforms, yet the comment explains Unix-like behavior. Clarify the comment to avoid confusion.
Apply this diff:
-// current namespace. On Unix-like systems, this is -// /tmp/ns.$USER.$DISPLAY. +// current namespace. On Unix-like systems, the path is typically +// /tmp/ns.$USER.$DISPLAY, but for non-Unix platforms this implementation +// uses os.TempDir() as the base.
1-1: Add modern build constraint for forward-compatibilityConsider adding the //go:build form alongside the legacy // +build for clarity and future-proofing. This mirrors Go's recommended syntax since 1.17.
Apply this diff:
+//go:build !linux && !darwin // +build !linux,!darwinvendor/github.com/DeedleFake/p9/internal/debug/debug.go (2)
1-1: Add //go:build for consistencyAdd the modern //go:build constraint alongside the legacy form.
Apply this diff:
+//go:build p9debug // +build p9debug
10-12: Optional: append newline to logsCurrent logging will not append a newline, which can jumble outputs in stderr across multiple calls. If callers don't consistently include "\n", consider appending one here.
Apply this diff:
-func Log(str string, args ...interface{}) { - fmt.Fprintf(os.Stderr, str, args...) -} +func Log(str string, args ...interface{}) { + if len(args) == 0 && len(str) > 0 && str[len(str)-1] == '\n' { + fmt.Fprint(os.Stderr, str) + return + } + fmt.Fprintf(os.Stderr, str+"\n", args...) +}vendor/github.com/DeedleFake/p9/README.md (1)
1-52: Vendor docs violate markdownlint rules; exclude vendor from lintingmarkdownlint is flagging heading styles and hard tabs in vendored README. Vendor content should generally be excluded from linters to avoid false positives and churn.
Consider updating your markdownlint configuration to ignore vendor/:
Example .markdownlint-cli2.yaml at repo root:
- ignores:
- "vendor/**"
If you prefer to keep linting vendor content, we can convert headings to setext style and replace tabs with spaces in this file.
vendor/github.com/DeedleFake/p9/addr_unix.go (1)
1-1: Add //go:build for clarityAdd the modern build constraint along with the legacy tag.
Apply this diff:
+//go:build linux || darwin // +build linux darwinvendor/github.com/DeedleFake/p9/proto/encoding.go (2)
82-84: Avoid encoding uintptr in a wire protocolIncluding reflect.Uintptr introduces arch-dependent sizes. It's safer to reject uintptr or normalize it to a fixed width.
Apply this diff to drop uintptr support:
- case reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64, reflect.Uintptr: + case reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64:
171-171: Prefer safe []byte->string conversion over unsafe.PointerThe unsafe conversion ties the string lifetime to a transient []byte backing array and relies on runtime internals. For robustness, use string(buf). If performance is a concern, justify with benchmarks and limit to hot paths.
Apply this diff:
- v.SetString(*(*string)(unsafe.Pointer(&buf))) + v.SetString(string(buf))pkg/crc/constants/constants.go (1)
60-60: Validate msize across client implementationsConfirm that 1MiB msize works with the kernel 9p client(s) you target (Linux/macOS guests) and matches mount-time options. If necessary, consider making it configurable to ease tuning for performance/path MTU constraints.
vendor/github.com/DeedleFake/p9/dir_other.go (1)
1-1: Add go:build constraint for Go 1.17+ compatibilityThe legacy // +build form is still supported, but pairing it with //go:build improves tooling compatibility.
Apply this diff:
+//go:build !linux && !darwin && !plan9 && !windows // +build !linux,!darwin,!plan9,!windowsvendor/github.com/DeedleFake/p9/encoding.go (1)
16-30: Graceful EOF handling is good; consider treating ErrUnexpectedEOF as end-of-stream too.proto.Read may return io.ErrUnexpectedEOF on truncated directory streams. If your intent is “read until the stream ends,” it’s reasonable to treat both EOF and ErrUnexpectedEOF as a normal termination and return the accumulated entries.
Apply within this range:
- err := proto.Read(r, &stat) - if err != nil { - if err == io.EOF { - err = nil - } - return entries, err - } + err := proto.Read(r, &stat) + if err != nil { + if errors.Is(err, io.EOF) || errors.Is(err, io.ErrUnexpectedEOF) { + return entries, nil + } + return entries, err + }Additionally, add the import:
// add to imports "errors"vendor/github.com/DeedleFake/p9/internal/util/util.go (3)
8-13: Fix misleading comment about size semantics of N.io.LimitedReader already uses int64, which is larger than uint32 on all architectures. The current comment suggests uint32 “allows larger sizes,” which is inaccurate. Recommend clarifying the rationale and the cap.
-// LimitedReader is a reimplementation of io.LimitedReader with two -// main differences: -// -// * N is a uint32, allowing for larger sizes on 32-bit systems. -// * A custom error can be returned if N becomes zero. +// LimitedReader is similar to io.LimitedReader with two differences: +// +// * N is a uint32 (io.LimitedReader uses int64). This keeps the type small and +// consistent across architectures but caps the readable size at 4 GiB. +// * A custom error can be returned when the limit is exhausted.
27-31: Use equality check for unsigned counter.N is uint32; use N == 0 instead of N <= 0 for clarity.
- if lr.N <= 0 { + if lr.N == 0 { return 0, lr.err() }
41-44: Align doc with implementation for Errorf.The comment mentions “nil or io.EOF,” but the code only returns io.EOF directly. Either expand the implementation or narrow the comment. Suggest tightening the comment.
-// Errorf is a variant of fmt.Errorf that returns an error being -// wrapped directly if it is one of a number of specific values, such -// as nil or io.EOF. +// Errorf is a variant of fmt.Errorf that returns io.EOF directly if it +// appears among the arguments; otherwise it formats the error.vendor/github.com/DeedleFake/p9/dir_plan9.go (2)
10-31: Populate QID fields in DirEntry when available.When syscall.Dir is present, you can also fill DirEntry.Path and DirEntry.Version from sys.Qid to avoid an extra stat/QID lookup elsewhere.
return DirEntry{ FileMode: ModeFromOS(fi.Mode()), ATime: time.Unix(int64(sys.Atime), 0), MTime: fi.ModTime(), Length: uint64(fi.Size()), EntryName: fi.Name(), UID: sys.Uid, GID: sys.Gid, MUID: sys.Muid, + Path: sys.Qid.Path, + Version: sys.Qid.Vers, }
39-42: Clarify error message to include the concrete type.Improve diagnosability by naming the expected concrete type.
- return QID{}, errors.New("failed to get QID: FileInfo was not Dir") + return QID{}, errors.New("failed to get QID: FileInfo.Sys() was not *syscall.Dir")vendor/github.com/DeedleFake/p9/dir_linux.go (2)
52-55: Clarify error message to include the concrete type.Improves diagnosability.
- if sys == nil { - return QID{}, errors.New("failed to get QID: FileInfo was not Stat_t") - } + if sys == nil { + return QID{}, errors.New("failed to get QID: FileInfo.Sys() was not *syscall.Stat_t") + }
23-33: Optional: Fallback to numeric UID/GID when lookups fail.LookupId/LookupGroupId can fail or be slow; consider falling back to numeric strings to retain information.
var uname string uid, err := user.LookupId(strconv.FormatUint(uint64(sys.Uid), 10)) if err == nil { uname = uid.Username + } else { + uname = strconv.FormatUint(uint64(sys.Uid), 10) } var gname string gid, err := user.LookupGroupId(strconv.FormatUint(uint64(sys.Gid), 10)) if err == nil { gname = gid.Name + } else { + gname = strconv.FormatUint(uint64(sys.Gid), 10) }vendor/github.com/DeedleFake/p9/proto/server.go (1)
80-93: Msizer handling: prevent double size negotiation warning spamThe setter.Do already guards the once-only behavior, but the outer check logs a warning even when the race resolves to first set wins. Consider moving the “already set” warning inside the Do block and only emit if setter wasn’t executed. Optional improvement.
vendor/github.com/DeedleFake/p9/proto/proto.go (1)
101-108: Dead branch on invalid type check.Inside
if t == nil { ... }the nestedif err != nil { return ... }can never trigger;errwasn’t modified after the previous successful read. This is dead code and reduces clarity.- t := p.TypeFromID(msgType) - if t == nil { - if err != nil { - return nil, NoTag, err - } - - return nil, NoTag, util.Errorf("receive: invalid message type: %v", msgType) - } + t := p.TypeFromID(msgType) + if t == nil { + return nil, NoTag, util.Errorf("receive: invalid message type: %v", msgType) + }vendor/github.com/DeedleFake/p9/remote.go (2)
127-135: Variable shadowing obscures receiver; rename local for clarity.
file, err := file.walk(p)shadows the method receiver, reducing readability. Use a different name and avoid confusion.- file, err := file.walk(p) + next, err := file.walk(p) if err != nil { return err } // Close is not necessary. Remove is also a clunk. - return file.Remove("") + return next.Remove("")
323-331: Same shadowing issue in Stat(); rename local.Avoid shadowing the receiver to keep code unambiguous.
- file, err := file.walk(p) + next, err := file.walk(p) if err != nil { return DirEntry{}, err } - defer file.Close() + defer next.Close() - return file.Stat("") + return next.Stat("")vendor/github.com/DeedleFake/p9/fs.go (2)
179-186: Avoid unsafe string-to-byte conversion when hashing QID path.Using
unsafeto convert string to []byte is brittle and can break across Go versions. The allocation saved here is negligible compared to a SHA-256. Prefer a safe conversion.- sum := sha256.Sum256(*(*[]byte)(unsafe.Pointer(&p))) + sum := sha256.Sum256([]byte(p)) path := binary.LittleEndian.Uint64(sum[:])Additionally remove the now-unused
unsafeimport from this file’s imports.
To support import cleanup, apply this change to the import block:// Remove the "unsafe" import from the import list in this file.
446-527: Directory read handling is intentionally spec-relaxed; document client expectations.The offset handling for directory reads is relaxed (offsets other than 0 are ignored). This is acceptable since the implementation returns the full entry list, but client-side code must not rely on precise offset semantics. Consider adding a short doc comment to the public-facing server wiring to set expectations.
vendor/github.com/DeedleFake/p9/stat.go (1)
135-153: Minor: unsafe-to-string conversion in String() is a trade-off.Using unsafe to avoid allocation is fine in vendored perf-sensitive code, but note it ties to Go’s internal string representation. If we ever fork this lib, consider a safe
string(buf)for maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (43)
cmd/crc/cmd/daemon.go(4 hunks)cmd/crc/cmd/start.go(0 hunks)go.mod(1 hunks)packaging/windows/product.wxs.template(0 hunks)pkg/crc/config/settings.go(1 hunks)pkg/crc/constants/constants.go(1 hunks)pkg/crc/machine/driver.go(0 hunks)pkg/crc/machine/driver_darwin.go(0 hunks)pkg/crc/machine/driver_linux.go(0 hunks)pkg/crc/machine/driver_windows.go(0 hunks)pkg/crc/machine/libhvee/driver_windows.go(2 hunks)pkg/crc/machine/start.go(2 hunks)pkg/drivers/libhvee/libhvee_windows.go(0 hunks)pkg/drivers/libhvee/powershell_windows.go(0 hunks)pkg/fileserver/fs9p/server.go(1 hunks)pkg/fileserver/fs9p/shares.go(1 hunks)vendor/github.com/DeedleFake/p9/LICENSE(1 hunks)vendor/github.com/DeedleFake/p9/README.md(1 hunks)vendor/github.com/DeedleFake/p9/addr.go(1 hunks)vendor/github.com/DeedleFake/p9/addr_other.go(1 hunks)vendor/github.com/DeedleFake/p9/addr_unix.go(1 hunks)vendor/github.com/DeedleFake/p9/client.go(1 hunks)vendor/github.com/DeedleFake/p9/dir.go(1 hunks)vendor/github.com/DeedleFake/p9/dir_darwin.go(1 hunks)vendor/github.com/DeedleFake/p9/dir_linux.go(1 hunks)vendor/github.com/DeedleFake/p9/dir_other.go(1 hunks)vendor/github.com/DeedleFake/p9/dir_plan9.go(1 hunks)vendor/github.com/DeedleFake/p9/dir_windows.go(1 hunks)vendor/github.com/DeedleFake/p9/doc.go(1 hunks)vendor/github.com/DeedleFake/p9/encoding.go(1 hunks)vendor/github.com/DeedleFake/p9/fs.go(1 hunks)vendor/github.com/DeedleFake/p9/internal/debug/debug.go(1 hunks)vendor/github.com/DeedleFake/p9/internal/debug/nodebug.go(1 hunks)vendor/github.com/DeedleFake/p9/internal/util/util.go(1 hunks)vendor/github.com/DeedleFake/p9/msg.go(1 hunks)vendor/github.com/DeedleFake/p9/p9.go(1 hunks)vendor/github.com/DeedleFake/p9/proto/client.go(1 hunks)vendor/github.com/DeedleFake/p9/proto/encoding.go(1 hunks)vendor/github.com/DeedleFake/p9/proto/proto.go(1 hunks)vendor/github.com/DeedleFake/p9/proto/server.go(1 hunks)vendor/github.com/DeedleFake/p9/remote.go(1 hunks)vendor/github.com/DeedleFake/p9/stat.go(1 hunks)vendor/modules.txt(1 hunks)
💤 Files with no reviewable changes (8)
- pkg/crc/machine/driver_darwin.go
- pkg/crc/machine/driver_windows.go
- cmd/crc/cmd/start.go
- pkg/drivers/libhvee/libhvee_windows.go
- pkg/drivers/libhvee/powershell_windows.go
- pkg/crc/machine/driver_linux.go
- pkg/crc/machine/driver.go
- packaging/windows/product.wxs.template
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: redbeam
PR: crc-org/crc#4866
File: pkg/fileserver/fs9p/shares.go:0-0
Timestamp: 2025-08-06T09:48:10.441Z
Learning: The vsock implementation in pkg/fileserver/fs9p/shares.go was temporarily removed because 9pfs doesn't fully support it yet. It will be added back when 9pfs provides proper vsock support.
📚 Learning: 2025-08-06T09:48:10.441Z
Learnt from: redbeam
PR: crc-org/crc#4866
File: pkg/fileserver/fs9p/shares.go:0-0
Timestamp: 2025-08-06T09:48:10.441Z
Learning: The vsock implementation in pkg/fileserver/fs9p/shares.go was temporarily removed because 9pfs doesn't fully support it yet. It will be added back when 9pfs provides proper vsock support.
Applied to files:
vendor/github.com/DeedleFake/p9/README.mdcmd/crc/cmd/daemon.gopkg/fileserver/fs9p/server.govendor/github.com/DeedleFake/p9/fs.gopkg/fileserver/fs9p/shares.go
🧬 Code Graph Analysis (28)
vendor/github.com/DeedleFake/p9/dir_other.go (1)
vendor/github.com/DeedleFake/p9/stat.go (3)
DirEntry(255-267)FileMode(23-23)ModeFromOS(43-78)
vendor/github.com/DeedleFake/p9/internal/debug/debug.go (1)
vendor/github.com/DeedleFake/p9/internal/debug/nodebug.go (1)
Log(5-5)
vendor/github.com/DeedleFake/p9/encoding.go (2)
vendor/github.com/DeedleFake/p9/stat.go (2)
DirEntry(255-267)Stat(156-168)vendor/github.com/DeedleFake/p9/proto/encoding.go (2)
Read(34-38)Write(25-31)
vendor/github.com/DeedleFake/p9/addr_unix.go (1)
vendor/github.com/DeedleFake/p9/addr_other.go (1)
NamespaceDir(17-29)
vendor/github.com/DeedleFake/p9/addr_other.go (1)
vendor/github.com/DeedleFake/p9/addr_unix.go (1)
NamespaceDir(17-29)
vendor/github.com/DeedleFake/p9/dir_plan9.go (3)
vendor/github.com/DeedleFake/p9/stat.go (4)
DirEntry(255-267)FileMode(23-23)ModeFromOS(43-78)Stat(156-168)vendor/github.com/DeedleFake/p9/dir.go (1)
Dir(16-16)vendor/github.com/DeedleFake/p9/p9.go (2)
QID(42-46)Version(10-10)
vendor/github.com/DeedleFake/p9/dir_darwin.go (3)
vendor/github.com/DeedleFake/p9/stat.go (3)
DirEntry(255-267)FileMode(23-23)ModeFromOS(43-78)vendor/github.com/DeedleFake/p9/dir.go (1)
Dir(16-16)vendor/github.com/DeedleFake/p9/p9.go (3)
QID(42-46)QIDType(49-49)Version(10-10)
vendor/github.com/DeedleFake/p9/internal/debug/nodebug.go (1)
vendor/github.com/DeedleFake/p9/internal/debug/debug.go (1)
Log(10-12)
vendor/github.com/DeedleFake/p9/dir_windows.go (1)
vendor/github.com/DeedleFake/p9/stat.go (3)
DirEntry(255-267)FileMode(23-23)ModeFromOS(43-78)
vendor/github.com/DeedleFake/p9/addr.go (2)
vendor/github.com/DeedleFake/p9/addr_other.go (1)
NamespaceDir(17-29)vendor/github.com/DeedleFake/p9/addr_unix.go (1)
NamespaceDir(17-29)
vendor/github.com/DeedleFake/p9/client.go (5)
vendor/github.com/DeedleFake/p9/proto/client.go (3)
Client(18-31)NewClient(35-56)Dial(60-67)vendor/github.com/DeedleFake/p9/msg.go (8)
Proto(75-77)Tversion(79-82)Tversion(84-84)Rversion(86-89)Tauth(95-99)Rauth(101-103)Tattach(105-110)Rattach(112-114)vendor/github.com/DeedleFake/p9/proto/proto.go (1)
Proto(38-41)vendor/github.com/DeedleFake/p9/p9.go (3)
Version(10-10)NoFID(18-18)QID(42-46)vendor/github.com/DeedleFake/p9/remote.go (1)
Remote(19-27)
pkg/crc/machine/libhvee/driver_windows.go (1)
vendor/github.com/crc-org/machine/libmachine/drivers/base.go (1)
SharedDir(27-35)
vendor/github.com/DeedleFake/p9/proto/encoding.go (1)
vendor/github.com/DeedleFake/p9/internal/util/util.go (1)
Errorf(44-52)
vendor/github.com/DeedleFake/p9/dir_linux.go (3)
vendor/github.com/DeedleFake/p9/stat.go (4)
DirEntry(255-267)FileMode(23-23)ModeFromOS(43-78)Stat(156-168)vendor/github.com/DeedleFake/p9/dir.go (1)
Dir(16-16)vendor/github.com/DeedleFake/p9/p9.go (3)
QID(42-46)QIDType(49-49)Version(10-10)
cmd/crc/cmd/daemon.go (2)
pkg/crc/constants/constants.go (2)
Plan9Port(59-59)GetHomeDir(164-170)pkg/fileserver/fs9p/shares.go (2)
StartShares(25-56)Mount9p(12-15)
vendor/github.com/DeedleFake/p9/remote.go (6)
vendor/github.com/DeedleFake/p9/client.go (1)
Client(22-26)vendor/github.com/DeedleFake/p9/proto/client.go (1)
Client(18-31)vendor/github.com/DeedleFake/p9/p9.go (4)
QID(42-46)QIDType(49-49)Version(10-10)IOHeaderSize(70-70)vendor/github.com/DeedleFake/p9/msg.go (14)
Twalk(133-137)Rwalk(139-141)Topen(143-146)Ropen(148-151)Tcreate(153-158)Rcreate(160-163)Tremove(192-194)Tread(165-169)Rread(171-173)Twrite(175-179)Rwrite(181-183)Tclunk(185-187)Tstat(199-201)Rstat(203-205)vendor/github.com/DeedleFake/p9/stat.go (3)
FileMode(23-23)Stat(156-168)DirEntry(255-267)vendor/github.com/DeedleFake/p9/internal/util/util.go (1)
Errorf(44-52)
pkg/crc/config/settings.go (2)
pkg/crc/config/validations.go (1)
ValidateBool(19-25)pkg/crc/config/callbacks.go (1)
SuccessfullyApplied(36-38)
pkg/crc/machine/start.go (1)
pkg/os/exec.go (1)
RunPrivileged(48-59)
pkg/fileserver/fs9p/server.go (3)
vendor/github.com/DeedleFake/p9/fs.go (2)
FileSystem(21-34)FSConnHandler(162-167)vendor/github.com/DeedleFake/p9/proto/server.go (1)
Serve(17-42)pkg/crc/constants/constants.go (1)
Plan9Msize(60-60)
vendor/github.com/DeedleFake/p9/p9.go (1)
vendor/github.com/DeedleFake/p9/stat.go (1)
FileMode(23-23)
vendor/github.com/DeedleFake/p9/fs.go (8)
vendor/github.com/DeedleFake/p9/stat.go (4)
Stat(156-168)DirEntry(255-267)StatChanges(316-318)FileMode(23-23)vendor/github.com/DeedleFake/p9/p9.go (7)
QID(42-46)QIDType(49-49)IOHeaderSize(70-70)Version(10-10)QTAuth(58-58)NoFID(18-18)QTDir(54-54)vendor/github.com/DeedleFake/p9/proto/server.go (3)
MessageHandler(135-139)ConnHandler(112-114)ConnHandlerFunc(125-125)vendor/github.com/DeedleFake/p9/internal/debug/nodebug.go (1)
Log(5-5)vendor/github.com/DeedleFake/p9/internal/debug/debug.go (1)
Log(10-12)vendor/github.com/DeedleFake/p9/msg.go (2)
Tversion(79-82)Tversion(84-84)vendor/github.com/DeedleFake/p9/client.go (1)
ErrUnsupportedVersion(14-14)vendor/github.com/DeedleFake/p9/encoding.go (1)
WriteDir(33-42)
vendor/github.com/DeedleFake/p9/stat.go (3)
vendor/github.com/DeedleFake/p9/p9.go (3)
QIDType(49-49)QID(42-46)Version(10-10)vendor/github.com/DeedleFake/p9/proto/encoding.go (3)
Write(25-31)Read(34-38)Size(14-20)vendor/github.com/DeedleFake/p9/internal/util/util.go (1)
LimitedReader(13-17)
vendor/github.com/DeedleFake/p9/proto/proto.go (3)
vendor/github.com/DeedleFake/p9/proto/encoding.go (3)
Read(34-38)Write(25-31)Size(14-20)vendor/github.com/DeedleFake/p9/p9.go (1)
NoTag(15-15)vendor/github.com/DeedleFake/p9/internal/util/util.go (2)
Errorf(44-52)LimitedReader(13-17)
pkg/fileserver/fs9p/shares.go (1)
pkg/fileserver/fs9p/server.go (2)
Server(15-22)New9pServer(26-62)
vendor/github.com/DeedleFake/p9/dir.go (3)
vendor/github.com/DeedleFake/p9/stat.go (4)
Stat(156-168)DirEntry(255-267)StatChanges(316-318)FileMode(23-23)vendor/github.com/DeedleFake/p9/fs.go (4)
File(100-119)Attachment(44-74)FileSystem(21-34)QIDFS(89-91)vendor/github.com/DeedleFake/p9/p9.go (7)
OWRITE(27-27)ORDWR(28-28)OEXEC(29-29)OTRUNC(31-31)OCEXEC(32-32)ORCLOSE(33-33)OREAD(26-26)
vendor/github.com/DeedleFake/p9/proto/server.go (2)
vendor/github.com/DeedleFake/p9/msg.go (1)
Proto(75-77)vendor/github.com/DeedleFake/p9/proto/proto.go (1)
Proto(38-41)
vendor/github.com/DeedleFake/p9/proto/client.go (6)
vendor/github.com/DeedleFake/p9/msg.go (1)
Proto(75-77)vendor/github.com/DeedleFake/p9/proto/proto.go (3)
Proto(38-41)NoTag(27-27)ErrClientClosed(22-22)vendor/github.com/DeedleFake/p9/client.go (3)
NewClient(30-32)Client(22-26)Dial(36-43)vendor/github.com/DeedleFake/p9/internal/debug/nodebug.go (1)
Log(5-5)vendor/github.com/DeedleFake/p9/internal/debug/debug.go (1)
Log(10-12)vendor/github.com/DeedleFake/p9/p9.go (1)
NoTag(15-15)
vendor/github.com/DeedleFake/p9/msg.go (6)
vendor/github.com/DeedleFake/p9/proto/proto.go (2)
NewProto(44-60)Proto(38-41)vendor/github.com/DeedleFake/p9/p9.go (2)
Version(10-10)QID(42-46)vendor/github.com/DeedleFake/p9/proto/client.go (1)
P9NoTag(221-223)vendor/github.com/DeedleFake/p9/stat.go (3)
FileMode(23-23)Stat(156-168)ErrLargeStat(18-18)vendor/github.com/DeedleFake/p9/proto/encoding.go (2)
Write(25-31)Read(34-38)vendor/github.com/DeedleFake/p9/internal/util/util.go (1)
LimitedReader(13-17)
🪛 markdownlint-cli2 (0.17.2)
vendor/github.com/DeedleFake/p9/README.md
12-12: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
17-17: Hard tabs
Column: 1
(MD010, no-hard-tabs)
21-21: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
26-26: Hard tabs
Column: 1
(MD010, no-hard-tabs)
32-32: Hard tabs
Column: 1
(MD010, no-hard-tabs)
37-37: Hard tabs
Column: 1
(MD010, no-hard-tabs)
43-43: Hard tabs
Column: 1
(MD010, no-hard-tabs)
49-49: Hard tabs
Column: 1
(MD010, no-hard-tabs)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Konflux kflux-prd-rh02 / crc-binary-on-pull-request
- GitHub Check: build (macOS-14, 1.23)
- GitHub Check: Run OKD bundle with crc (1.23)
- GitHub Check: build (ubuntu-latest, 1.23)
- GitHub Check: build (windows-2022, 1.23)
- GitHub Check: build (macOS-13, 1.23)
🔇 Additional comments (26)
vendor/github.com/DeedleFake/p9/LICENSE (1)
1-21: MIT license file correctly included for vendored dependency.License content and attribution look correct for github.com/DeedleFake/p9.
vendor/modules.txt (1)
13-18: Vendoring metadata for p9 v0.6.12 looks consistent.Entries for root and subpackages are present and marked explicit.
vendor/github.com/DeedleFake/p9/internal/debug/nodebug.go (1)
1-5: No-op debug build variant is fine.Build tag and signature match the debug counterpart; safe to vendor as-is.
pkg/crc/machine/libhvee/driver_windows.go (2)
4-4: fmt import is justified by dynamic 9P tag generation.No issues.
39-45: No change needed — Windows uses TCP 9p, vsock support is disabled and 9p server does not rely on "dir%d" tagsShort summary: I verified that drivers set Tag = fmt.Sprintf("dir%d", i) client-side, but the fs9p server exposes a directory per net.Listener (no tag mapping) and vsock shares are disabled. Windows path already uses TCP/9p, so the original tag/vsock concern is not applicable.
Files to note:
- pkg/crc/machine/libhvee/driver_windows.go — sets Tag: fmt.Sprintf("dir%d", i) and Type: "9p"
- pkg/fileserver/fs9p/shares.go — StartVsockShares returns nil (vsock support removed); StartShares uses New9pServer(listener, path)
- pkg/fileserver/fs9p/server.go — New9pServer serves exposeDir on the provided net.Listener (no tag mapping)
- cmd/crc/cmd/daemon.go — creates a TCP listener and calls fs9p.StartShares(...)
- pkg/crc/machine/start.go — configureSharedDirs mounts "9p" via "9pfs 192.168.127.1" (TCP), not vsock
Conclusion: the original review suggestion to align 9p share tags and avoid vsock assumptions is incorrect for this codebase — Windows is already using the TCP/GVProxy 9p path and the server is listener-bound rather than tag-mapped.
Likely an incorrect or invalid review comment.
pkg/crc/machine/start.go (1)
209-211: LGTM: explanatory comment is accurateThe note about rpc.ServerError wrapping and the limitation of errors.Is() is correct and useful context here.
pkg/crc/config/settings.go (1)
87-88: Clarify EnableSharedDirs help text; do NOT enforce HostNetworkAccessShort summary: I verified the implementation — the 9P server is started on the CRC virtual-network gateway and the guest mounts that gateway address, so the shared-dir feature is served via the CRC virtual network (not by mounting host /). Therefore the proposed validator that forces HostNetworkAccess=true for user networking is incorrect: HostNetworkAccess controls NAT to the host loopback and is not required for the 9P server the daemon creates.
Files to note:
- pkg/crc/config/settings.go — AddSetting for EnableSharedDirs (update help text here)
- cmd/crc/cmd/daemon.go — vn.Listen on configuration.GatewayIP and fs9p.StartShares (9P server is bound to the virtual-network gateway)
- pkg/crc/machine/start.go — configureSharedDirs uses "9pfs 192.168.127.1" (guest mounts the gateway, mountpoint is not '/')
- pkg/fileserver/fs9p/server.go & shares.go — New9pServer / StartShares bind to the provided listener
- pkg/crc/constants/constants.go — Plan9Port = 564 (9P port)
Proposed help-text change (apply to pkg/crc/config/settings.go where EnableSharedDirs is registered):
- cfg.AddSetting(EnableSharedDirs, true, ValidateBool, SuccessfullyApplied, - "Mounts host's home directory at '/' in the CRC VM (true/false, default: true)") + cfg.AddSetting(EnableSharedDirs, true, ValidateBool, SuccessfullyApplied, + "Expose the host's home directory to the CRC VM via 9P (true/false, default: true). The share is served via the CRC virtual-network gateway (e.g. 192.168.127.1) and mounted inside the VM via 9P (not mounted at '/')")Validator guidance:
- Do not add a validator that requires HostNetworkAccess=true for NetworkMode=user. Verified evidence: daemon binds the 9P server to the CRC virtual network gateway (cmd/crc/cmd/daemon.go -> vn.Listen on configuration.GatewayIP) and the VM mounts that gateway address (pkg/crc/machine/start.go uses "9pfs 192.168.127.1"). HostNetworkAccess is a separate toggle that maps a virtual host IP to the host loopback and is not necessary for the 9P share.
Likely an incorrect or invalid review comment.
vendor/github.com/DeedleFake/p9/doc.go (1)
1-63: LGTM: Clear, useful package-level docsThe overview and examples set the right expectations for both client and server APIs. No code changes; nothing else to flag.
vendor/github.com/DeedleFake/p9/proto/encoding.go (1)
147-159: No change required — reads are already capped with LimitedReaderproto.Receive reads the message size, enforces msize, then wraps the connection with util.LimitedReader (N = size, E = ErrLargeMessage) and uses that reader for subsequent decoding, so decoder.decode runs against a length-limited reader.
- vendor/github.com/DeedleFake/p9/proto/proto.go: Receive(...) — reads size, checks msize, constructs lr := &util.LimitedReader{R: r, N: size, E: ErrLargeMessage} and calls Read(lr, ...)
- vendor/github.com/DeedleFake/p9/internal/util/util.go: LimitedReader implementation that returns the configured error when the limit is reached
- vendor/github.com/DeedleFake/p9/proto/encoding.go: decoder.decode handles slice lengths/allocations — safe when invoked via Receive
vendor/github.com/DeedleFake/p9/addr.go (1)
10-66: LGTM: ParseAddr covers common 9P forms and pseudo-port mappingSolid handling of “$namespace”, Unix sockets, host:port (including 9p/9fs), and proto!addr[!port] forms. No functional issues spotted.
vendor/github.com/DeedleFake/p9/p9.go (2)
21-39: LGTM: Open mode/flag constants consistent with 9P semanticsThe bit layout and the iota math are correct; no concerns.
41-66: LGTM: QID/QIDType and FileMode conversion look correctQIDType->FileMode shift aligns with 9P’s mode top bits. No issues.
pkg/crc/constants/constants.go (1)
59-60: Plan 9 constants addition looks goodUsing the standard 9P port (564) and a generous 1MiB msize is sensible for the initial integration.
cmd/crc/cmd/daemon.go (3)
27-27: Importing fs9p for 9P shares is appropriateThe new import aligns with the 9P home directory sharing introduced below.
181-185: Good: use net.JoinHostPort for listener addressThis is safer (IPv6-aware) and avoids string formatting pitfalls.
197-211: Good: consistent JoinHostPort usage for network listenerSame benefits here; consistent and correct.
vendor/github.com/DeedleFake/p9/dir_other.go (1)
7-14: LGTM: sane fallback entry mappingFor non-listed OSes, mapping mode/mtime/size/name is correct and minimal.
vendor/github.com/DeedleFake/p9/dir_windows.go (1)
9-27: Windows DirEntry mapping looks correct
- Safe type assertion on fi.Sys() and proper fallback when sys is nil.
- Using Filetime.Nanoseconds() with time.Unix(0, ...) is the idiomatic conversion.
vendor/github.com/DeedleFake/p9/dir_darwin.go (1)
46-62: Darwin QID generation approach is soundUsing inode for Path and deriving QID type from mode mirrors Linux behavior and is appropriate. Error on missing Stat_t keeps semantics clear.
vendor/github.com/DeedleFake/p9/encoding.go (1)
33-42: LGTM: WriteDir is straightforward and correct.Looped encode via proto.Write is clear and returns on first error as expected.
vendor/github.com/DeedleFake/p9/proto/client.go (2)
69-75: LGTM: clean shutdown pathClose() cancels the coordination goroutines and closes the underlying connection. This aligns with the reader/coord loop design.
176-208: Solid tagged/untagged messaging coordinationThe Send/reader/coord interplay correctly handles both tagged messages and the special untagged case (P9NoTag), with backpressure and cancellation paths.
vendor/github.com/DeedleFake/p9/dir.go (1)
18-21: Consider hardening against traversal in path resolutionDir.path simply joins the base with the provided p. If upstream always supplies normalized, component-wise paths (as expected by the 9P Walk semantics), this is fine. If not guaranteed, normalize and reject any attempt to escape the base (e.g., by resolving and verifying that the result has the base as a prefix).
Would you like me to add a safe-join helper that rejects absolute paths and any result escaping the base?
vendor/github.com/DeedleFake/p9/stat.go (1)
191-216: Stat encoding/decoding looks correct and spec-compliant.The size-prefix and LimitedReader enforcement align with 9P stat encoding, and error propagation is sound.
vendor/github.com/DeedleFake/p9/msg.go (2)
207-233: Rstat length-prefixing matches Stat.P9Encode; decode bounds are correct.The extra
+2accounts for Stat’s internal size field, and the LimitedReader bounds prevent overreads. This aligns with the 9P2000 spec.
44-72: Protocol registry is comprehensive and type-safe mapping looks good.The reflect-based registry setup matches the declared message types and provides a solid base for dispatch.
| if (msize > 0) && (size > msize) { | ||
| return nil, NoTag, util.Errorf("receive: %w", ErrLargeMessage) | ||
| } | ||
|
|
||
| lr := &util.LimitedReader{ | ||
| R: r, | ||
| N: size, | ||
| E: ErrLargeMessage, | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Off-by-4 framing bug: LimitedReader should bound to size-4 (size includes itself).
9P’s size field includes the 4 bytes of the size itself. After reading size, the remaining frame length is size-4. Using N: size allows reads to leak 4 bytes into the next frame on exact-boundary payloads. This breaks message framing and can cause hard-to-debug, nondeterministic errors.
Apply this diff to bound reads correctly and add a guard for undersized frames:
- if (msize > 0) && (size > msize) {
+ if (msize > 0) && (size > msize) {
return nil, NoTag, util.Errorf("receive: %w", ErrLargeMessage)
}
- lr := &util.LimitedReader{
- R: r,
- N: size,
- E: ErrLargeMessage,
- }
+ if size < 4 {
+ return nil, NoTag, util.Errorf("receive: invalid frame size: %d", size)
+ }
+ lr := &util.LimitedReader{
+ R: r,
+ N: size - 4,
+ E: ErrLargeMessage,
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (msize > 0) && (size > msize) { | |
| return nil, NoTag, util.Errorf("receive: %w", ErrLargeMessage) | |
| } | |
| lr := &util.LimitedReader{ | |
| R: r, | |
| N: size, | |
| E: ErrLargeMessage, | |
| } | |
| if (msize > 0) && (size > msize) { | |
| return nil, NoTag, util.Errorf("receive: %w", ErrLargeMessage) | |
| } | |
| if size < 4 { | |
| return nil, NoTag, util.Errorf("receive: invalid frame size: %d", size) | |
| } | |
| lr := &util.LimitedReader{ | |
| R: r, | |
| N: size - 4, | |
| E: ErrLargeMessage, | |
| } |
🤖 Prompt for AI Agents
In vendor/github.com/DeedleFake/p9/proto/proto.go around lines 85 to 94, the
LimitedReader is currently bounded with N: size which is off-by-4 because the 9P
size field includes its own 4 bytes; change the reader bound to N: size-4 and
add a guard that returns an error if size < 4 (undersized frame) before
constructing the LimitedReader so we never allow negative/invalid bounds or leak
4 bytes into the next frame.
| import ( | ||
| "io" | ||
| "log" | ||
| "net" | ||
| "sync" | ||
| ) | ||
|
|
There was a problem hiding this comment.
Break on non-EOF read errors and don’t dispatch a handler on failed Receive
Currently, after a Receive error, the loop logs and then proceeds to call the handler with a garbage message/tag, which can panic or send invalid responses. Also, closed connections (e.g., net.ErrClosed) should terminate the loop, not busy-loop and spam logs.
Apply this diff:
@@
-import (
+import (
+ "errors"
"io"
"log"
"net"
"sync"
)
@@
- tmsg, tag, err := p.Receive(c, msize)
+ tmsg, tag, err := p.Receive(c, msize)
if err != nil {
- if err == io.EOF {
- return
- }
-
- log.Printf("Error reading message: %v", err)
+ // Terminate on EOF or closed connection.
+ if err == io.EOF || errors.Is(err, net.ErrClosed) {
+ return
+ }
+ // For other errors, log and continue to try receiving again.
+ log.Printf("Error reading message: %v", err)
+ continue
}
mode(func() {
rmsg := handler.HandleMessage(tmsg)Also applies to: 71-79, 79-99
🤖 Prompt for AI Agents
In vendor/github.com/DeedleFake/p9/proto/server.go around lines 3-9, 71-79 and
79-99, the loop continues after a Receive error and calls the handler with
invalid data, and it busy-loops on closed connections; update the
receive-and-dispatch loop to: check the error returned by Receive and if err !=
nil then if errors.Is(err, io.EOF) or errors.Is(err, net.ErrClosed) break the
loop (cleanly close/return), otherwise log the error and break (do not call the
handler); only call the handler when Receive succeeds (no error) and ensure you
don’t use stale/garbage msg/tag after a failed Receive.
| panic(util.Errorf("Invalid whence: %v", whence)) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Don’t panic on invalid whence; return an error.
Library code should not panic for invalid input. Return the current position and a descriptive error instead.
- panic(util.Errorf("Invalid whence: %v", whence))
+ return int64(file.pos), util.Errorf("invalid whence: %v", whence)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| panic(util.Errorf("Invalid whence: %v", whence)) | |
| } | |
| return int64(file.pos), util.Errorf("invalid whence: %v", whence) | |
| } |
🤖 Prompt for AI Agents
In vendor/github.com/DeedleFake/p9/remote.go around lines 184-185, replace the
panic call for an invalid whence with returning the current position and an
error; specifically, remove panic(util.Errorf(...)) and instead return the
current offset/position variable (the value the function would report as the
current file position) along with util.Errorf("invalid whence: %v", whence) so
callers receive an error instead of the process aborting.
There was a problem hiding this comment.
Only took a quick look, but it’s unexpected to see the 9p code being added in the first commits before being disabled, and then to have the smb code removal.
Regarding the 9p code, some of the comments of coderabbit.ai (including in the vendor’ed code) deserve a closer look.
I based this branch on the 9p branch to avoid conflicts. This PR is expected to be merged as soon as 9p support is merged, so the 9p code will already be there.
I'll look at comments on my parts of the code, but what can be done about the vendored code? |
If they are actual issues, in particular issues with security impacts, if it’s not too hard to fix them, I’d file PRs upstream, this way we’ll get the fixes in our codebase when they are merged. |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packaging/windows/product.wxs.template (1)
95-103: Remove stray '~' in MSI condition stringspackaging/windows/product.wxs.template — occurrences of REMOVE~="ALL" at lines 96, 97, 98, 99, 100, 102. WiX conditions must be REMOVE="ALL" (no '
'); replace all REMOVE="ALL" with REMOVE="ALL".
♻️ Duplicate comments (11)
vendor/github.com/DeedleFake/p9/dir_darwin.go (1)
35-43: Fix Darwin build: Timespec.Unix() does not exist; use Sec/Nsec fieldsThis won’t compile on darwin; syscall.Timespec has Sec/Nsec, not Unix().
Please also send this fix upstream to DeedleFake/p9 so we don’t carry a long‑term vendor delta.
return DirEntry{ FileMode: ModeFromOS(fi.Mode()), - ATime: time.Unix(sys.Atimespec.Unix()), + ATime: time.Unix(int64(sys.Atimespec.Sec), int64(sys.Atimespec.Nsec)), MTime: fi.ModTime(), Length: uint64(fi.Size()), EntryName: fi.Name(), UID: uname, GID: gname, }#!/bin/bash # Confirm there are no other invalid Timespec.Unix() calls. rg -nP 'Atimespec\.Unix\(' -Spkg/crc/machine/libhvee/driver_windows.go (1)
39-45: Deterministic 9P tag mapping; keep in sync with server (duplicate)Sort the list before assigning tags to keep tag->share stable across runs and avoid drift with the 9P server’s aname. Also consider centralizing the tag format constant so both sides stay aligned.
@@ -import ( - "fmt" - "path/filepath" - "strings" -) +import ( + "fmt" + "path/filepath" + "strings" + "sort" +) @@ - for i, dir := range machineConfig.SharedDirs { + dirs := append([]string(nil), machineConfig.SharedDirs...) + sort.Strings(dirs) + for i, dir := range dirs { sharedDir := drivers.SharedDir{ Source: dir, Target: convertToUnixPath(dir), Tag: fmt.Sprintf("dir%d", i), Type: "9p", }cmd/crc/cmd/daemon.go (1)
253-266: Don’t drop 9P server handles; defer Stop() for clean shutdown (duplicate)Keep the returned servers and stop them to avoid goroutine/listener leaks.
// 9p home directory sharing if runtime.GOOS == "windows" { - if _, err := fs9p.StartHvsockShares([]fs9p.HvsockMount9p{{Path: constants.GetHomeDir(), HvsockGUID: constants.Plan9HvsockGUID}}); err != nil { + if servers, err := fs9p.StartHvsockShares([]fs9p.HvsockMount9p{{Path: constants.GetHomeDir(), HvsockGUID: constants.Plan9HvsockGUID}}); err != nil { logging.Warnf("Failed to start 9p file server on hvsock: %v", err) logging.Warnf("Falling back to 9p over TCP") listener9p, err := vn.Listen("tcp", net.JoinHostPort(configuration.GatewayIP, fmt.Sprintf("%d", constants.Plan9TcpPort))) if err != nil { return err } - if _, err := fs9p.StartShares([]fs9p.Mount9p{{Listener: listener9p, Path: constants.GetHomeDir()}}); err != nil { + if serversTCP, err := fs9p.StartShares([]fs9p.Mount9p{{Listener: listener9p, Path: constants.GetHomeDir()}}); err != nil { return err + } else { + defer func() { + for _, s := range serversTCP { + if stopErr := s.Stop(); stopErr != nil { + logging.Errorf("Error stopping 9p server: %v", stopErr) + } + } + }() } + } else { + defer func() { + for _, s := range servers { + if stopErr := s.Stop(); stopErr != nil { + logging.Errorf("Error stopping 9p server: %v", stopErr) + } + } + }() } }pkg/crc/machine/start.go (1)
244-256: 9P mount should be privileged, idempotent, and explicit; avoid brittle helperMounting requires root; using sshRunner.Run will likely fail. Also, add mountpoint check and use explicit 9P mount with options and tag for TCP fallback. Keep the hvsock attempt if needed, but run it privileged.
case "9p": - // change owner to core user to allow mounting to it as a non-root user - if _, _, err := sshRunner.RunPrivileged("Changing owner of mount directory", "chown core:core", mount.Target); err != nil { - return err - } - if _, _, err := sshRunner.Run("9pfs -V -p", fmt.Sprintf("%d", constants.Plan9HvsockPort), mount.Target); err != nil { - logging.Warnf("Failed to connect to 9p server over hvsock: %v", err) - logging.Warnf("Falling back to 9p over TCP") - if _, _, err := sshRunner.Run("9pfs 192.168.127.1", mount.Target); err != nil { - return err - } - } + // idempotency + if _, _, err := sshRunner.RunPrivileged("Check if already mounted", "mountpoint", "-q", mount.Target); err == nil { + logging.Debugf("Already mounted, skipping: %s", mount.Target) + continue + } + // change owner to core user to allow non-root access after mounting + if _, _, err := sshRunner.RunPrivileged("Changing owner of mount directory", "chown", "core:core", mount.Target); err != nil { + return err + } + // Try hvsock via helper (if present) with elevated privileges + if _, _, err := sshRunner.RunPrivileged("Mounting 9p over hvsock", "9pfs", "-V", "-p", fmt.Sprintf("%d", constants.Plan9HvsockPort), mount.Target); err != nil { + logging.Warnf("Failed to connect to 9p server over hvsock: %v", err) + logging.Warnf("Falling back to 9p over TCP") + opts := fmt.Sprintf("trans=tcp,port=%d,msize=%d,cache=loose,version=9p2000.L,aname=%s", constants.Plan9TcpPort, constants.Plan9Msize, mount.Tag) + if _, _, err := sshRunner.RunPrivileged( + fmt.Sprintf("Mounting %s (9p tag %s) over TCP", mount.Target, mount.Tag), + "mount", "-t", "9p", "-o", opts, "192.168.127.1", mount.Target, + ); err != nil { + return err + } + }Follow-ups:
- Consider gating the hvsock attempt behind a feature flag until client support stabilizes.
pkg/fileserver/fs9p/shares.go (1)
24-61: Fix loop-variable capture; close listener on per-mount failures; preallocate sliceAvoid capturing loop vars in defer/goroutine, and ensure the current mount’s listener is closed on errors before the defer is installed. Also preallocate the servers slice.
-func StartShares(mounts []Mount9p) (servers []*Server, defErr error) { - servers9p := []*Server{} +func StartShares(mounts []Mount9p) (servers []*Server, defErr error) { + servers = make([]*Server, 0, len(mounts)) for _, m := range mounts { server, err := New9pServer(m.Listener, m.Path) if err != nil { - return nil, fmt.Errorf("serving directory %s on %s: %w", m.Path, m.Listener.Addr().String(), err) + _ = m.Listener.Close() // avoid leaking a listener on constructor failure + return nil, fmt.Errorf("serving directory %s on %s: %w", m.Path, m.Listener.Addr().String(), err) } - err = server.Start() + if err := server.Start(); err != nil { + _ = server.Stop() // close listener if start fails before defer is in place if err != nil { - return nil, fmt.Errorf("starting 9p server for directory %s: %w", m.Path, err) + return nil, fmt.Errorf("starting 9p server for directory %s: %w", m.Path, err) } - } - - servers9p = append(servers9p, server) + } + servers = append(servers, server) - defer func() { + s := server + defer func(s *Server) { if defErr != nil { - if err := server.Stop(); err != nil { + if err := s.Stop(); err != nil { logrus.Errorf("Error stopping 9p server: %v", err) } } - }() + }(s) - serverDir := m.Path + serverDir := m.Path + addr := m.Listener.Addr().String() - go func() { - if err := server.WaitForError(); err != nil { - logrus.Errorf("Error from 9p server on %s for %s: %v", m.Listener.Addr().String(), serverDir, err) + go func(s *Server, address, dir string) { + if err := s.WaitForError(); err != nil { + logrus.Errorf("Error from 9p server on %s for %s: %v", address, dir, err) } else { // We do not expect server exits - this should run until the program exits. - logrus.Warnf("9p server on %s for %s exited without error", m.Listener.Addr().String(), serverDir) + logrus.Warnf("9p server on %s for %s exited without error", address, dir) } - }() + }(s, addr, serverDir) } - return servers9p, nil + return servers, nil }pkg/fileserver/fs9p/server.go (1)
42-51: Buffer ErrChan and treat net.ErrClosed as clean shutdown to avoid leaks/noiseUnbuffered ErrChan can block the server goroutine if nobody reads; also treat listener closure as non-error in WaitForError.
-import ( +import ( + "errors" "fmt" "net" "os" "path/filepath" @@ - errChan := make(chan error) + errChan := make(chan error, 1) @@ -func (s *Server) WaitForError() error { - err := <-s.ErrChan - return err -} +func (s *Server) WaitForError() error { + if err, ok := <-s.ErrChan; ok { + if errors.Is(err, net.ErrClosed) { + return nil + } + return err + } + return nil +}Also applies to: 83-87
vendor/github.com/DeedleFake/p9/proto/proto.go (1)
85-93: Off‑by‑4 framing: bound LimitedReader to size‑4 and guard undersized frames.9P size includes the size field; using N=size leaks 4 bytes into the next frame on boundaries.
Apply this diff:
if (msize > 0) && (size > msize) { return nil, NoTag, util.Errorf("receive: %w", ErrLargeMessage) } - lr := &util.LimitedReader{ - R: r, - N: size, - E: ErrLargeMessage, - } + if size < 4 { + return nil, NoTag, util.Errorf("receive: invalid frame size: %d", size) + } + rem := size - 4 + if rem < 3 { // need at least 1 byte type + 2 bytes tag + return nil, NoTag, util.Errorf("receive: frame too small for header: %d", size) + } + lr := &util.LimitedReader{ + R: r, + N: rem, + E: ErrLargeMessage, + }vendor/github.com/DeedleFake/p9/remote.go (4)
37-43: Build Twalk Wname correctly for absolute paths (no empty elements).Leading “/” currently yields an empty first element or “/” entry, which is invalid for Twalk.
- w := []string{path.Clean(p)} - if w[0] != "/" { - w = strings.Split(w[0], "/") - } - if (len(w) == 1) && (w[0] == ".") { - w = nil - } + p = path.Clean(p) + var w []string + switch { + case p == "" || p == ".": + w = nil + default: + if strings.HasPrefix(p, "/") { + p = strings.TrimPrefix(p, "/") + } + if p != "" { + w = strings.Split(p, "/") + } else { + w = nil + } + }
184-185: Don’t panic on invalid whence; return error.- panic(util.Errorf("Invalid whence: %v", whence)) + return int64(file.pos), util.Errorf("invalid whence: %v", whence)
198-201: Guard maxBufSize underflow; avoid infinite loops on small msize.int(msize-IOHeaderSize) can underflow or be 0, causing ReadAt/WriteAt to loop forever.
-func (file *Remote) maxBufSize() int { - return int(file.client.Msize() - IOHeaderSize) -} +func (file *Remote) maxBufSize() int { + m := file.client.Msize() + if m <= uint32(IOHeaderSize) { + // Minimum 1 to ensure forward progress; handshake should reject too-small msize. + return 1 + } + return int(m - uint32(IOHeaderSize)) +}Additionally, validate negotiated msize during handshake (in vendor/github.com/DeedleFake/p9/client.go) and fail if msize <= IOHeaderSize. Want a patch?
230-239: Prevent zero/negative chunk size in ReadAt.If maxBufSize ≤ 0, the loop never advances.
- size := len(buf) - if size > file.maxBufSize() { - size = file.maxBufSize() - } + size := len(buf) + max := file.maxBufSize() + if max <= 0 { + return 0, util.Errorf("msize too small: %d", file.client.Msize()) + } + if size > max { + size = max + }
🧹 Nitpick comments (10)
vendor/github.com/DeedleFake/p9/dir_darwin.go (2)
23-33: Preserve identity when name lookup fails by falling back to numeric UID/GIDAvoid empty UID/GID fields; default to the numeric IDs and override on successful lookup.
-var uname string -uid, err := user.LookupId(strconv.FormatUint(uint64(sys.Uid), 10)) -if err == nil { - uname = uid.Username -} +uname := strconv.FormatUint(uint64(sys.Uid), 10) +if u, err := user.LookupId(uname); err == nil && u != nil && u.Username != "" { + uname = u.Username +} -var gname string -gid, err := user.LookupGroupId(strconv.FormatUint(uint64(sys.Gid), 10)) -if err == nil { - gname = gid.Name -} +gname := strconv.FormatUint(uint64(sys.Gid), 10) +if g, err := user.LookupGroupId(gname); err == nil && g != nil && g.Name != "" { + gname = g.Name +}
52-55: Improve diagnostics when FileInfo.Sys() isn’t Stat_tIncluding the path and actual type eases debugging. Optional, but helpful.
-import ( - "errors" +import ( + "fmt" "os" "os/user" "strconv" "syscall" "time" ) @@ - sys, _ := fi.Sys().(*syscall.Stat_t) - if sys == nil { - return QID{}, errors.New("failed to get QID: FileInfo was not Stat_t") - } + sys, _ := fi.Sys().(*syscall.Stat_t) + if sys == nil { + return QID{}, fmt.Errorf("failed to get QID for %q: FileInfo.Sys()=%T; want *syscall.Stat_t", d.path(p), fi.Sys()) + }vendor/github.com/DeedleFake/p9/internal/debug/debug.go (1)
1-1: Add go:build tag and newline in debug outputInclude the modern build tag for Go 1.17+ and add a newline to avoid run‑on log lines.
-// +build p9debug +//go:build p9debug +// +build p9debug @@ func Log(str string, args ...interface{}) { - fmt.Fprintf(os.Stderr, str, args...) + _, _ = fmt.Fprintf(os.Stderr, str+"\n", args...) }Also applies to: 10-12
vendor/github.com/DeedleFake/p9/README.md (1)
1-52: Markdown lint noise from vendored docs: exclude vendor/Static analysis flags heading style/tabs here. Prefer excluding vendor/** from markdownlint rather than editing upstream docs.
pkg/crc/config/settings.go (1)
87-88: Help text: don’t claim mountpoint ‘/’ across OSesThe mount location differs by platform. Make the message neutral.
- cfg.AddSetting(EnableSharedDirs, true, ValidateBool, SuccessfullyApplied, - "Mounts host's home directory at '/' in the CRC VM (true/false, default: true)") + cfg.AddSetting(EnableSharedDirs, true, ValidateBool, SuccessfullyApplied, + "Enable sharing of the host home directory into the CRC VM (true/false, default: true)")pkg/fileserver/fs9p/shares_fallback.go (1)
5-13: Return a sentinel error instead of a formatted stringMakes detection easier for callers without string matching.
-import ( - "fmt" - "runtime" -) +import ( + "errors" + "runtime" +) + +var ErrHvsockUnsupported = errors.New("fs9p: StartHvsockShares is only supported on windows") @@ func StartHvsockShares(mounts []HvsockMount9p) ([]*Server, error) { - return nil, fmt.Errorf("StartHvsockShares() not implemented on %s", runtime.GOOS) + _ = runtime.GOOS // keep message parity with windows-only implementation if needed elsewhere + return nil, ErrHvsockUnsupported }pkg/crc/machine/start.go (2)
237-243: SELinux context: virtiofs uses container_file_t; 9P may need labeling or doc9P often doesn’t support SELinux contexts like virtiofs. If containers need access, ensure appropriate labeling/workaround is applied or document the limitation.
320-323: Leftover SMB creds in machine config?SharedDirPassword/Username are still passed into MachineConfig though SMB is removed. If unused, drop them to avoid confusion.
pkg/fileserver/fs9p/server.go (1)
61-69: Avoid flagging immediate net.ErrClosed as startup errorIf the listener gets closed quickly, Start() will surface net.ErrClosed as a startup error. Consider ignoring net.ErrClosed here as well to reduce false alarms.
vendor/github.com/DeedleFake/p9/proto/proto.go (1)
101-108: Remove dead check and return a clear error for unknown msg type.- t := p.TypeFromID(msgType) - if t == nil { - if err != nil { - return nil, NoTag, err - } - - return nil, NoTag, util.Errorf("receive: invalid message type: %v", msgType) - } + t := p.TypeFromID(msgType) + if t == nil { + return nil, NoTag, util.Errorf("receive: invalid message type: %v", msgType) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (44)
cmd/crc/cmd/daemon.go(5 hunks)cmd/crc/cmd/start.go(0 hunks)go.mod(2 hunks)packaging/windows/product.wxs.template(2 hunks)pkg/crc/config/settings.go(1 hunks)pkg/crc/constants/constants.go(1 hunks)pkg/crc/machine/driver.go(0 hunks)pkg/crc/machine/driver_darwin.go(0 hunks)pkg/crc/machine/driver_linux.go(0 hunks)pkg/crc/machine/driver_windows.go(0 hunks)pkg/crc/machine/libhvee/driver_windows.go(2 hunks)pkg/crc/machine/start.go(2 hunks)pkg/drivers/libhvee/libhvee_windows.go(0 hunks)pkg/drivers/libhvee/powershell_windows.go(0 hunks)pkg/fileserver/fs9p/server.go(1 hunks)pkg/fileserver/fs9p/shares.go(1 hunks)pkg/fileserver/fs9p/shares_fallback.go(1 hunks)pkg/fileserver/fs9p/shares_windows.go(1 hunks)vendor/github.com/DeedleFake/p9/LICENSE(1 hunks)vendor/github.com/DeedleFake/p9/README.md(1 hunks)vendor/github.com/DeedleFake/p9/addr.go(1 hunks)vendor/github.com/DeedleFake/p9/addr_other.go(1 hunks)vendor/github.com/DeedleFake/p9/addr_unix.go(1 hunks)vendor/github.com/DeedleFake/p9/client.go(1 hunks)vendor/github.com/DeedleFake/p9/dir.go(1 hunks)vendor/github.com/DeedleFake/p9/dir_darwin.go(1 hunks)vendor/github.com/DeedleFake/p9/dir_linux.go(1 hunks)vendor/github.com/DeedleFake/p9/dir_other.go(1 hunks)vendor/github.com/DeedleFake/p9/dir_plan9.go(1 hunks)vendor/github.com/DeedleFake/p9/dir_windows.go(1 hunks)vendor/github.com/DeedleFake/p9/doc.go(1 hunks)vendor/github.com/DeedleFake/p9/encoding.go(1 hunks)vendor/github.com/DeedleFake/p9/fs.go(1 hunks)vendor/github.com/DeedleFake/p9/internal/debug/debug.go(1 hunks)vendor/github.com/DeedleFake/p9/internal/debug/nodebug.go(1 hunks)vendor/github.com/DeedleFake/p9/internal/util/util.go(1 hunks)vendor/github.com/DeedleFake/p9/msg.go(1 hunks)vendor/github.com/DeedleFake/p9/p9.go(1 hunks)vendor/github.com/DeedleFake/p9/proto/client.go(1 hunks)vendor/github.com/DeedleFake/p9/proto/encoding.go(1 hunks)vendor/github.com/DeedleFake/p9/proto/proto.go(1 hunks)vendor/github.com/DeedleFake/p9/proto/server.go(1 hunks)vendor/github.com/DeedleFake/p9/remote.go(1 hunks)vendor/github.com/DeedleFake/p9/stat.go(1 hunks)
💤 Files with no reviewable changes (7)
- pkg/crc/machine/driver_linux.go
- pkg/crc/machine/driver_darwin.go
- pkg/crc/machine/driver_windows.go
- cmd/crc/cmd/start.go
- pkg/drivers/libhvee/libhvee_windows.go
- pkg/drivers/libhvee/powershell_windows.go
- pkg/crc/machine/driver.go
✅ Files skipped from review due to trivial changes (1)
- vendor/github.com/DeedleFake/p9/doc.go
🚧 Files skipped from review as they are similar to previous changes (20)
- vendor/github.com/DeedleFake/p9/LICENSE
- vendor/github.com/DeedleFake/p9/internal/debug/nodebug.go
- vendor/github.com/DeedleFake/p9/addr_unix.go
- vendor/github.com/DeedleFake/p9/proto/encoding.go
- vendor/github.com/DeedleFake/p9/addr.go
- vendor/github.com/DeedleFake/p9/proto/server.go
- vendor/github.com/DeedleFake/p9/dir_other.go
- vendor/github.com/DeedleFake/p9/encoding.go
- vendor/github.com/DeedleFake/p9/fs.go
- vendor/github.com/DeedleFake/p9/stat.go
- vendor/github.com/DeedleFake/p9/dir_plan9.go
- vendor/github.com/DeedleFake/p9/dir_windows.go
- vendor/github.com/DeedleFake/p9/addr_other.go
- vendor/github.com/DeedleFake/p9/dir.go
- go.mod
- vendor/github.com/DeedleFake/p9/dir_linux.go
- vendor/github.com/DeedleFake/p9/internal/util/util.go
- vendor/github.com/DeedleFake/p9/msg.go
- vendor/github.com/DeedleFake/p9/client.go
- vendor/github.com/DeedleFake/p9/p9.go
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: redbeam
PR: crc-org/crc#4866
File: pkg/fileserver/fs9p/shares.go:0-0
Timestamp: 2025-08-06T09:48:10.441Z
Learning: The vsock implementation in pkg/fileserver/fs9p/shares.go was temporarily removed because 9pfs doesn't fully support it yet. It will be added back when 9pfs provides proper vsock support.
📚 Learning: 2025-08-06T09:48:10.441Z
Learnt from: redbeam
PR: crc-org/crc#4866
File: pkg/fileserver/fs9p/shares.go:0-0
Timestamp: 2025-08-06T09:48:10.441Z
Learning: The vsock implementation in pkg/fileserver/fs9p/shares.go was temporarily removed because 9pfs doesn't fully support it yet. It will be added back when 9pfs provides proper vsock support.
Applied to files:
pkg/fileserver/fs9p/shares_fallback.gopkg/fileserver/fs9p/shares_windows.gocmd/crc/cmd/daemon.gopkg/fileserver/fs9p/shares.gopkg/fileserver/fs9p/server.govendor/github.com/DeedleFake/p9/README.md
🧬 Code graph analysis (13)
pkg/fileserver/fs9p/shares_fallback.go (3)
pkg/fileserver/fs9p/shares_windows.go (1)
StartHvsockShares(12-33)pkg/fileserver/fs9p/shares.go (1)
HvsockMount9p(19-22)pkg/fileserver/fs9p/server.go (1)
Server(15-24)
pkg/fileserver/fs9p/shares_windows.go (3)
pkg/fileserver/fs9p/shares_fallback.go (1)
StartHvsockShares(11-13)pkg/fileserver/fs9p/shares.go (3)
HvsockMount9p(19-22)Mount9p(12-15)StartShares(25-61)pkg/fileserver/fs9p/server.go (1)
Server(15-24)
vendor/github.com/DeedleFake/p9/internal/debug/debug.go (1)
vendor/github.com/DeedleFake/p9/internal/debug/nodebug.go (1)
Log(5-5)
vendor/github.com/DeedleFake/p9/dir_darwin.go (3)
vendor/github.com/DeedleFake/p9/stat.go (4)
DirEntry(255-267)FileMode(23-23)ModeFromOS(43-78)Stat(156-168)vendor/github.com/DeedleFake/p9/dir.go (1)
Dir(16-16)vendor/github.com/DeedleFake/p9/p9.go (3)
QID(42-46)QIDType(49-49)Version(10-10)
pkg/crc/machine/libhvee/driver_windows.go (1)
vendor/github.com/crc-org/machine/libmachine/drivers/base.go (1)
SharedDir(27-35)
vendor/github.com/DeedleFake/p9/proto/proto.go (4)
vendor/github.com/DeedleFake/p9/msg.go (1)
Proto(75-77)vendor/github.com/DeedleFake/p9/proto/encoding.go (3)
Read(34-38)Write(25-31)Size(14-20)vendor/github.com/DeedleFake/p9/p9.go (1)
NoTag(15-15)vendor/github.com/DeedleFake/p9/internal/util/util.go (2)
Errorf(44-52)LimitedReader(13-17)
cmd/crc/cmd/daemon.go (5)
pkg/fileserver/fs9p/shares_fallback.go (1)
StartHvsockShares(11-13)pkg/fileserver/fs9p/shares_windows.go (1)
StartHvsockShares(12-33)pkg/fileserver/fs9p/shares.go (3)
HvsockMount9p(19-22)StartShares(25-61)Mount9p(12-15)pkg/crc/constants/constants.go (3)
GetHomeDir(166-172)Plan9HvsockGUID(61-61)Plan9TcpPort(60-60)pkg/crc/logging/logging.go (1)
Warnf(100-102)
pkg/crc/config/settings.go (2)
pkg/crc/config/validations.go (1)
ValidateBool(19-25)pkg/crc/config/callbacks.go (1)
SuccessfullyApplied(36-38)
vendor/github.com/DeedleFake/p9/remote.go (7)
vendor/github.com/DeedleFake/p9/client.go (1)
Client(22-26)vendor/github.com/DeedleFake/p9/proto/client.go (1)
Client(18-31)vendor/github.com/DeedleFake/p9/p9.go (4)
QID(42-46)QIDType(49-49)Version(10-10)IOHeaderSize(70-70)vendor/github.com/DeedleFake/p9/msg.go (13)
Twalk(133-137)Rwalk(139-141)Topen(143-146)Ropen(148-151)Tcreate(153-158)Rcreate(160-163)Tread(165-169)Rread(171-173)Twrite(175-179)Rwrite(181-183)Tclunk(185-187)Tstat(199-201)Rstat(203-205)vendor/github.com/DeedleFake/p9/stat.go (3)
FileMode(23-23)Stat(156-168)DirEntry(255-267)vendor/github.com/DeedleFake/p9/internal/util/util.go (1)
Errorf(44-52)vendor/github.com/DeedleFake/p9/encoding.go (1)
ReadDir(16-30)
pkg/crc/machine/start.go (3)
pkg/os/exec.go (1)
RunPrivileged(48-59)pkg/crc/constants/constants.go (1)
Plan9HvsockPort(62-62)pkg/crc/logging/logging.go (1)
Warnf(100-102)
pkg/fileserver/fs9p/shares.go (1)
pkg/fileserver/fs9p/server.go (2)
Server(15-24)New9pServer(29-52)
vendor/github.com/DeedleFake/p9/proto/client.go (5)
vendor/github.com/DeedleFake/p9/msg.go (1)
Proto(75-77)vendor/github.com/DeedleFake/p9/proto/proto.go (3)
Proto(38-41)NoTag(27-27)ErrClientClosed(22-22)vendor/github.com/DeedleFake/p9/client.go (3)
NewClient(30-32)Client(22-26)Dial(36-43)vendor/github.com/DeedleFake/p9/internal/debug/debug.go (1)
Log(10-12)vendor/github.com/DeedleFake/p9/internal/debug/nodebug.go (1)
Log(5-5)
pkg/fileserver/fs9p/server.go (3)
vendor/github.com/DeedleFake/p9/fs.go (2)
FileSystem(21-34)FSConnHandler(162-167)vendor/github.com/DeedleFake/p9/proto/server.go (1)
Serve(17-42)pkg/crc/constants/constants.go (1)
Plan9Msize(59-59)
🪛 markdownlint-cli2 (0.17.2)
vendor/github.com/DeedleFake/p9/README.md
12-12: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
17-17: Hard tabs
Column: 1
(MD010, no-hard-tabs)
21-21: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
26-26: Hard tabs
Column: 1
(MD010, no-hard-tabs)
32-32: Hard tabs
Column: 1
(MD010, no-hard-tabs)
37-37: Hard tabs
Column: 1
(MD010, no-hard-tabs)
43-43: Hard tabs
Column: 1
(MD010, no-hard-tabs)
49-49: Hard tabs
Column: 1
(MD010, no-hard-tabs)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (17)
- GitHub Check: build (windows-2022, 1.24)
- GitHub Check: build (macOS-14, 1.24)
- GitHub Check: build (ubuntu-22.04, 1.24)
- GitHub Check: build (macOS-14, 1.24)
- GitHub Check: build-qe (linux, arm64)
- GitHub Check: build (windows-2022, 1.24)
- GitHub Check: build-qe (windows, amd64)
- GitHub Check: build (ubuntu-latest, 1.24)
- GitHub Check: build (macOS-13, 1.24)
- GitHub Check: build-qe (darwin, amd64)
- GitHub Check: build-qe (linux, amd64)
- GitHub Check: build-qe (darwin, arm64)
- GitHub Check: build-installer (windows-2022, 1.24)
- GitHub Check: build (ubuntu-latest, 1.24)
- GitHub Check: Run OKD bundle with crc (1.24)
- GitHub Check: verify-devcontainer
- GitHub Check: Konflux kflux-prd-rh02 / crc-binary-on-pull-request
🔇 Additional comments (7)
pkg/crc/machine/libhvee/driver_windows.go (1)
41-45: LGTM: switch to 9P and drop SMB credsUsing Type="9p" with no Username/Password on Windows is correct for the new path.
pkg/crc/constants/constants.go (1)
59-62: LGTM: 9P constants addedValues look sane for 9P defaults and hvsock integration.
cmd/crc/cmd/daemon.go (1)
182-186: LGTM: net.JoinHostPortSafer IPv6‑aware address construction.
Also applies to: 198-202
pkg/crc/config/settings.go (1)
33-35: Do not remove SharedDirPassword — it's referenced in the code.
SharedDirPassword is referenced in pkg/crc/machine/start.go:321, pkg/crc/machine/types/types.go:37, and pkg/crc/machine/config/config.go:24; remove only after updating/removing those usages.Likely an incorrect or invalid review comment.
vendor/github.com/DeedleFake/p9/proto/client.go (1)
115-146: Skip NoTag (0xFFFF) when allocating tags to prevent protocol collisionWhen nextTag wraps it can equal NoTag; advance past NoTag before returning the next tag.
- var nextTag uint16 + var nextTag uint16 tags := make(map[uint16]chan interface{}) @@ - case c.nextTag <- nextTag: - for { - nextTag++ - if _, ok := tags[nextTag]; !ok { - break - } - } + case c.nextTag <- nextTag: + for { + nextTag++ + if nextTag == NoTag { + nextTag++ + } + if _, ok := tags[nextTag]; !ok { + break + } + }Given this is vendored, send the fix upstream so it persists across updates.
packaging/windows/product.wxs.template (2)
111-112: LGTM: Feature references include the new HVSOCK 9P component.
69-73: Resolved — GUIDs are alignedpkg/crc/constants/constants.go defines Plan9HvsockGUID = "00009000-FACB-11E6-BD58-64006A7986D3"; the same value is present in packaging/windows/product.wxs.template and used via constants.Plan9HvsockGUID in cmd/crc/cmd/daemon.go. No action required.
| <Component Id="Hvsock9pRegistryEntry" Guid="*"> | ||
| <RegistryKey Root="HKLM" Key="Software\Microsoft\Windows NT\CurrentVersion\Virtualization\GuestCommunicationServices\00009000-FACB-11E6-BD58-64006A7986D3"> | ||
| <RegistryValue Type="string" Name="ElementName" Value="fs9p-hvsock" KeyPath="yes"/> | ||
| </RegistryKey> | ||
| </Component> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Write 9P HVSOCK registry to 64‑bit hive (avoid WOW6432 redirection).
Without marking the component as 64‑bit, the key may land under Wow6432Node and not be honored by Guest Communication Services.
Apply this diff:
- <Component Id="Hvsock9pRegistryEntry" Guid="*">
+ <Component Id="Hvsock9pRegistryEntry" Guid="*" Win64="yes">
<RegistryKey Root="HKLM" Key="Software\Microsoft\Windows NT\CurrentVersion\Virtualization\GuestCommunicationServices\00009000-FACB-11E6-BD58-64006A7986D3">
<RegistryValue Type="string" Name="ElementName" Value="fs9p-hvsock" KeyPath="yes"/>
</RegistryKey>
</Component>Optional (recommended): also mark VsockRegistryEntry as Win64 for consistency:
- <Component Id="VsockRegistryEntry" Guid="*">
+ <Component Id="VsockRegistryEntry" Guid="*" Win64="yes">🤖 Prompt for AI Agents
In packaging/windows/product.wxs.template around lines 69 to 73, the
Hvsock9pRegistryEntry component currently writes to the registry without
specifying 64-bit, which can cause the key to be redirected under Wow6432Node;
update the Component element to include Win64="yes" so the registry key is
written to the 64-bit hive, and for consistency also set Win64="yes" on the
VsockRegistryEntry component.
There was a problem hiding this comment.
@anjannath do you think this is necessary? especially the "VsockRegistryEntry" one
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| size := len(data) | ||
| if size > file.maxBufSize() { | ||
| size = file.maxBufSize() | ||
| } | ||
|
|
There was a problem hiding this comment.
Mirror the chunk-size guard in WriteAt.
- size := len(data)
- if size > file.maxBufSize() {
- size = file.maxBufSize()
- }
+ size := len(data)
+ max := file.maxBufSize()
+ if max <= 0 {
+ return 0, util.Errorf("msize too small: %d", file.client.Msize())
+ }
+ if size > max {
+ size = max
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| size := len(data) | |
| if size > file.maxBufSize() { | |
| size = file.maxBufSize() | |
| } | |
| size := len(data) | |
| max := file.maxBufSize() | |
| if max <= 0 { | |
| return 0, util.Errorf("msize too small: %d", file.client.Msize()) | |
| } | |
| if size > max { | |
| size = max | |
| } |
🤖 Prompt for AI Agents
In vendor/github.com/DeedleFake/p9/remote.go around lines 289 to 293, the Read
chunk-size handling only sets size := len(data) and caps it to
file.maxBufSize(), but it needs to mirror the exact guard logic used in WriteAt:
compute the chunk size the same way WriteAt does (respecting maxBufSize and any
remaining-file or offset boundaries), and replace the current simple cap with
that same calculation so reads use identical chunk limits as writes.
redbeam
force-pushed
the
remove_smb
branch
2 times, most recently
from
5959d98 to
f06fba0
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (17)
pkg/crc/machine/libhvee/driver_windows.go (1)
39-45: Past review concerns remain unaddressed: tag determinism and centralization.The previous review's concerns about tag/name symmetry, deterministic ordering, and centralizing the tag format are still applicable. The tag generation using
fmt.Sprintf("dir%d", i)remains:
- Non-deterministic: Tag assignment depends on iteration order; reordering
SharedDirswill change tags and break existing mounts.- Not centralized: The "dir%d" format is hardcoded here and must match the 9P server's share naming. If they diverge, mounts will fail.
The removal of the
Usernamefield is correct for 9p (which doesn't require Windows credentials), and theType: "9p"change aligns with the PR's migration from SMB to 9p.To verify tag format consistency with the 9P server, run:
#!/bin/bash # Description: Find where 9P share names/tags are defined on the server side. # Expected: Server should use matching "dir%d" format or derive from same source. # Search for 9P share/tag/aname definitions in fileserver code rg -n -C3 --type=go 'Share.*Tag|aname|dir%d|Plan9.*Tag' pkg/fileserver/ # Search for any tag format constants or helpers rg -n -C3 --type=go 'ShareTag|Plan9Tag|dir%d' pkg/crc/constants/ pkg/fileserver/vendor/github.com/DeedleFake/p9/addr_other.go (1)
23-28: Critical Windows path issue requires upstream fix.As flagged in the previous review, using DISPLAY values containing ":" (like the default ":0") creates invalid Windows paths, causing runtime failures. This is a critical correctness issue for Windows deployments.
Per the PR discussion, this vendored code issue should be fixed upstream. Please file a PR to the DeedleFake/p9 repository with the sanitization fix suggested in the previous comment (adding runtime.GOOS check and replacing ":" with "_" on Windows).
Based on PR discussion guidance.
cmd/crc/cmd/daemon.go (1)
253-280: Missing graceful shutdown for 9P servers.Both
server9pHvsock(line 260-264) andserver9pTCP(line 273-277) are started but their handles are not retained for cleanup. When the daemon shuts down, these servers will leak goroutines and listeners.Capture the server instances and defer their cleanup to ensure graceful shutdown.
For example:
// 9p home directory sharing if runtime.GOOS == "windows" { // 9p over hvsock listener9pHvsock, err := fs9p.GetHvsockListener(constants.Plan9HvsockGUID) if err != nil { return err } server9pHvsock, err := fs9p.New9pServer(listener9pHvsock, constants.GetHomeDir()) if err != nil { return err } if err := server9pHvsock.Start(); err != nil { return err } + defer func() { + if err := server9pHvsock.Stop(); err != nil { + logging.Errorf("Error stopping hvsock 9p server: %v", err) + } + }() // 9p over TCP (as a backup) listener9pTCP, err := vn.Listen("tcp", net.JoinHostPort(configuration.GatewayIP, fmt.Sprintf("%d", constants.Plan9TcpPort))) if err != nil { return err } server9pTCP, err := fs9p.New9pServer(listener9pTCP, constants.GetHomeDir()) if err != nil { return err } if err := server9pTCP.Start(); err != nil { return err } + defer func() { + if err := server9pTCP.Stop(); err != nil { + logging.Errorf("Error stopping TCP 9p server: %v", err) + } + }() }Note: This issue was previously flagged in an earlier review.
pkg/crc/machine/start.go (1)
244-256: Make 9P mount idempotent and clarify privilege/options (FUSE vs kernel 9p)
- Add a pre-check to skip if already mounted.
- If this uses a kernel 9p mount, run it privileged and pass
aname,msize,version,cache. If it’s a FUSE client, keep unprivileged but ensure tag/export is specified and considermsize.Example (kernel mount path):
case "9p": + // Skip if already mounted + if _, _, err := sshRunner.RunPrivileged("Check mountpoint", "mountpoint", "-q", mount.Target); err == nil { + logging.Debugf("Already mounted: %s", mount.Target) + continue + } // change owner to core user to allow mounting to it as a non-root user if _, _, err := sshRunner.RunPrivileged("Changing owner of mount directory", "chown", "core:core", mount.Target); err != nil { return err } - if _, _, err := sshRunner.Run("9pfs", "-V", "-p", fmt.Sprintf("%d", constants.Plan9HvsockPort), mount.Target); err != nil { + // Prefer explicit 9p kernel mount with options; if you intend to use FUSE 9pfs, please confirm and keep it. + opts := fmt.Sprintf("trans=tcp,port=%d,msize=%d,cache=loose,version=9p2000.L,aname=%s", constants.Plan9TcpPort, constants.Plan9Msize, mount.Tag) + if _, _, err := sshRunner.RunPrivileged( + fmt.Sprintf("Mounting %s (9p tag %s)", mount.Target, mount.Tag), + "mount", "-t", "9p", "-o", opts, constants.VSockGateway, mount.Target, + ); err != nil { logging.Warnf("Failed to connect to 9p server over hvsock: %v", err) logging.Warnf("Falling back to 9p over TCP") - if _, _, err := sshRunner.Run("9pfs", "192.168.127.1", mount.Target); err != nil { + if _, _, err := sshRunner.RunPrivileged("Mount 9p over TCP", "mount", "-t", "9p", "-o", opts, constants.VSockGateway, mount.Target); err != nil { return err } }If you intend to use a FUSE-based
9pfsclient, please confirm its CLI for specifying the export/tag and msize, and we’ll adjust accordingly instead of usingmount -t 9p.Run to find other combined-token calls:
#!/bin/bash rg -nP -C2 'Run(?:Privileged)?\([^,]+,\s*"[^",\s]+ [^"]+"' --type govendor/github.com/DeedleFake/p9/dir_linux.go (1)
35-43: Fix ATime construction: syscall.Timespec lacks Unix() (compile error)Use Sec/Nsec fields of
Stat_t.Atim.- ATime: time.Unix(sys.Atim.Unix()), + ATime: time.Unix(int64(sys.Atim.Sec), int64(sys.Atim.Nsec)),pkg/fileserver/fs9p/server.go (2)
71-80: Stop() is a hard close; consider graceful shutdown or document clearlyEither track connections and drain with a timeout, or document at call sites that Stop forcibly interrupts active I/O and callers should ignore net.ErrClosed from WaitForError.
3-13: Avoid goroutine leak; treat net.ErrClosed as clean shutdownMake ErrChan buffered and ignore net.ErrClosed in WaitForError. Adds errors import.
@@ -import ( +import ( + "errors" "fmt" "net" "os" "path/filepath" @@ - errChan := make(chan error) + errChan := make(chan error, 1) @@ -func (s *Server) WaitForError() error { - err := <-s.ErrChan - return err -} +func (s *Server) WaitForError() error { + if err, ok := <-s.ErrChan; ok { + if errors.Is(err, net.ErrClosed) { + return nil + } + return err + } + return nil +}Also applies to: 43-51, 83-87
vendor/github.com/DeedleFake/p9/dir_darwin.go (1)
35-43: Fix Darwin build: ATime construction uses non-existent Timespec.Unix()Use time.Unix(sec, nsec) with Timespec fields. Current code won’t compile on darwin.
return DirEntry{ FileMode: ModeFromOS(fi.Mode()), - ATime: time.Unix(sys.Atimespec.Unix()), + ATime: time.Unix(int64(sys.Atimespec.Sec), int64(sys.Atimespec.Nsec)), MTime: fi.ModTime(), Length: uint64(fi.Size()), EntryName: fi.Name(), UID: uname, GID: gname, }vendor/github.com/DeedleFake/p9/client.go (1)
3-9: Guard response type assertions to avoid panics (Handshake/Auth/Attach)Unchecked rsp.(*Type) will panic on protocol errors. Guard and return errors; add fmt import.
@@ -import ( +import ( "errors" + "fmt" "net" "sync/atomic" @@ - version := rsp.(*Rversion) + version, ok := rsp.(*Rversion) + if !ok { + return 0, fmt.Errorf("handshake: expected Rversion, got %T", rsp) + } @@ - rauth := rsp.(*Rauth) + rauth, ok := rsp.(*Rauth) + if !ok { + return nil, fmt.Errorf("auth: expected Rauth, got %T", rsp) + } @@ - attach := rsp.(*Rattach) + attach, ok := rsp.(*Rattach) + if !ok { + return nil, fmt.Errorf("attach: expected Rattach, got %T", rsp) + }Also applies to: 61-69, 84-91, 113-120
vendor/github.com/DeedleFake/p9/dir.go (2)
52-60: Avoid clobbering timestamps on partial ATime/MTime updatesCalling Chtimes with only one set overwrites the other with zero. Reject partial updates.
- atime, ok1 := changes.ATime() - mtime, ok2 := changes.MTime() - if ok1 || ok2 { - err := os.Chtimes(p, atime, mtime) + atime, ok1 := changes.ATime() + mtime, ok2 := changes.MTime() + if ok1 != ok2 { + return errors.New("setting only one of ATime or MTime is not supported") + } + if ok1 && ok2 { + err := os.Chtimes(p, atime, mtime) if err != nil { return err } }
69-75: Prevent path traversal on rename (validate name is a base name)New name must not contain separators or traversal; keep rename within directory.
@@ -import ( +import ( "errors" "os" "path/filepath" + "strings" ) @@ - name, ok := changes.Name() + name, ok := changes.Name() if ok { - err := os.Rename(p, filepath.Join(base, filepath.FromSlash(name))) + // disallow separators/traversal; only a base name is allowed + if name != filepath.Base(name) || strings.ContainsRune(name, os.PathSeparator) { + return errors.New("invalid name: must be a base name without path separators") + } + err := os.Rename(p, filepath.Join(base, name)) if err != nil { return err } }Also applies to: 3-7
vendor/github.com/DeedleFake/p9/proto/server.go (1)
3-9: Don’t dispatch after Receive errors; exit on closed connectionsBreak/continue appropriately; avoid using stale msg/tag and busy-looping on closed sockets. Add errors import.
@@ -import ( +import ( + "errors" "io" "log" "net" "sync" ) @@ - tmsg, tag, err := p.Receive(c, msize) + tmsg, tag, err := p.Receive(c, msize) if err != nil { - if err == io.EOF { - return - } - - log.Printf("Error reading message: %v", err) + // Terminate on EOF or closed connection; otherwise log and continue. + if err == io.EOF || errors.Is(err, net.ErrClosed) { + return + } + log.Printf("Error reading message: %v", err) + continue }Also applies to: 69-78
packaging/windows/product.wxs.template (2)
64-68: Do the same for gvisor‑tap‑vsock for consistency.Prevent potential redirection for the existing vsock key as well.
- <Component Id="VsockRegistryEntry" Guid="*"> + <Component Id="VsockRegistryEntry" Guid="*" Win64="yes">
69-73: Write 9P HVSOCK registry to 64‑bit hive (avoid Wow6432 redirection).Mark component as 64‑bit so the key lands under the native HKLM hive.
- <Component Id="Hvsock9pRegistryEntry" Guid="*"> + <Component Id="Hvsock9pRegistryEntry" Guid="*" Win64="yes">vendor/github.com/DeedleFake/p9/remote.go (3)
37-43: Fix Wname building for absolute paths.Leading “/” yields empty element; “/” yields invalid path. Clean, trim leading slash, split, drop empties.
- w := []string{path.Clean(p)} - if w[0] != "/" { - w = strings.Split(w[0], "/") - } - if (len(w) == 1) && (w[0] == ".") { - w = nil - } + p = path.Clean(p) + var w []string + switch { + case p == "" || p == "." || p == "/": + w = nil + default: + w = strings.Split(strings.TrimPrefix(p, "/"), "/") + }
184-185: Don’t panic on invalid whence; return error.Library code should not abort the process.
- panic(util.Errorf("Invalid whence: %v", whence)) + return int64(file.pos), util.Errorf("invalid whence: %v", whence)
198-201: Avoid underflow/zero in maxBufSize; guard for tiny msize.If
Msize() <= IOHeaderSize, the subtraction underflows or returns 0; callers then loop forever.-func (file *Remote) maxBufSize() int { - return int(file.client.Msize() - IOHeaderSize) -} +func (file *Remote) maxBufSize() int { + m := file.client.Msize() + if m <= uint32(IOHeaderSize) { + return -1 + } + return int(m - uint32(IOHeaderSize)) +}
🧹 Nitpick comments (7)
vendor/github.com/DeedleFake/p9/internal/debug/nodebug.go (1)
1-1: Consider updating to modern build tag format (upstream).The old-style
// +build !p9debugconstraint is deprecated in favor of the//go:build !p9debugformat introduced in Go 1.17. Since this is vendored code, if you consider this worth addressing, you could file an upstream PR to modernize the build tags.Based on learnings (cfergeau's guidance on vendored code).
vendor/github.com/DeedleFake/p9/README.md (3)
4-5: Use HTTPS and pkg.go.dev for the docs badge/link (upstream change).Prefer pkg.go.dev over the legacy GoDoc URL and enforce HTTPS to avoid mixed-content/link-rot.
If we keep vendor pristine, consider an upstream PR. Locally, ensure link checkers ignore vendor paths.
15-19: Hard tabs inside fenced code blocks trigger MD010; decide policy.Either keep tabs (common in Go) and ignore vendor/** in markdownlint, or replace with spaces upstream. Avoid local edits to vendored files.
Also applies to: 24-51
12-12: Consistent heading style or vendor exclusion
The vendored README mixes ATX (### Server) with setext headings, triggering markdownlint’s style rules. Either update the upstream file to unify heading styles or addvendor/**to your.markdownlintignore(or CI exclude patterns) so vendored docs are skipped.pkg/crc/constants/constants.go (1)
59-62: Constants look good; consider documenting expected units and consumersAdd short comments for
Plan9Msize(bytes),Plan9TcpPort,Plan9HvsockGUID/Port, and ensure both client and server use the samemsize.vendor/github.com/DeedleFake/p9/stat.go (1)
187-193: Guard stat size overflow.
size()sums into uint16; very long names/IDs overflow silently. Validate before encode.func (s Stat) size() uint16 { - return uint16(47 + len(s.Name) + len(s.UID) + len(s.GID) + len(s.MUID)) + return uint16(47 + len(s.Name) + len(s.UID) + len(s.GID) + len(s.MUID)) } func (s Stat) P9Encode() (r []byte, err error) { + if int(47+len(s.Name)+len(s.UID)+len(s.GID)+len(s.MUID)) > 0xFFFF { + return nil, ErrLargeStat + }vendor/github.com/DeedleFake/p9/remote.go (1)
124-141: Minor: avoid shadowing the receiver in Remove().Using
file, err := file.walk(p)shadowsfile(receiver), hurting readability.- file, err := file.walk(p) + target, err := file.walk(p) if err != nil { return err } // Close is not necessary. Remove is also a clunk. - return file.Remove("") + return target.Remove("")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (44)
cmd/crc/cmd/daemon.go(5 hunks)cmd/crc/cmd/start.go(0 hunks)go.mod(1 hunks)packaging/windows/product.wxs.template(2 hunks)pkg/crc/config/settings.go(1 hunks)pkg/crc/constants/constants.go(1 hunks)pkg/crc/machine/driver.go(0 hunks)pkg/crc/machine/driver_darwin.go(0 hunks)pkg/crc/machine/driver_linux.go(0 hunks)pkg/crc/machine/driver_windows.go(0 hunks)pkg/crc/machine/libhvee/driver_windows.go(2 hunks)pkg/crc/machine/start.go(2 hunks)pkg/drivers/libhvee/libhvee_windows.go(0 hunks)pkg/drivers/libhvee/powershell_windows.go(0 hunks)pkg/fileserver/fs9p/server.go(1 hunks)pkg/fileserver/fs9p/server_fallback.go(1 hunks)pkg/fileserver/fs9p/server_windows.go(1 hunks)vendor/github.com/DeedleFake/p9/LICENSE(1 hunks)vendor/github.com/DeedleFake/p9/README.md(1 hunks)vendor/github.com/DeedleFake/p9/addr.go(1 hunks)vendor/github.com/DeedleFake/p9/addr_other.go(1 hunks)vendor/github.com/DeedleFake/p9/addr_unix.go(1 hunks)vendor/github.com/DeedleFake/p9/client.go(1 hunks)vendor/github.com/DeedleFake/p9/dir.go(1 hunks)vendor/github.com/DeedleFake/p9/dir_darwin.go(1 hunks)vendor/github.com/DeedleFake/p9/dir_linux.go(1 hunks)vendor/github.com/DeedleFake/p9/dir_other.go(1 hunks)vendor/github.com/DeedleFake/p9/dir_plan9.go(1 hunks)vendor/github.com/DeedleFake/p9/dir_windows.go(1 hunks)vendor/github.com/DeedleFake/p9/doc.go(1 hunks)vendor/github.com/DeedleFake/p9/encoding.go(1 hunks)vendor/github.com/DeedleFake/p9/fs.go(1 hunks)vendor/github.com/DeedleFake/p9/internal/debug/debug.go(1 hunks)vendor/github.com/DeedleFake/p9/internal/debug/nodebug.go(1 hunks)vendor/github.com/DeedleFake/p9/internal/util/util.go(1 hunks)vendor/github.com/DeedleFake/p9/msg.go(1 hunks)vendor/github.com/DeedleFake/p9/p9.go(1 hunks)vendor/github.com/DeedleFake/p9/proto/client.go(1 hunks)vendor/github.com/DeedleFake/p9/proto/encoding.go(1 hunks)vendor/github.com/DeedleFake/p9/proto/proto.go(1 hunks)vendor/github.com/DeedleFake/p9/proto/server.go(1 hunks)vendor/github.com/DeedleFake/p9/remote.go(1 hunks)vendor/github.com/DeedleFake/p9/stat.go(1 hunks)vendor/modules.txt(1 hunks)
💤 Files with no reviewable changes (7)
- pkg/drivers/libhvee/powershell_windows.go
- pkg/drivers/libhvee/libhvee_windows.go
- pkg/crc/machine/driver_darwin.go
- pkg/crc/machine/driver.go
- pkg/crc/machine/driver_windows.go
- cmd/crc/cmd/start.go
- pkg/crc/machine/driver_linux.go
✅ Files skipped from review due to trivial changes (1)
- vendor/github.com/DeedleFake/p9/doc.go
🚧 Files skipped from review as they are similar to previous changes (12)
- vendor/github.com/DeedleFake/p9/addr_unix.go
- vendor/github.com/DeedleFake/p9/LICENSE
- vendor/github.com/DeedleFake/p9/proto/proto.go
- vendor/github.com/DeedleFake/p9/proto/encoding.go
- go.mod
- vendor/github.com/DeedleFake/p9/dir_plan9.go
- pkg/crc/config/settings.go
- vendor/github.com/DeedleFake/p9/p9.go
- vendor/modules.txt
- vendor/github.com/DeedleFake/p9/msg.go
- vendor/github.com/DeedleFake/p9/internal/util/util.go
- vendor/github.com/DeedleFake/p9/internal/debug/debug.go
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: redbeam
PR: crc-org/crc#4866
File: pkg/fileserver/fs9p/shares.go:0-0
Timestamp: 2025-08-06T09:48:10.441Z
Learning: The vsock implementation in pkg/fileserver/fs9p/shares.go was temporarily removed because 9pfs doesn't fully support it yet. It will be added back when 9pfs provides proper vsock support.
📚 Learning: 2025-08-06T09:48:10.441Z
Learnt from: redbeam
PR: crc-org/crc#4866
File: pkg/fileserver/fs9p/shares.go:0-0
Timestamp: 2025-08-06T09:48:10.441Z
Learning: The vsock implementation in pkg/fileserver/fs9p/shares.go was temporarily removed because 9pfs doesn't fully support it yet. It will be added back when 9pfs provides proper vsock support.
Applied to files:
pkg/fileserver/fs9p/server_windows.gopkg/fileserver/fs9p/server_fallback.go
🧬 Code graph analysis (21)
pkg/fileserver/fs9p/server_windows.go (1)
pkg/fileserver/fs9p/server_fallback.go (1)
GetHvsockListener(13-15)
vendor/github.com/DeedleFake/p9/encoding.go (2)
vendor/github.com/DeedleFake/p9/stat.go (2)
DirEntry(255-267)Stat(156-168)vendor/github.com/DeedleFake/p9/proto/encoding.go (2)
Read(34-38)Write(25-31)
vendor/github.com/DeedleFake/p9/dir_windows.go (1)
vendor/github.com/DeedleFake/p9/stat.go (3)
DirEntry(255-267)FileMode(23-23)ModeFromOS(43-78)
pkg/crc/machine/start.go (3)
pkg/os/exec.go (1)
RunPrivileged(48-59)pkg/crc/constants/constants.go (1)
Plan9HvsockPort(62-62)pkg/crc/logging/logging.go (1)
Warnf(100-102)
vendor/github.com/DeedleFake/p9/addr_other.go (1)
vendor/github.com/DeedleFake/p9/addr_unix.go (1)
NamespaceDir(17-29)
pkg/fileserver/fs9p/server.go (3)
vendor/github.com/DeedleFake/p9/fs.go (2)
FileSystem(21-34)FSConnHandler(162-167)vendor/github.com/DeedleFake/p9/proto/server.go (1)
Serve(17-42)pkg/crc/constants/constants.go (1)
Plan9Msize(59-59)
vendor/github.com/DeedleFake/p9/proto/server.go (2)
vendor/github.com/DeedleFake/p9/msg.go (1)
Proto(75-77)vendor/github.com/DeedleFake/p9/proto/proto.go (1)
Proto(38-41)
vendor/github.com/DeedleFake/p9/client.go (4)
vendor/github.com/DeedleFake/p9/proto/client.go (3)
Client(18-31)NewClient(35-56)Dial(60-67)vendor/github.com/DeedleFake/p9/msg.go (8)
Proto(75-77)Tversion(79-82)Tversion(84-84)Rversion(86-89)Tauth(95-99)Rauth(101-103)Tattach(105-110)Rattach(112-114)vendor/github.com/DeedleFake/p9/p9.go (3)
Version(10-10)NoFID(18-18)QID(42-46)vendor/github.com/DeedleFake/p9/remote.go (1)
Remote(19-27)
vendor/github.com/DeedleFake/p9/dir_linux.go (3)
vendor/github.com/DeedleFake/p9/stat.go (4)
DirEntry(255-267)FileMode(23-23)ModeFromOS(43-78)Stat(156-168)vendor/github.com/DeedleFake/p9/dir.go (1)
Dir(16-16)vendor/github.com/DeedleFake/p9/p9.go (3)
QID(42-46)QIDType(49-49)Version(10-10)
vendor/github.com/DeedleFake/p9/internal/debug/nodebug.go (1)
vendor/github.com/DeedleFake/p9/internal/debug/debug.go (1)
Log(10-12)
cmd/crc/cmd/daemon.go (4)
pkg/fileserver/fs9p/server_fallback.go (1)
GetHvsockListener(13-15)pkg/fileserver/fs9p/server_windows.go (1)
GetHvsockListener(14-29)pkg/crc/constants/constants.go (3)
Plan9HvsockGUID(61-61)GetHomeDir(166-172)Plan9TcpPort(60-60)pkg/fileserver/fs9p/server.go (1)
New9pServer(29-52)
vendor/github.com/DeedleFake/p9/remote.go (7)
vendor/github.com/DeedleFake/p9/client.go (1)
Client(22-26)vendor/github.com/DeedleFake/p9/proto/client.go (1)
Client(18-31)vendor/github.com/DeedleFake/p9/p9.go (4)
QID(42-46)QIDType(49-49)Version(10-10)IOHeaderSize(70-70)vendor/github.com/DeedleFake/p9/msg.go (14)
Twalk(133-137)Rwalk(139-141)Topen(143-146)Ropen(148-151)Tcreate(153-158)Rcreate(160-163)Tremove(192-194)Tread(165-169)Rread(171-173)Twrite(175-179)Rwrite(181-183)Tclunk(185-187)Tstat(199-201)Rstat(203-205)vendor/github.com/DeedleFake/p9/stat.go (3)
FileMode(23-23)Stat(156-168)DirEntry(255-267)vendor/github.com/DeedleFake/p9/internal/util/util.go (1)
Errorf(44-52)vendor/github.com/DeedleFake/p9/encoding.go (1)
ReadDir(16-30)
vendor/github.com/DeedleFake/p9/dir.go (3)
vendor/github.com/DeedleFake/p9/stat.go (5)
Stat(156-168)DirEntry(255-267)StatChanges(316-318)FileMode(23-23)ModeDir(27-27)vendor/github.com/DeedleFake/p9/fs.go (4)
File(100-119)Attachment(44-74)FileSystem(21-34)QIDFS(89-91)vendor/github.com/DeedleFake/p9/p9.go (7)
OWRITE(27-27)ORDWR(28-28)OEXEC(29-29)OTRUNC(31-31)OCEXEC(32-32)ORCLOSE(33-33)OREAD(26-26)
vendor/github.com/DeedleFake/p9/fs.go (7)
vendor/github.com/DeedleFake/p9/stat.go (4)
Stat(156-168)DirEntry(255-267)StatChanges(316-318)FileMode(23-23)vendor/github.com/DeedleFake/p9/p9.go (7)
QID(42-46)QIDType(49-49)IOHeaderSize(70-70)Version(10-10)QTAuth(58-58)NoFID(18-18)QTDir(54-54)vendor/github.com/DeedleFake/p9/proto/server.go (1)
MessageHandler(135-139)vendor/github.com/DeedleFake/p9/internal/debug/debug.go (1)
Log(10-12)vendor/github.com/DeedleFake/p9/internal/debug/nodebug.go (1)
Log(5-5)vendor/github.com/DeedleFake/p9/msg.go (27)
Tversion(79-82)Tversion(84-84)Rerror(118-120)Rversion(86-89)Tauth(95-99)Rauth(101-103)Tflush(126-128)Tattach(105-110)Rattach(112-114)Twalk(133-137)Rwalk(139-141)Topen(143-146)Ropen(148-151)Tcreate(153-158)Rcreate(160-163)Tread(165-169)Rread(171-173)Twrite(175-179)Rwrite(181-183)Tclunk(185-187)Rclunk(189-190)Tremove(192-194)Rremove(196-197)Tstat(199-201)Rstat(203-205)Twstat(235-238)Rwstat(278-279)vendor/github.com/DeedleFake/p9/encoding.go (1)
WriteDir(33-42)
vendor/github.com/DeedleFake/p9/stat.go (3)
vendor/github.com/DeedleFake/p9/p9.go (3)
QIDType(49-49)QID(42-46)Version(10-10)vendor/github.com/DeedleFake/p9/proto/encoding.go (3)
Write(25-31)Read(34-38)Size(14-20)vendor/github.com/DeedleFake/p9/internal/util/util.go (1)
LimitedReader(13-17)
vendor/github.com/DeedleFake/p9/proto/client.go (5)
vendor/github.com/DeedleFake/p9/msg.go (1)
Proto(75-77)vendor/github.com/DeedleFake/p9/proto/proto.go (3)
Proto(38-41)NoTag(27-27)ErrClientClosed(22-22)vendor/github.com/DeedleFake/p9/client.go (3)
NewClient(30-32)Client(22-26)Dial(36-43)vendor/github.com/DeedleFake/p9/internal/debug/debug.go (1)
Log(10-12)vendor/github.com/DeedleFake/p9/internal/debug/nodebug.go (1)
Log(5-5)
pkg/crc/machine/libhvee/driver_windows.go (1)
vendor/github.com/crc-org/machine/libmachine/drivers/base.go (1)
SharedDir(27-35)
vendor/github.com/DeedleFake/p9/addr.go (2)
vendor/github.com/DeedleFake/p9/addr_other.go (1)
NamespaceDir(17-29)vendor/github.com/DeedleFake/p9/addr_unix.go (1)
NamespaceDir(17-29)
vendor/github.com/DeedleFake/p9/dir_darwin.go (3)
vendor/github.com/DeedleFake/p9/stat.go (4)
DirEntry(255-267)FileMode(23-23)ModeFromOS(43-78)Stat(156-168)vendor/github.com/DeedleFake/p9/dir.go (1)
Dir(16-16)vendor/github.com/DeedleFake/p9/p9.go (3)
QID(42-46)QIDType(49-49)Version(10-10)
vendor/github.com/DeedleFake/p9/dir_other.go (1)
vendor/github.com/DeedleFake/p9/stat.go (3)
DirEntry(255-267)FileMode(23-23)ModeFromOS(43-78)
pkg/fileserver/fs9p/server_fallback.go (1)
pkg/fileserver/fs9p/server_windows.go (1)
GetHvsockListener(14-29)
🪛 markdownlint-cli2 (0.18.1)
vendor/github.com/DeedleFake/p9/README.md
12-12: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
17-17: Hard tabs
Column: 1
(MD010, no-hard-tabs)
21-21: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
26-26: Hard tabs
Column: 1
(MD010, no-hard-tabs)
32-32: Hard tabs
Column: 1
(MD010, no-hard-tabs)
37-37: Hard tabs
Column: 1
(MD010, no-hard-tabs)
43-43: Hard tabs
Column: 1
(MD010, no-hard-tabs)
49-49: Hard tabs
Column: 1
(MD010, no-hard-tabs)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build (windows-2022, 1.24)
- GitHub Check: build (macOS-14, 1.24)
- GitHub Check: build (ubuntu-latest, 1.24)
- GitHub Check: Konflux kflux-prd-rh02 / crc-binary-on-pull-request
🔇 Additional comments (14)
vendor/github.com/DeedleFake/p9/internal/debug/nodebug.go (1)
3-5: LGTM! Clean no-op debug stub.The implementation correctly provides a no-op debug interface for non-debug builds. The function signature matches the active debug implementation, and the build constraint ensures proper conditional compilation.
pkg/crc/machine/libhvee/driver_windows.go (1)
4-4: LGTM: Import needed for dynamic tag generation.The
fmtimport is correctly added to support thefmt.Sprintfcall on line 43.vendor/github.com/DeedleFake/p9/addr.go (3)
32-66: LGTM - ParseAddr logic is sound.The multi-path parsing logic correctly handles namespace addresses, Unix sockets, TCP with ports, and the network!address[!port] format. The pseudo-port mapping for "9p" and "9fs" to standard port 564 is appropriate.
70-72: GetNamespace inherits Windows path issue from NamespaceDir().This function correctly constructs Unix socket paths for namespaces, but it will inherit the Windows path safety issue from
NamespaceDir()inaddr_other.go(where DISPLAY with ":" creates invalid Windows paths). Once that upstream issue is fixed, this function will work correctly on Windows.
76-78: LGTM - IsNamespaceAddr is correct.Simple and correct implementation for detecting namespace addresses by checking for the "$" prefix.
vendor/github.com/DeedleFake/p9/README.md (1)
1-51: Vendored p9 includes its LICENSE (v0.6.12); no changes required. Continue filing any fixes upstream and update the vendor commit rather than modifying vendored sources.pkg/fileserver/fs9p/server_windows.go (1)
12-29: LGTM! Clean HVSOCK listener implementation.The function correctly parses the HVSOCK GUID, creates a listener with a wildcard VMID to accept connections from any VM, and provides clear error messages with proper context wrapping.
vendor/github.com/DeedleFake/p9/proto/client.go (1)
1-224: Vendored 9P client looks reasonable.This is vendored code from
github.com/DeedleFake/p9. The implementation includes proper goroutine coordination, error handling, and resource cleanup. No obvious critical issues that would require immediate upstream reporting.Per the PR discussion, if issues are discovered later (particularly security-related), they should be reported upstream for proper propagation.
vendor/github.com/DeedleFake/p9/dir_other.go (1)
1-15: Vendored fallback implementation for non-major platforms.This is vendored code providing a minimal
os.FileInfo→DirEntryconversion for platforms other than Linux, Darwin, Plan9, and Windows. The implementation omits UID/GID/MUID/Path/Version fields (zero values). Since this PR targets Windows (which has its own platform-specific implementation), this fallback is unlikely to be exercised in crc.If issues arise with missing fields on edge-case platforms, they should be reported upstream.
pkg/fileserver/fs9p/server_fallback.go (1)
11-15: LGTM! Appropriate non-Windows stub.The fallback implementation correctly returns a not-implemented error for non-Windows platforms, allowing the codebase to compile across platforms while providing clear feedback if the function is mistakenly called.
vendor/github.com/DeedleFake/p9/dir_windows.go (1)
9-27: LGTM: safe type assertion and sane defaultsGraceful fallback when
Sys()lacks Win32 data; ATime filled when available; Mode/Length/Name always set.vendor/github.com/DeedleFake/p9/encoding.go (2)
16-30: LGTM: ReadDir handles EOF correctly and preserves partial resultsEOF normalization and accumulation are appropriate for directory streams.
33-41: LGTM: WriteDir writes sequentially and fails fastEarly return on first error is fine for callers.
vendor/github.com/DeedleFake/p9/dir_linux.go (1)
46-62: QID derivation looks correctUsing inode for Path and Mode-derived QID type is reasonable on Linux.
| func (d Dir) path(p string) string { | ||
| return filepath.Join(string(d), filepath.FromSlash(p)) | ||
| } |
There was a problem hiding this comment.
Constrain all filesystem access to the served root (escape via .. possible)
filepath.Join alone allows “..” to escape the base (e.g., /share/../../etc/passwd → /etc/passwd). Validate paths stay within the root for every operation.
@@
-func (d Dir) path(p string) string {
- return filepath.Join(string(d), filepath.FromSlash(p))
-}
+func (d Dir) path(p string) (string, error) {
+ base := string(d)
+ joined := filepath.Join(base, filepath.FromSlash(p))
+ rel, err := filepath.Rel(base, joined)
+ if err != nil {
+ return "", err
+ }
+ if rel == ".." || strings.HasPrefix(rel, ".."+string(os.PathSeparator)) {
+ return "", errors.New("path escapes base")
+ }
+ return joined, nil
+}
@@
- fi, err := os.Stat(d.path(p))
+ tpath, err := d.path(p)
+ if err != nil {
+ return DirEntry{}, err
+ }
+ fi, err := os.Stat(tpath)
@@
- p = d.path(p)
- base := filepath.Dir(p)
+ tpath, err := d.path(p)
+ if err != nil {
+ return err
+ }
+ base := filepath.Dir(tpath)
@@
- err := os.Chmod(p, mode.OS())
+ err := os.Chmod(tpath, mode.OS())
@@
- err := os.Chtimes(p, atime, mtime)
+ err := os.Chtimes(tpath, atime, mtime)
@@
- err := os.Truncate(p, int64(length))
+ err := os.Truncate(tpath, int64(length))
@@
- err := os.Rename(p, filepath.Join(base, name))
+ err := os.Rename(tpath, filepath.Join(base, name))
@@
- file, err := os.OpenFile(d.path(p), flag, 0644)
+ tpath, err := d.path(p)
+ if err != nil {
+ return nil, err
+ }
+ file, err := os.OpenFile(tpath, flag, 0644)
@@
- p = d.path(p)
+ tpath, err := d.path(p)
+ if err != nil {
+ return nil, err
+ }
@@
- err := os.Mkdir(p, os.FileMode(perm.Perm()))
+ err := os.Mkdir(tpath, os.FileMode(perm.Perm()))
@@
- file, err := os.OpenFile(p, flag|os.O_CREATE, os.FileMode(perm.Perm()))
+ file, err := os.OpenFile(tpath, flag|os.O_CREATE, os.FileMode(perm.Perm()))
@@
-func (d Dir) Remove(p string) error {
- return os.Remove(d.path(p))
-}
+func (d Dir) Remove(p string) error {
+ tpath, err := d.path(p)
+ if err != nil {
+ return err
+ }
+ return os.Remove(tpath)
+}Consider upstreaming this to the p9 project; keep a local patch until merged.
Also applies to: 22-35, 41-51, 61-68, 69-75, 95-103, 105-122, 124-127
🤖 Prompt for AI Agents
In vendor/github.com/DeedleFake/p9/dir.go around lines 18-20 (and also apply to
ranges 22-35, 41-51, 61-68, 69-75, 95-103, 105-122, 124-127), the current path
construction uses filepath.Join which allows “..” to escape the served root;
replace simple Join with a guarded resolution: compute the absolute/cleaned path
for the root and the requested path (use filepath.Clean and filepath.Join or
filepath.Abs as appropriate), then use filepath.Rel to get the path relative to
the root and reject any path where the relative result begins with ".." or
contains a path traversal; return an error or an empty/denied result when
traversal is detected. Ensure every file-system operation in the listed line
ranges performs this validation before accessing the filesystem so no operation
can escape the served root.
| sum := sha256.Sum256(*(*[]byte)(unsafe.Pointer(&p))) | ||
| path := binary.LittleEndian.Uint64(sum[:]) | ||
|
|
||
| return QID{ | ||
| Type: stat.FileMode.QIDType(), | ||
| Path: path, | ||
| }, nil |
There was a problem hiding this comment.
Unsafe string→[]byte cast corrupts slice header; use a safe conversion.
*(*[]byte)(unsafe.Pointer(&p)) is invalid (string and []byte headers differ). This is undefined behavior and can crash/corrupt data under GC/moving stack.
Apply:
- sum := sha256.Sum256(*(*[]byte)(unsafe.Pointer(&p)))
+ sum := sha256.Sum256([]byte(p))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| sum := sha256.Sum256(*(*[]byte)(unsafe.Pointer(&p))) | |
| path := binary.LittleEndian.Uint64(sum[:]) | |
| return QID{ | |
| Type: stat.FileMode.QIDType(), | |
| Path: path, | |
| }, nil | |
| sum := sha256.Sum256([]byte(p)) | |
| path := binary.LittleEndian.Uint64(sum[:]) | |
| return QID{ | |
| Type: stat.FileMode.QIDType(), | |
| Path: path, | |
| }, nil |
🤖 Prompt for AI Agents
In vendor/github.com/DeedleFake/p9/fs.go around lines 179 to 185, the code uses
an unsafe cast from string to []byte which corrupts the slice header; replace
the unsafe conversion with a safe conversion by passing a byte slice created
from the string (e.g., convert the string to []byte before hashing or use
sha256.Sum256([]byte(p))) so the slice header is correct and no unsafe.Pointer
is used; keep the rest of the logic (compute sha256, read uint64, return QID)
unchanged.
| rsp := h.clunk(&Tclunk{ | ||
| FID: msg.FID, | ||
| }) | ||
| if _, ok := rsp.(error); ok { | ||
| return rsp | ||
| } | ||
|
|
There was a problem hiding this comment.
remove(): wrong type assertion misses clunk error.
rsp.(error) is never true; clunk returns *Rerror. Propagate correctly.
- rsp := h.clunk(&Tclunk{
+ rsp := h.clunk(&Tclunk{
FID: msg.FID,
})
- if _, ok := rsp.(error); ok {
- return rsp
- }
+ if er, ok := rsp.(*Rerror); ok {
+ return er
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| rsp := h.clunk(&Tclunk{ | |
| FID: msg.FID, | |
| }) | |
| if _, ok := rsp.(error); ok { | |
| return rsp | |
| } | |
| rsp := h.clunk(&Tclunk{ | |
| FID: msg.FID, | |
| }) | |
| if er, ok := rsp.(*Rerror); ok { | |
| return er | |
| } |
🤖 Prompt for AI Agents
vendor/github.com/DeedleFake/p9/fs.go around lines 593-599: the current type
assertion uses rsp.(error) which never matches because clunk returns *Rerror;
change the check to assert for *Rerror (or use a type switch) and return that
error value so the clunk error is propagated correctly. Specifically, replace
the rsp.(error) assertion with a check like rsp.(*Rerror) (or switch on type)
and return the resulting *Rerror (or convert it to an error) when present.
| func (file *Remote) ReadAt(buf []byte, off int64) (int, error) { | ||
| size := len(buf) | ||
| if size > file.maxBufSize() { | ||
| size = file.maxBufSize() | ||
| } | ||
|
|
||
| var total int | ||
| for start := 0; start < len(buf); start += size { | ||
| end := start + size | ||
| if end > len(buf) { | ||
| end = len(buf) | ||
| } | ||
|
|
||
| n, err := file.readPart(buf[start:end], off+int64(start)) | ||
| total += n | ||
| if err != nil { | ||
| return total, err | ||
| } | ||
| } | ||
| return total, nil |
There was a problem hiding this comment.
ReadAt/WriteAt: guard non-positive chunk to prevent infinite loop.
If maxBufSize() <= 0, bail with a clear error before the for start += size loop.
func (file *Remote) ReadAt(buf []byte, off int64) (int, error) {
- size := len(buf)
- if size > file.maxBufSize() {
- size = file.maxBufSize()
- }
+ max := file.maxBufSize()
+ if max <= 0 {
+ return 0, util.Errorf("msize too small: %d", file.client.Msize())
+ }
+ size := len(buf)
+ if size > max {
+ size = max
+ }
@@
func (file *Remote) WriteAt(data []byte, off int64) (int, error) {
- size := len(data)
- if size > file.maxBufSize() {
- size = file.maxBufSize()
- }
+ max := file.maxBufSize()
+ if max <= 0 {
+ return 0, util.Errorf("msize too small: %d", file.client.Msize())
+ }
+ size := len(data)
+ if size > max {
+ size = max
+ }Also applies to: 289-308
🤖 Prompt for AI Agents
In vendor/github.com/DeedleFake/p9/remote.go around lines 229-248 (and similarly
289-308), the loop increments by chunk size without guarding against
non-positive size, which can cause an infinite loop when file.maxBufSize() <= 0;
add an explicit check after computing size that if size <= 0 return 0 and a
clear error (e.g., fmt.Errorf("invalid maxBufSize: %d", size) or a sentinel
error variable) before entering the for loop so the function bails out safely;
apply the same guard to the corresponding ReadAt/WriteAt block at lines 289-308.
| func (m FileMode) String() string { | ||
| buf := []byte("----------") | ||
|
|
||
| const types = "dalMATL!DpSug" | ||
| for i := range types { | ||
| if m&(1<<uint(31-i)) != 0 { | ||
| buf[0] = types[i] | ||
| } | ||
| } | ||
|
|
||
| const perms = "rwx" | ||
| for i := 1; i < len(buf); i++ { | ||
| if m&(1<<uint32(len(buf)-1-i)) != 0 { | ||
| buf[i] = perms[(i-1)%len(perms)] | ||
| } | ||
| } | ||
|
|
||
| return *(*string)(unsafe.Pointer(&buf)) | ||
| } |
There was a problem hiding this comment.
String() builds a string from a stack slice via unsafe; use safe conversion.
Returning *(*string)(unsafe.Pointer(&buf)) can point to freed memory.
- return *(*string)(unsafe.Pointer(&buf))
+ return string(buf)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (m FileMode) String() string { | |
| buf := []byte("----------") | |
| const types = "dalMATL!DpSug" | |
| for i := range types { | |
| if m&(1<<uint(31-i)) != 0 { | |
| buf[0] = types[i] | |
| } | |
| } | |
| const perms = "rwx" | |
| for i := 1; i < len(buf); i++ { | |
| if m&(1<<uint32(len(buf)-1-i)) != 0 { | |
| buf[i] = perms[(i-1)%len(perms)] | |
| } | |
| } | |
| return *(*string)(unsafe.Pointer(&buf)) | |
| } | |
| func (m FileMode) String() string { | |
| buf := []byte("----------") | |
| const types = "dalMATL!DpSug" | |
| for i := range types { | |
| if m&(1<<uint(31-i)) != 0 { | |
| buf[0] = types[i] | |
| } | |
| } | |
| const perms = "rwx" | |
| for i := 1; i < len(buf); i++ { | |
| if m&(1<<uint32(len(buf)-1-i)) != 0 { | |
| buf[i] = perms[(i-1)%len(perms)] | |
| } | |
| } | |
| return string(buf) | |
| } |
🤖 Prompt for AI Agents
In vendor/github.com/DeedleFake/p9/stat.go around lines 135–153, the String()
method returns *(*string)(unsafe.Pointer(&buf)) which can reference stack memory
and is unsafe; replace the unsafe conversion with a safe conversion by returning
string(buf) so the bytes are copied into a Go string, and remove the unsafe
import if no longer used.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (20)
packaging/windows/product.wxs.template (1)
69-73: Mark Hvsock component as 64-bit to avoid registry redirection.Without
Win64="yes"the installer writes the GuestCommunicationServices key under Wow6432Node, so the 9P HVSOCK endpoint never registers. Please add the attribute (same fix previously suggested).- <Component Id="Hvsock9pRegistryEntry" Guid="*"> + <Component Id="Hvsock9pRegistryEntry" Guid="*" Win64="yes">vendor/github.com/DeedleFake/p9/dir_darwin.go (1)
35-43: Fix ATime construction to use Timespec fields
sys.Atimespechas noUnix()method on Darwin, so this code does not compile. BuildATimewith theSec/Nsecfields (cast toint64) instead.- ATime: time.Unix(sys.Atimespec.Unix()), + ATime: time.Unix(int64(sys.Atimespec.Sec), int64(sys.Atimespec.Nsec)),vendor/github.com/DeedleFake/p9/dir.go (1)
18-127: Critical security issues from previous review remain unaddressed.This vendored file still contains the following critical vulnerabilities identified in prior reviews:
Path traversal vulnerability (lines 18-20 and throughout):
filepath.Joinalone does not prevent ".." from escaping the served root directory, allowing clients to access arbitrary filesystem paths.Timestamp corruption (lines 52-60): Partial ATime/MTime updates can clobber timestamps when only one is provided.
Name change path traversal (lines 69-75): The rename operation allows path separators in names, enabling directory traversal.
Per the PR discussion and learnings, these issues should be addressed upstream in the DeedleFake/p9 repository. Consider filing PRs with the fixes suggested in the previous review comments, then updating the vendor when merged.
Based on learnings.
vendor/github.com/DeedleFake/p9/proto/proto.go (1)
85-94: Critical framing bug from previous review remains unaddressed.The
LimitedReaderbound is still set toN: sizeinstead ofN: size - 4. The 9P size field includes its own 4 bytes, so the remaining frame length after reading the size issize - 4. This off-by-4 error allows reads to leak 4 bytes into the next frame, breaking message boundaries and causing hard-to-debug protocol errors.As with other issues in this vendored code, consider filing an upstream PR with the fix suggested in the previous review comment.
Based on learnings.
vendor/github.com/DeedleFake/p9/fs.go (3)
179-185: Unsafe string→[]byte cast causes undefined behavior.Line 179 uses
*(*[]byte)(unsafe.Pointer(&p))to convert a string to[]bytefor hashing. This is invalid because string and slice headers have different layouts, resulting in undefined behavior that can corrupt memory or crash under GC/stack movement.The safe fix is simple:
sum := sha256.Sum256([]byte(p)).Based on learnings.
244-249: Tflush handler violates 9P spec by returning Rerror.The 9P specification requires that Tflush requests be acknowledged with Rflush, not Rerror. Even if the implementation cannot cancel in-flight operations, it should return
&Rflush{}as a best-effort acknowledgment.Based on learnings.
593-599: Type assertion bug prevents clunk error propagation.Line 596 checks
rsp.(error), buth.clunk()returns*Rerror, noterror. This assertion never succeeds, so clunk errors are silently ignored instead of being propagated to the client.The correct check is
if er, ok := rsp.(*Rerror); ok { return er }.Based on learnings.
vendor/github.com/DeedleFake/p9/proto/server.go (1)
70-99: Error handling bug allows handler invocation with invalid data.After a
Receiveerror at line 70, the code logs the error but continues to line 79, calling the handler with potentially garbagetmsgandtagvalues. This can cause panics or invalid protocol responses.The correct behavior is:
- On
io.EOFornet.ErrClosed: return/break to terminate the connection cleanly- On other errors: log and
continueto skip handler invocation and retry receivingThe previous review comment provides the complete fix.
Based on learnings.
vendor/github.com/DeedleFake/p9/remote.go (5)
34-71: Absolute path handling remains broken (duplicate).This issue was flagged in previous reviews:
walk()incorrectly splits absolute paths, producing empty elements or invalid "/" in Wname, violating the 9P protocol. Based on coding guidelines, file an upstream PR so the fix propagates when the vendor dependency is updated.
147-185: panic on invalid whence remains (duplicate).Previous review flagged that library code should not panic for invalid input. Return an error instead. File an upstream PR per project guidance.
198-200: maxBufSize() underflow guard missing (duplicate).Previous reviews identified that when
Msize() <= IOHeaderSize, the subtraction wraps/underflows, and returning 0 causes infinite loops in ReadAt/WriteAt. The extended comment in past reviews recommends validating negotiated msize at handshake (client.go) and clamping to a safe positive value here. File an upstream PR.
229-248: ReadAt loop guard missing (duplicate).Previous review flagged that if
maxBufSize() <= 0, the loop increments by zero or negative size, causing an infinite loop. Guard against non-positive chunk size before entering the loop. File an upstream PR.
288-308: WriteAt loop guard missing (duplicate).Previous review flagged the same infinite-loop risk as ReadAt. Guard against non-positive chunk size. File an upstream PR.
vendor/github.com/DeedleFake/p9/stat.go (1)
135-153: Unsafe string conversion creates dangling pointer (duplicate).Previous review correctly identified that
*(*string)(unsafe.Pointer(&buf))at line 152 creates a string header pointing to stack memory inbuf, which is freed when the function returns. This is a use-after-free bug. Usereturn string(buf)for safe conversion. File an upstream PR per project guidance.pkg/fileserver/fs9p/server.go (4)
3-13: Add missing import for errors.import ( + "errors" "fmt" "net" "os" "path/filepath"
71-81: Stop() is a hard close; consider graceful shutdown or tighten contract.If graceful isn’t feasible now, document at call sites and ensure callers either drain/ignore WaitForError (net.ErrClosed ignored per earlier change) and don’t leave clients stuck. Add a TODO with rationale and link to tracking issue.
42-50: Buffer ErrChan to avoid goroutine leak if nobody is receiving.Unbuffered channel can block the Serve goroutine on shutdown. Use capacity 1.
- fs := p9.FileSystem(p9.Dir(exposeDir)) - errChan := make(chan error) + fs := p9.FileSystem(p9.Dir(exposeDir)) + errChan := make(chan error, 1)
83-87: Treat net.ErrClosed as clean shutdown and handle closed channel.WaitForError should not surface net.ErrClosed when Stop() closes the listener; also handle closed channel.
-func (s *Server) WaitForError() error { - err := <-s.ErrChan - return err -} +func (s *Server) WaitForError() error { + if err, ok := <-s.ErrChan; ok { + if errors.Is(err, net.ErrClosed) { + return nil + } + return err + } + return nil +}vendor/github.com/DeedleFake/p9/proto/encoding.go (2)
33-38: Fail fast if Read target isn’t a non-nil pointer (prevents panic).Avoid panic when callers pass non-pointers or nil.
func Read(r io.Reader, v interface{}) error { - d := &decoder{r: r} - d.decode(reflect.ValueOf(v)) + d := &decoder{r: r} + rv := reflect.ValueOf(v) + if rv.Kind() != reflect.Ptr || rv.IsNil() { + return util.Errorf("proto.Read expects a non-nil pointer, got %T", v) + } + d.decode(rv) return d.err }
97-100: Guard against strings > 65535 bytes to avoid uint16 wrap/truncation.case reflect.String: - e.mode(uint16(v.Len())) - e.mode([]byte(v.String())) + if v.Len() > 0xFFFF { + e.err = util.Errorf("string too long: %d bytes", v.Len()) + return + } + e.mode(uint16(v.Len())) + e.mode([]byte(v.String()))
🧹 Nitpick comments (2)
pkg/fileserver/fs9p/server.go (2)
29-41: Guard against nil listener in New9pServer.Fail fast if a nil listener is passed.
func New9pServer(listener net.Listener, exposeDir string) (*Server, error) { + if listener == nil { + return nil, fmt.Errorf("listener must not be nil") + } // Verify that exposeDir makes sense.
55-69: Add an idempotency guard to Server.Start. Although existing callers only invoke Start once per instance (e.g. server9pHvsock and server9pTCP in cmd/crc/cmd/daemon.go), a repeated call would panic from closing ErrChan twice. Introduce astarted uint32flag and wrap Start withatomic.CompareAndSwapUint32(&s.started, 0, 1)to return an error on duplicate starts.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (44)
cmd/crc/cmd/daemon.go(5 hunks)cmd/crc/cmd/start.go(0 hunks)go.mod(2 hunks)packaging/windows/product.wxs.template(2 hunks)pkg/crc/config/settings.go(1 hunks)pkg/crc/constants/constants.go(1 hunks)pkg/crc/machine/driver.go(0 hunks)pkg/crc/machine/driver_darwin.go(0 hunks)pkg/crc/machine/driver_linux.go(0 hunks)pkg/crc/machine/driver_windows.go(0 hunks)pkg/crc/machine/libhvee/driver_windows.go(2 hunks)pkg/crc/machine/start.go(2 hunks)pkg/drivers/libhvee/libhvee_windows.go(0 hunks)pkg/drivers/libhvee/powershell_windows.go(0 hunks)pkg/fileserver/fs9p/server.go(1 hunks)pkg/fileserver/fs9p/server_fallback.go(1 hunks)pkg/fileserver/fs9p/server_windows.go(1 hunks)vendor/github.com/DeedleFake/p9/LICENSE(1 hunks)vendor/github.com/DeedleFake/p9/README.md(1 hunks)vendor/github.com/DeedleFake/p9/addr.go(1 hunks)vendor/github.com/DeedleFake/p9/addr_other.go(1 hunks)vendor/github.com/DeedleFake/p9/addr_unix.go(1 hunks)vendor/github.com/DeedleFake/p9/client.go(1 hunks)vendor/github.com/DeedleFake/p9/dir.go(1 hunks)vendor/github.com/DeedleFake/p9/dir_darwin.go(1 hunks)vendor/github.com/DeedleFake/p9/dir_linux.go(1 hunks)vendor/github.com/DeedleFake/p9/dir_other.go(1 hunks)vendor/github.com/DeedleFake/p9/dir_plan9.go(1 hunks)vendor/github.com/DeedleFake/p9/dir_windows.go(1 hunks)vendor/github.com/DeedleFake/p9/doc.go(1 hunks)vendor/github.com/DeedleFake/p9/encoding.go(1 hunks)vendor/github.com/DeedleFake/p9/fs.go(1 hunks)vendor/github.com/DeedleFake/p9/internal/debug/debug.go(1 hunks)vendor/github.com/DeedleFake/p9/internal/debug/nodebug.go(1 hunks)vendor/github.com/DeedleFake/p9/internal/util/util.go(1 hunks)vendor/github.com/DeedleFake/p9/msg.go(1 hunks)vendor/github.com/DeedleFake/p9/p9.go(1 hunks)vendor/github.com/DeedleFake/p9/proto/client.go(1 hunks)vendor/github.com/DeedleFake/p9/proto/encoding.go(1 hunks)vendor/github.com/DeedleFake/p9/proto/proto.go(1 hunks)vendor/github.com/DeedleFake/p9/proto/server.go(1 hunks)vendor/github.com/DeedleFake/p9/remote.go(1 hunks)vendor/github.com/DeedleFake/p9/stat.go(1 hunks)vendor/modules.txt(1 hunks)
💤 Files with no reviewable changes (7)
- pkg/crc/machine/driver_darwin.go
- pkg/crc/machine/driver_windows.go
- pkg/drivers/libhvee/powershell_windows.go
- pkg/drivers/libhvee/libhvee_windows.go
- pkg/crc/machine/driver.go
- cmd/crc/cmd/start.go
- pkg/crc/machine/driver_linux.go
✅ Files skipped from review due to trivial changes (1)
- vendor/github.com/DeedleFake/p9/doc.go
🚧 Files skipped from review as they are similar to previous changes (16)
- pkg/crc/constants/constants.go
- vendor/github.com/DeedleFake/p9/dir_other.go
- vendor/github.com/DeedleFake/p9/addr_unix.go
- pkg/crc/machine/start.go
- pkg/fileserver/fs9p/server_windows.go
- vendor/github.com/DeedleFake/p9/client.go
- go.mod
- vendor/github.com/DeedleFake/p9/proto/client.go
- vendor/modules.txt
- vendor/github.com/DeedleFake/p9/dir_linux.go
- pkg/crc/machine/libhvee/driver_windows.go
- vendor/github.com/DeedleFake/p9/LICENSE
- cmd/crc/cmd/daemon.go
- vendor/github.com/DeedleFake/p9/addr_other.go
- vendor/github.com/DeedleFake/p9/internal/debug/debug.go
- vendor/github.com/DeedleFake/p9/encoding.go
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: redbeam
PR: crc-org/crc#4866
File: pkg/fileserver/fs9p/shares.go:0-0
Timestamp: 2025-08-06T09:48:10.441Z
Learning: The vsock implementation in pkg/fileserver/fs9p/shares.go was temporarily removed because 9pfs doesn't fully support it yet. It will be added back when 9pfs provides proper vsock support.
📚 Learning: 2025-08-06T09:48:10.441Z
Learnt from: redbeam
PR: crc-org/crc#4866
File: pkg/fileserver/fs9p/shares.go:0-0
Timestamp: 2025-08-06T09:48:10.441Z
Learning: The vsock implementation in pkg/fileserver/fs9p/shares.go was temporarily removed because 9pfs doesn't fully support it yet. It will be added back when 9pfs provides proper vsock support.
Applied to files:
pkg/fileserver/fs9p/server_fallback.go
🧬 Code graph analysis (18)
vendor/github.com/DeedleFake/p9/internal/debug/nodebug.go (1)
vendor/github.com/DeedleFake/p9/internal/debug/debug.go (1)
Log(10-12)
pkg/crc/config/settings.go (2)
pkg/crc/config/validations.go (1)
ValidateBool(19-25)pkg/crc/config/callbacks.go (1)
SuccessfullyApplied(36-38)
vendor/github.com/DeedleFake/p9/dir_darwin.go (3)
vendor/github.com/DeedleFake/p9/stat.go (3)
DirEntry(255-267)FileMode(23-23)ModeFromOS(43-78)vendor/github.com/DeedleFake/p9/dir.go (1)
Dir(16-16)vendor/github.com/DeedleFake/p9/p9.go (3)
QID(42-46)QIDType(49-49)Version(10-10)
vendor/github.com/DeedleFake/p9/dir_windows.go (1)
vendor/github.com/DeedleFake/p9/stat.go (3)
DirEntry(255-267)FileMode(23-23)ModeFromOS(43-78)
pkg/fileserver/fs9p/server_fallback.go (1)
pkg/fileserver/fs9p/server_windows.go (1)
GetHvsockListener(14-29)
vendor/github.com/DeedleFake/p9/dir_plan9.go (3)
vendor/github.com/DeedleFake/p9/stat.go (3)
DirEntry(255-267)FileMode(23-23)ModeFromOS(43-78)vendor/github.com/DeedleFake/p9/dir.go (1)
Dir(16-16)vendor/github.com/DeedleFake/p9/p9.go (2)
QID(42-46)Version(10-10)
vendor/github.com/DeedleFake/p9/proto/proto.go (2)
vendor/github.com/DeedleFake/p9/proto/encoding.go (3)
Read(34-38)Write(25-31)Size(14-20)vendor/github.com/DeedleFake/p9/internal/util/util.go (2)
Errorf(44-52)LimitedReader(13-17)
vendor/github.com/DeedleFake/p9/addr.go (2)
vendor/github.com/DeedleFake/p9/addr_other.go (1)
NamespaceDir(17-29)vendor/github.com/DeedleFake/p9/addr_unix.go (1)
NamespaceDir(17-29)
vendor/github.com/DeedleFake/p9/fs.go (8)
vendor/github.com/DeedleFake/p9/stat.go (4)
Stat(156-168)DirEntry(255-267)StatChanges(316-318)FileMode(23-23)vendor/github.com/DeedleFake/p9/p9.go (5)
QID(42-46)QIDType(49-49)Version(10-10)QTAuth(58-58)QTDir(54-54)vendor/github.com/DeedleFake/p9/proto/server.go (3)
MessageHandler(135-139)ConnHandler(112-114)ConnHandlerFunc(125-125)vendor/github.com/DeedleFake/p9/internal/debug/debug.go (1)
Log(10-12)vendor/github.com/DeedleFake/p9/internal/debug/nodebug.go (1)
Log(5-5)vendor/github.com/DeedleFake/p9/msg.go (25)
Tversion(79-82)Tversion(84-84)Rversion(86-89)Tauth(95-99)Rauth(101-103)Tattach(105-110)Rattach(112-114)Twalk(133-137)Rwalk(139-141)Topen(143-146)Ropen(148-151)Tcreate(153-158)Rcreate(160-163)Tread(165-169)Rread(171-173)Twrite(175-179)Rwrite(181-183)Tclunk(185-187)Rclunk(189-190)Tremove(192-194)Rremove(196-197)Tstat(199-201)Rstat(203-205)Twstat(235-238)Rwstat(278-279)vendor/github.com/DeedleFake/p9/encoding.go (1)
WriteDir(33-42)vendor/github.com/DeedleFake/p9/proto/encoding.go (1)
Read(34-38)
vendor/github.com/DeedleFake/p9/proto/server.go (2)
vendor/github.com/DeedleFake/p9/msg.go (1)
Proto(75-77)vendor/github.com/DeedleFake/p9/proto/proto.go (1)
Proto(38-41)
pkg/fileserver/fs9p/server.go (3)
vendor/github.com/DeedleFake/p9/fs.go (2)
FileSystem(21-34)FSConnHandler(162-167)vendor/github.com/DeedleFake/p9/proto/server.go (1)
Serve(17-42)pkg/crc/constants/constants.go (1)
Plan9Msize(59-59)
vendor/github.com/DeedleFake/p9/dir.go (3)
vendor/github.com/DeedleFake/p9/stat.go (5)
Stat(156-168)DirEntry(255-267)StatChanges(316-318)FileMode(23-23)ModeDir(27-27)vendor/github.com/DeedleFake/p9/fs.go (4)
File(100-119)Attachment(44-74)FileSystem(21-34)QIDFS(89-91)vendor/github.com/DeedleFake/p9/p9.go (7)
OWRITE(27-27)ORDWR(28-28)OEXEC(29-29)OTRUNC(31-31)OCEXEC(32-32)ORCLOSE(33-33)OREAD(26-26)
vendor/github.com/DeedleFake/p9/remote.go (6)
vendor/github.com/DeedleFake/p9/client.go (1)
Client(22-26)vendor/github.com/DeedleFake/p9/p9.go (4)
QID(42-46)QIDType(49-49)Version(10-10)IOHeaderSize(70-70)vendor/github.com/DeedleFake/p9/msg.go (13)
Twalk(133-137)Rwalk(139-141)Topen(143-146)Ropen(148-151)Tcreate(153-158)Rcreate(160-163)Tread(165-169)Rread(171-173)Twrite(175-179)Rwrite(181-183)Tclunk(185-187)Tstat(199-201)Rstat(203-205)vendor/github.com/DeedleFake/p9/stat.go (3)
FileMode(23-23)Stat(156-168)DirEntry(255-267)vendor/github.com/DeedleFake/p9/internal/util/util.go (1)
Errorf(44-52)vendor/github.com/DeedleFake/p9/encoding.go (1)
ReadDir(16-30)
vendor/github.com/DeedleFake/p9/msg.go (5)
vendor/github.com/DeedleFake/p9/proto/proto.go (2)
NewProto(44-60)Proto(38-41)vendor/github.com/DeedleFake/p9/proto/client.go (1)
P9NoTag(221-223)vendor/github.com/DeedleFake/p9/stat.go (3)
FileMode(23-23)Stat(156-168)ErrLargeStat(18-18)vendor/github.com/DeedleFake/p9/proto/encoding.go (2)
Write(25-31)Read(34-38)vendor/github.com/DeedleFake/p9/internal/util/util.go (1)
LimitedReader(13-17)
vendor/github.com/DeedleFake/p9/proto/encoding.go (1)
vendor/github.com/DeedleFake/p9/internal/util/util.go (1)
Errorf(44-52)
vendor/github.com/DeedleFake/p9/stat.go (3)
vendor/github.com/DeedleFake/p9/p9.go (3)
QIDType(49-49)QID(42-46)Version(10-10)vendor/github.com/DeedleFake/p9/proto/encoding.go (3)
Write(25-31)Read(34-38)Size(14-20)vendor/github.com/DeedleFake/p9/internal/util/util.go (1)
LimitedReader(13-17)
vendor/github.com/DeedleFake/p9/p9.go (1)
vendor/github.com/DeedleFake/p9/stat.go (1)
FileMode(23-23)
vendor/github.com/DeedleFake/p9/internal/util/util.go (1)
vendor/github.com/DeedleFake/p9/proto/encoding.go (1)
Read(34-38)
🪛 markdownlint-cli2 (0.18.1)
vendor/github.com/DeedleFake/p9/README.md
12-12: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
17-17: Hard tabs
Column: 1
(MD010, no-hard-tabs)
21-21: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
26-26: Hard tabs
Column: 1
(MD010, no-hard-tabs)
32-32: Hard tabs
Column: 1
(MD010, no-hard-tabs)
37-37: Hard tabs
Column: 1
(MD010, no-hard-tabs)
43-43: Hard tabs
Column: 1
(MD010, no-hard-tabs)
49-49: Hard tabs
Column: 1
(MD010, no-hard-tabs)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Konflux kflux-prd-rh02 / crc-binary-on-pull-request
🔇 Additional comments (11)
vendor/github.com/DeedleFake/p9/internal/debug/nodebug.go (1)
1-5: LGTM! Vendored no-op debug implementation is correct.This vendored file correctly implements a no-op debug logger that's compiled when the
p9debugbuild tag is not set, complementing the active implementation indebug.go. The build constraint and function signature are appropriate for conditional compilation of debug logging.pkg/crc/config/settings.go (2)
87-88: LGTM! Clean cross-platform shared directory configuration.The unconditional
EnableSharedDirssetting correctly replaces the OS-specific SMB configuration with a universal approach that works with 9p. The default value oftruemaintains backward compatibility, and the implementation properly uses standard validation and callback functions.
34-34: Keep SharedDirPassword constant as it’s still in use
SharedDirPasswordis referenced in pkg/crc/machine/config/config.go, pkg/crc/machine/types/types.go, and pkg/crc/machine/start.go and must not be removed.Likely an incorrect or invalid review comment.
pkg/fileserver/fs9p/server_fallback.go (1)
1-15: LGTM!The build tag separation and error message are appropriate. The stub correctly signals that HVSOCK is Windows-only.
vendor/github.com/DeedleFake/p9/addr.go (1)
32-66: LGTM!The address parsing logic correctly handles multiple address formats (namespace, Unix socket, TCP, and bang notation). The implementation is sound for a vendored 9P library.
vendor/github.com/DeedleFake/p9/dir_windows.go (1)
9-27: LGTM!The Windows-specific FileInfo-to-DirEntry conversion correctly handles both cases (with and without syscall data) and properly extracts ATime from Windows file attributes.
vendor/github.com/DeedleFake/p9/dir_plan9.go (2)
10-31: LGTM!The Plan9-specific FileInfo-to-DirEntry conversion correctly handles syscall.Dir metadata extraction, including proper time conversion for Plan9's uint32 Atime field.
33-49: LGTM!The QID extraction correctly stats the file, validates the syscall.Dir type, and constructs the QID from the native Plan9 Qid fields.
vendor/github.com/DeedleFake/p9/p9.go (1)
1-72: LGTM – Vendored 9P constants and types.The constants and types defined here are standard 9P protocol scaffolding. No issues identified.
vendor/github.com/DeedleFake/p9/stat.go (1)
1-351: Rest of stat.go looks reasonable.FileMode conversions, Stat/DirEntry types, and P9Encode/P9Decode implementations follow safe patterns and match the 9P protocol specification.
vendor/github.com/DeedleFake/p9/msg.go (1)
1-280: LGTM – 9P message types and protocol registry.Message type constants, structs, and encoding/decoding implementations follow the 9P2000 specification and use safe patterns. No issues identified.
| case reflect.Array, reflect.Slice: | ||
| switch v.Type().Elem().Kind() { | ||
| case reflect.Uint8: | ||
| e.mode(uint32(v.Len())) | ||
| default: | ||
| e.mode(uint16(v.Len())) | ||
| } | ||
|
|
||
| for i := 0; i < v.Len(); i++ { | ||
| e.encode(v.Index(i)) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Also bound-check non-byte slice lengths encoded as uint16.
Prevents silent wrap for element counts > 65535.
case reflect.Array, reflect.Slice:
switch v.Type().Elem().Kind() {
case reflect.Uint8:
e.mode(uint32(v.Len()))
default:
+ if v.Len() > 0xFFFF {
+ e.err = util.Errorf("slice too long: %d elements", v.Len())
+ return
+ }
e.mode(uint16(v.Len()))
}Optionally confirm actual message sizes never exceed these bounds in your protocol types.
🏁 Script executed:
#!/bin/bash
# Heuristic: find encodes of large string/slice fields in p9 structs
rg -nP --type go -C2 'string|\\[\\]' vendor/github.com/DeedleFake/p9 | sed -n '1,200p'Length of output: 12185
🏁 Script executed:
#!/bin/bash
# Display the encode function and context around the Array/Slice case
rg -nP -C5 'func \(e \*encoder\) encode' -n vendor/github.com/DeedleFake/p9/proto/encoding.goLength of output: 202
🏁 Script executed:
#!/bin/bash
# Show import statements in encoding.go
rg -n '^import' -n -C2 vendor/github.com/DeedleFake/p9/proto/encoding.goLength of output: 58
Bound-check non-byte slices before encoding length
Add a guard to return an error if the slice length exceeds 65 535 before casting to uint16:
+ import "fmt"
…
case reflect.Array, reflect.Slice:
switch v.Type().Elem().Kind() {
case reflect.Uint8:
e.mode(uint32(v.Len()))
default:
+ if v.Len() > 0xFFFF {
+ e.err = fmt.Errorf("slice too long: %d elements", v.Len())
+ return
+ }
e.mode(uint16(v.Len()))
}Audit protocol message definitions to confirm no slice fields legitimately exceed this limit.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| case reflect.Array, reflect.Slice: | |
| switch v.Type().Elem().Kind() { | |
| case reflect.Uint8: | |
| e.mode(uint32(v.Len())) | |
| default: | |
| e.mode(uint16(v.Len())) | |
| } | |
| for i := 0; i < v.Len(); i++ { | |
| e.encode(v.Index(i)) | |
| } | |
| // --- a/vendor/github.com/DeedleFake/p9/proto/encoding.go | |
| // +++ b/vendor/github.com/DeedleFake/p9/proto/encoding.go | |
| @@ -1,7 +1,8 @@ | |
| import ( | |
| "io" | |
| "reflect" | |
| "fmt" | |
| // ... other imports ... | |
| ) | |
| @@ -85,12 +86,18 @@ func (e *encoder) encode(v reflect.Value) { | |
| case reflect.Array, reflect.Slice: | |
| switch v.Type().Elem().Kind() { | |
| case reflect.Uint8: | |
| e.mode(uint32(v.Len())) | |
| default: | |
| // Prevent uint16 overflow when encoding long slices | |
| if v.Len() > 0xFFFF { | |
| e.err = fmt.Errorf("slice too long: %d elements", v.Len()) | |
| return | |
| } | |
| e.mode(uint16(v.Len())) | |
| } | |
| for i := 0; i < v.Len(); i++ { | |
| e.encode(v.Index(i)) | |
| } |
🤖 Prompt for AI Agents
In vendor/github.com/DeedleFake/p9/proto/encoding.go around lines 85 to 95, the
code casts non-byte slice lengths to uint16 without checking bounds; add a guard
that checks if v.Len() > 65535 and return a descriptive error before calling
e.mode(uint16(v.Len())), keeping the existing uint32 path for []byte untouched,
and update callers/tests or propagate the error type so encoding failures are
handled; also audit protocol message definitions to ensure no legitimate slice
field can exceed 65,535 elements.
|
/hold |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (15)
pkg/crc/machine/start.go (2)
246-246: Fix argv tokenization:chown core:corewill fail binary lookupThe
RunPrivilegedmethod expects the binary and each argument as separate tokens. Passing"chown core:core"as a single string will attempt to execute a binary named"chown core:core"and fail.Apply this diff to split the command into separate arguments:
- if _, _, err := sshRunner.RunPrivileged("Changing owner of mount directory", "chown core:core", mount.Target); err != nil { + if _, _, err := sshRunner.RunPrivileged("Changing owner of mount directory", "chown", "core:core", mount.Target); err != nil {
249-251: Fix multiple critical issues with 9P mountingThis implementation has several critical issues:
- Argv tokenization:
"9pfs 192.168.127.1"will fail binary lookup (should be"9pfs", "192.168.127.1")- Missing privileges: Mounting requires root; using
Runinstead ofRunPrivilegedwill likely fail- Missing mount options: No trans, port, version, msize, cache specified
- Missing tag/aname: The 9P server can't resolve which export to mount without the tag
- Non-standard helper: Using
9pfsis brittle; prefer explicitmount -t 9pApply this diff to use explicit 9P mount with all required options:
- if _, _, err := sshRunner.Run("9pfs 192.168.127.1", mount.Target); err != nil { - return err + opts := fmt.Sprintf("trans=tcp,port=%d,msize=%d,cache=loose,version=9p2000.L,aname=%s", constants.Plan9Port, constants.Plan9Msize, mount.Tag) + if _, _, err := sshRunner.RunPrivileged( + fmt.Sprintf("Mounting %s (9p tag %s)", mount.Target, mount.Tag), + "mount", "-t", "9p", "-o", opts, "192.168.127.1", mount.Target, + ); err != nil { + return err }Note: This assumes
constants.Plan9Portandconstants.Plan9Msizeare defined. If not, use standard values (e.g., port 564, msize 65536).cmd/crc/cmd/daemon.go (1)
253-266: Stop and monitor the Windows 9p serverWe still start the 9p server but never tear it down or surface its failures; this leaks the listener/goroutine after
runreturns (tests or retries in the same process will fail with “address already in use”) and hides runtime faults. Please defer aStop()and forwardErrChanintoerrChso the daemon can shut down cleanly and react to 9p errors.if runtime.GOOS == "windows" { listener9p, err := vn.Listen("tcp", net.JoinHostPort(configuration.GatewayIP, fmt.Sprintf("%d", constants.Plan9TcpPort))) if err != nil { return err } server9p, err := fs9p.New9pServer(listener9p, constants.GetHomeDir()) if err != nil { return err } + defer func() { + if err := server9p.Stop(); err != nil { + logging.Errorf("error stopping 9p server: %v", err) + } + }() + go func() { + for err := range server9p.ErrChan { + errCh <- errors.Wrap(err, "9p server failed") + } + }() if err := server9p.Start(); err != nil { return err } }vendor/github.com/DeedleFake/p9/proto/encoding.go (2)
33-38: Guard against nil or non-pointer inputs in Read
Readstill callsreflect.Indirecton whatever comes in, so a nil interface or non-pointer will panic. Please fail fast before decoding.Apply this diff to harden the decoder:
func Read(r io.Reader, v interface{}) error { - d := &decoder{r: r} - d.decode(reflect.ValueOf(v)) + rv := reflect.ValueOf(v) + if rv.Kind() != reflect.Ptr || rv.IsNil() { + return util.Errorf("proto.Read expects a non-nil pointer, got %T", v) + } + + d := &decoder{r: r} + d.decode(rv) return d.err }
97-100: Enforce the 64 KiB string limit instead of truncatingCasting
lentouint16silently wraps for payloads >65 535 bytes, so the wire length prefix no longer matches the bytes we emit—subsequent fields decode incorrectly. Please detect oversize strings and return a descriptive error.Apply this diff to enforce the boundary:
case reflect.String: - e.mode(uint16(v.Len())) - e.mode([]byte(v.String())) + if v.Len() > 0xFFFF { + e.err = util.Errorf("string too long: %d bytes", v.Len()) + return + } + e.mode(uint16(v.Len())) + e.mode([]byte(v.String()))vendor/github.com/DeedleFake/p9/dir_darwin.go (1)
35-41: Restore darwin build: use Sec/Nsec when building ATime
sys.Atimespec.Unix()does not exist on Darwin, so this file will not compile. Please construct the timestamp with theSec/Nsecfields explicitly.Apply this diff to fix the build:
return DirEntry{ FileMode: ModeFromOS(fi.Mode()), - ATime: time.Unix(sys.Atimespec.Unix()), + ATime: time.Unix(int64(sys.Atimespec.Sec), int64(sys.Atimespec.Nsec)), MTime: fi.ModTime(), Length: uint64(fi.Size()), EntryName: fi.Name(), UID: uname, GID: gname,vendor/github.com/DeedleFake/p9/dir.go (3)
3-35: Keep requests inside the served root
Dir.pathjust doesfilepath.Join, so inputs like../../Windows/System32break out of the share. Every method here relies on it, so this is a full sandbox bypass.dir := p9.Dir("C:\\crc") _, _ = dir.Open("../../Windows/System32", p9.OREAD) // escapes rootPlease harden
path()to validate the resolved path stays under the base and thread that error through all call sites (Stat, WriteStat, Open, Create, Remove, etc.).@@ -import ( - "errors" - "os" - "path/filepath" -) +import ( + "errors" + "os" + "path/filepath" + "strings" +) @@ -func (d Dir) path(p string) string { - return filepath.Join(string(d), filepath.FromSlash(p)) +func (d Dir) path(p string) (string, error) { + base := string(d) + joined := filepath.Join(base, filepath.FromSlash(p)) + rel, err := filepath.Rel(base, joined) + if err != nil { + return "", err + } + if rel == ".." || strings.HasPrefix(rel, ".."+string(os.PathSeparator)) { + return "", errors.New("path escapes base") + } + return joined, nil } @@ - fi, err := os.Stat(d.path(p)) + tpath, err := d.path(p) + if err != nil { + return DirEntry{}, err + } + fi, err := os.Stat(tpath)Apply the same pattern anywhere
d.pathis used.Also applies to: 41-126
39-68: Reject partial ATime/MTime updatesPassing only one of ATime/MTime drops the other to the zero time via
os.Chtimes, silently corrupting metadata.- atime, ok1 := changes.ATime() - mtime, ok2 := changes.MTime() - if ok1 || ok2 { - err := os.Chtimes(p, atime, mtime) + atime, ok1 := changes.ATime() + mtime, ok2 := changes.MTime() + if ok1 != ok2 { + return errors.New("setting only one of ATime or MTime is not supported") + } + if ok1 && ok2 { + err := os.Chtimes(tpath, atime, mtime)
69-75: Validate rename targets stay in the same directory
filepath.Join(base, filepath.FromSlash(name))accepts inputs with separators and.., letting clients move files outsidebase.- name, ok := changes.Name() - if ok { - err := os.Rename(p, filepath.Join(base, filepath.FromSlash(name))) + name, ok := changes.Name() + if ok { + if name != filepath.Base(name) || strings.ContainsRune(name, os.PathSeparator) { + return errors.New("invalid name: must be a base name without path separators") + } + err := os.Rename(tpath, filepath.Join(base, name))vendor/github.com/DeedleFake/p9/proto/server.go (1)
3-8: Don’t dispatch after a failed ReceiveAfter
p.Receivefails you still call the handler with zeroedtmsg/tag, which can panic or send garbage, and you loop forever onnet.ErrClosed.-import ( +import ( + "errors" "io" "log" "net" "sync" ) @@ - if err != nil { - if err == io.EOF { - return - } - - log.Printf("Error reading message: %v", err) - } + if err != nil { + if err == io.EOF || errors.Is(err, net.ErrClosed) { + return + } + log.Printf("Error reading message: %v", err) + continue + }Also applies to: 69-99
vendor/github.com/DeedleFake/p9/proto/proto.go (1)
85-114: Fix 9P framing off-by-4
sizealready includes the 4-byte length field, so bounding the reader withN: sizelets reads spill into the next frame. Frames under 4 bytes should be rejected.- if (msize > 0) && (size > msize) { + if (msize > 0) && (size > msize) { return nil, NoTag, util.Errorf("receive: %w", ErrLargeMessage) } - - lr := &util.LimitedReader{ - R: r, - N: size, - E: ErrLargeMessage, - } + if size < 4 { + return nil, NoTag, util.Errorf("receive: invalid frame size: %d", size) + } + lr := &util.LimitedReader{ + R: r, + N: size - 4, + E: ErrLargeMessage, + }vendor/github.com/DeedleFake/p9/remote.go (3)
34-67: Produce valid Twalk componentsLeading slashes turn into empty
Wnameentries (["", "foo"]) and/becomes["/"], both invalid. Absolute roots should walk zero components; everything else must split into non-empty names.- w := []string{path.Clean(p)} - if w[0] != "/" { - w = strings.Split(w[0], "/") - } - if (len(w) == 1) && (w[0] == ".") { - w = nil - } + p = path.Clean(p) + var w []string + switch { + case p == "", p == ".": + // stay on current fid + case strings.HasPrefix(p, "/"): + p = strings.TrimPrefix(p, "/") + if p != "" { + w = strings.FieldsFunc(p, func(r rune) bool { return r == '/' }) + } + default: + w = strings.FieldsFunc(p, func(r rune) bool { return r == '/' }) + }
151-185: Return an error for invalid whenceLibrary code shouldn’t panic on bad input; return the current position and an error instead.
default: - panic(util.Errorf("Invalid whence: %v", whence)) + return int64(file.pos), util.Errorf("invalid whence: %v", whence) }
198-308: Guard negotiated msize before chunking
int(client.Msize() - IOHeaderSize)underflows when msize ≤ header, and ReadAt/WriteAt then loop with size≤0. Fail fast with a clear error.func (file *Remote) maxBufSize() int { - return int(file.client.Msize() - IOHeaderSize) + msize := file.client.Msize() + if msize <= uint32(IOHeaderSize) { + return 0 + } + return int(msize - uint32(IOHeaderSize)) } @@ func (file *Remote) ReadAt(buf []byte, off int64) (int, error) { - size := len(buf) - if size > file.maxBufSize() { - size = file.maxBufSize() - } + max := file.maxBufSize() + if max <= 0 { + return 0, util.Errorf("msize too small: %d", file.client.Msize()) + } + size := len(buf) + if size > max { + size = max + } @@ func (file *Remote) WriteAt(data []byte, off int64) (int, error) { - size := len(data) - if size > file.maxBufSize() { - size = file.maxBufSize() - } + max := file.maxBufSize() + if max <= 0 { + return 0, util.Errorf("msize too small: %d", file.client.Msize()) + } + size := len(data) + if size > max { + size = max + }vendor/github.com/DeedleFake/p9/stat.go (1)
3-12: Remove unsafe string conversion
return *(*string)(unsafe.Pointer(&buf))hands back a string referencing stack memory. Use the standard conversion and drop the unsafe import.-import ( - "bytes" - "errors" - "io" - "os" - "time" - "unsafe" +import ( + "bytes" + "errors" + "io" + "os" + "time" @@ - return *(*string)(unsafe.Pointer(&buf)) + return string(buf)Also applies to: 135-153
🧹 Nitpick comments (1)
vendor/github.com/DeedleFake/p9/internal/debug/nodebug.go (1)
1-5: LGTM! Vendored no-op debug implementation is correct.The no-op
Logfunction correctly provides zero-overhead debug logging when thep9debugbuild tag is not set. The implementation matches the expected pattern for conditional compilation of debug helpers.Note: The build constraint uses the older
// +buildsyntax rather than the newer//go:builddirective introduced in Go 1.17. While both syntaxes remain valid, the newer directive is preferred. However, since this is vendored code, any such improvements should be contributed upstream per the guidance from cfergeau.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (42)
cmd/crc/cmd/daemon.go(5 hunks)cmd/crc/cmd/start.go(0 hunks)go.mod(1 hunks)packaging/windows/product.wxs.template(0 hunks)pkg/crc/config/settings.go(1 hunks)pkg/crc/constants/constants.go(1 hunks)pkg/crc/machine/driver.go(0 hunks)pkg/crc/machine/driver_darwin.go(0 hunks)pkg/crc/machine/driver_linux.go(0 hunks)pkg/crc/machine/driver_windows.go(0 hunks)pkg/crc/machine/libhvee/driver_windows.go(2 hunks)pkg/crc/machine/start.go(2 hunks)pkg/drivers/libhvee/libhvee_windows.go(0 hunks)pkg/drivers/libhvee/powershell_windows.go(0 hunks)pkg/fileserver/fs9p/server.go(1 hunks)vendor/github.com/DeedleFake/p9/LICENSE(1 hunks)vendor/github.com/DeedleFake/p9/README.md(1 hunks)vendor/github.com/DeedleFake/p9/addr.go(1 hunks)vendor/github.com/DeedleFake/p9/addr_other.go(1 hunks)vendor/github.com/DeedleFake/p9/addr_unix.go(1 hunks)vendor/github.com/DeedleFake/p9/client.go(1 hunks)vendor/github.com/DeedleFake/p9/dir.go(1 hunks)vendor/github.com/DeedleFake/p9/dir_darwin.go(1 hunks)vendor/github.com/DeedleFake/p9/dir_linux.go(1 hunks)vendor/github.com/DeedleFake/p9/dir_other.go(1 hunks)vendor/github.com/DeedleFake/p9/dir_plan9.go(1 hunks)vendor/github.com/DeedleFake/p9/dir_windows.go(1 hunks)vendor/github.com/DeedleFake/p9/doc.go(1 hunks)vendor/github.com/DeedleFake/p9/encoding.go(1 hunks)vendor/github.com/DeedleFake/p9/fs.go(1 hunks)vendor/github.com/DeedleFake/p9/internal/debug/debug.go(1 hunks)vendor/github.com/DeedleFake/p9/internal/debug/nodebug.go(1 hunks)vendor/github.com/DeedleFake/p9/internal/util/util.go(1 hunks)vendor/github.com/DeedleFake/p9/msg.go(1 hunks)vendor/github.com/DeedleFake/p9/p9.go(1 hunks)vendor/github.com/DeedleFake/p9/proto/client.go(1 hunks)vendor/github.com/DeedleFake/p9/proto/encoding.go(1 hunks)vendor/github.com/DeedleFake/p9/proto/proto.go(1 hunks)vendor/github.com/DeedleFake/p9/proto/server.go(1 hunks)vendor/github.com/DeedleFake/p9/remote.go(1 hunks)vendor/github.com/DeedleFake/p9/stat.go(1 hunks)vendor/modules.txt(1 hunks)
💤 Files with no reviewable changes (8)
- pkg/crc/machine/driver_darwin.go
- pkg/drivers/libhvee/powershell_windows.go
- cmd/crc/cmd/start.go
- pkg/crc/machine/driver.go
- pkg/drivers/libhvee/libhvee_windows.go
- packaging/windows/product.wxs.template
- pkg/crc/machine/driver_linux.go
- pkg/crc/machine/driver_windows.go
✅ Files skipped from review due to trivial changes (2)
- vendor/github.com/DeedleFake/p9/LICENSE
- vendor/github.com/DeedleFake/p9/doc.go
🚧 Files skipped from review as they are similar to previous changes (16)
- go.mod
- pkg/crc/config/settings.go
- vendor/github.com/DeedleFake/p9/dir_linux.go
- vendor/github.com/DeedleFake/p9/dir_windows.go
- vendor/github.com/DeedleFake/p9/encoding.go
- pkg/fileserver/fs9p/server.go
- vendor/github.com/DeedleFake/p9/internal/util/util.go
- vendor/github.com/DeedleFake/p9/internal/debug/debug.go
- vendor/github.com/DeedleFake/p9/addr_other.go
- vendor/github.com/DeedleFake/p9/fs.go
- pkg/crc/machine/libhvee/driver_windows.go
- pkg/crc/constants/constants.go
- vendor/github.com/DeedleFake/p9/addr_unix.go
- vendor/github.com/DeedleFake/p9/msg.go
- vendor/github.com/DeedleFake/p9/client.go
- vendor/github.com/DeedleFake/p9/p9.go
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: redbeam
PR: crc-org/crc#4866
File: pkg/fileserver/fs9p/shares.go:0-0
Timestamp: 2025-08-06T09:48:10.441Z
Learning: The vsock implementation in pkg/fileserver/fs9p/shares.go was temporarily removed because 9pfs doesn't fully support it yet. It will be added back when 9pfs provides proper vsock support.
🧬 Code graph analysis (14)
pkg/crc/machine/start.go (1)
pkg/os/exec.go (1)
RunPrivileged(48-59)
vendor/github.com/DeedleFake/p9/dir_darwin.go (3)
vendor/github.com/DeedleFake/p9/stat.go (4)
DirEntry(255-267)FileMode(23-23)ModeFromOS(43-78)Stat(156-168)vendor/github.com/DeedleFake/p9/dir.go (1)
Dir(16-16)vendor/github.com/DeedleFake/p9/p9.go (3)
QID(42-46)QIDType(49-49)Version(10-10)
vendor/github.com/DeedleFake/p9/proto/encoding.go (1)
vendor/github.com/DeedleFake/p9/internal/util/util.go (1)
Errorf(44-52)
vendor/github.com/DeedleFake/p9/proto/server.go (2)
vendor/github.com/DeedleFake/p9/msg.go (1)
Proto(75-77)vendor/github.com/DeedleFake/p9/proto/proto.go (1)
Proto(38-41)
vendor/github.com/DeedleFake/p9/internal/debug/nodebug.go (1)
vendor/github.com/DeedleFake/p9/internal/debug/debug.go (1)
Log(10-12)
vendor/github.com/DeedleFake/p9/proto/client.go (4)
vendor/github.com/DeedleFake/p9/proto/proto.go (3)
Proto(38-41)NoTag(27-27)ErrClientClosed(22-22)vendor/github.com/DeedleFake/p9/client.go (3)
NewClient(30-32)Client(22-26)Dial(36-43)vendor/github.com/DeedleFake/p9/internal/debug/debug.go (1)
Log(10-12)vendor/github.com/DeedleFake/p9/internal/debug/nodebug.go (1)
Log(5-5)
cmd/crc/cmd/daemon.go (2)
pkg/crc/constants/constants.go (2)
Plan9TcpPort(60-60)GetHomeDir(164-170)pkg/fileserver/fs9p/server.go (1)
New9pServer(29-53)
vendor/github.com/DeedleFake/p9/dir_plan9.go (3)
vendor/github.com/DeedleFake/p9/stat.go (3)
DirEntry(255-267)FileMode(23-23)ModeFromOS(43-78)vendor/github.com/DeedleFake/p9/dir.go (1)
Dir(16-16)vendor/github.com/DeedleFake/p9/p9.go (2)
QID(42-46)Version(10-10)
vendor/github.com/DeedleFake/p9/addr.go (2)
vendor/github.com/DeedleFake/p9/addr_other.go (1)
NamespaceDir(17-29)vendor/github.com/DeedleFake/p9/addr_unix.go (1)
NamespaceDir(17-29)
vendor/github.com/DeedleFake/p9/remote.go (7)
vendor/github.com/DeedleFake/p9/client.go (1)
Client(22-26)vendor/github.com/DeedleFake/p9/proto/client.go (1)
Client(18-31)vendor/github.com/DeedleFake/p9/p9.go (4)
QID(42-46)QIDType(49-49)Version(10-10)IOHeaderSize(70-70)vendor/github.com/DeedleFake/p9/msg.go (14)
Twalk(133-137)Rwalk(139-141)Topen(143-146)Ropen(148-151)Tcreate(153-158)Rcreate(160-163)Tremove(192-194)Tread(165-169)Rread(171-173)Twrite(175-179)Rwrite(181-183)Tclunk(185-187)Tstat(199-201)Rstat(203-205)vendor/github.com/DeedleFake/p9/stat.go (3)
FileMode(23-23)Stat(156-168)DirEntry(255-267)vendor/github.com/DeedleFake/p9/internal/util/util.go (1)
Errorf(44-52)vendor/github.com/DeedleFake/p9/encoding.go (1)
ReadDir(16-30)
vendor/github.com/DeedleFake/p9/proto/proto.go (3)
vendor/github.com/DeedleFake/p9/msg.go (1)
Proto(75-77)vendor/github.com/DeedleFake/p9/proto/encoding.go (3)
Read(34-38)Write(25-31)Size(14-20)vendor/github.com/DeedleFake/p9/internal/util/util.go (2)
Errorf(44-52)LimitedReader(13-17)
vendor/github.com/DeedleFake/p9/dir.go (3)
vendor/github.com/DeedleFake/p9/stat.go (5)
Stat(156-168)DirEntry(255-267)StatChanges(316-318)FileMode(23-23)ModeDir(27-27)vendor/github.com/DeedleFake/p9/fs.go (4)
File(100-119)Attachment(44-74)FileSystem(21-34)QIDFS(89-91)vendor/github.com/DeedleFake/p9/p9.go (7)
OWRITE(27-27)ORDWR(28-28)OEXEC(29-29)OTRUNC(31-31)OCEXEC(32-32)ORCLOSE(33-33)OREAD(26-26)
vendor/github.com/DeedleFake/p9/dir_other.go (1)
vendor/github.com/DeedleFake/p9/stat.go (3)
DirEntry(255-267)FileMode(23-23)ModeFromOS(43-78)
vendor/github.com/DeedleFake/p9/stat.go (3)
vendor/github.com/DeedleFake/p9/p9.go (3)
QIDType(49-49)QID(42-46)Version(10-10)vendor/github.com/DeedleFake/p9/proto/encoding.go (3)
Write(25-31)Read(34-38)Size(14-20)vendor/github.com/DeedleFake/p9/internal/util/util.go (1)
LimitedReader(13-17)
🪛 markdownlint-cli2 (0.18.1)
vendor/github.com/DeedleFake/p9/README.md
12-12: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
17-17: Hard tabs
Column: 1
(MD010, no-hard-tabs)
21-21: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
26-26: Hard tabs
Column: 1
(MD010, no-hard-tabs)
32-32: Hard tabs
Column: 1
(MD010, no-hard-tabs)
37-37: Hard tabs
Column: 1
(MD010, no-hard-tabs)
43-43: Hard tabs
Column: 1
(MD010, no-hard-tabs)
49-49: Hard tabs
Column: 1
(MD010, no-hard-tabs)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build (macOS-14, 1.24)
- GitHub Check: Konflux kflux-prd-rh02 / crc-binary-on-pull-request
| parts := strings.SplitN(addr, ":", 2) | ||
| if len(parts) == 2 { | ||
| if (parts[1] == "9p") || (parts[1] == "9fs") { | ||
| parts[1] = standardPort | ||
| } | ||
|
|
||
| return "tcp", strings.Join(parts, ":") | ||
| } |
There was a problem hiding this comment.
Fix IPv6 host:port parsing
strings.SplitN(addr, ":", 2) assumes single-colon host:port pairs. For IPv6 literals (e.g. [fe80::1]:564) it splits at the first colon, mangles the host, and even returns "tcp" as the network with an invalid address like [fe80:::1]:564. Any attempt to dial that address fails, so IPv6 targets are currently broken. Please switch to net.SplitHostPort (or equivalent) so bracketed IPv6 and other valid host:port strings survive intact.
Apply this diff to fix the parsing:
@@
-import (
- "path/filepath"
- "strings"
-)
+import (
+ "net"
+ "path/filepath"
+ "strings"
+)
@@
- parts := strings.SplitN(addr, ":", 2)
- if len(parts) == 2 {
- if (parts[1] == "9p") || (parts[1] == "9fs") {
- parts[1] = standardPort
- }
-
- return "tcp", strings.Join(parts, ":")
- }
+ if strings.Contains(addr, ":") {
+ if host, port, err := net.SplitHostPort(addr); err == nil {
+ switch port {
+ case "9p", "9fs":
+ port = standardPort
+ }
+ return "tcp", net.JoinHostPort(host, port)
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| parts := strings.SplitN(addr, ":", 2) | |
| if len(parts) == 2 { | |
| if (parts[1] == "9p") || (parts[1] == "9fs") { | |
| parts[1] = standardPort | |
| } | |
| return "tcp", strings.Join(parts, ":") | |
| } | |
| ++ b/vendor/github.com/DeedleFake/p9/addr.go | |
| @@ –1,6 +1,7 @@ | |
| -import ( | |
| - "path/filepath" | |
| - "strings" | |
| import ( | |
| "net" | |
| "path/filepath" | |
| "strings" | |
| ) | |
| func parseAddr(addr string) (network, address string) { | |
| @@ –41,9 +42,17 @@ func parseAddr(addr string) (network, address string) { | |
| - parts := strings.SplitN(addr, ":", 2) | |
| - if len(parts) == 2 { | |
| - if (parts[1] == "9p") || (parts[1] == "9fs") { | |
| - parts[1] = standardPort | |
| - } | |
| - | |
| - return "tcp", strings.Join(parts, ":") | |
| if strings.Contains(addr, ":") { | |
| if host, port, err := net.SplitHostPort(addr); err == nil { | |
| switch port { | |
| case "9p", "9fs": | |
| port = standardPort | |
| } | |
| return "tcp", net.JoinHostPort(host, port) | |
| } | |
| } | |
| // fallback or other protocols… | |
| return "unix", filepath.Clean(addr) |
| case c.nextTag <- nextTag: | ||
| for { | ||
| nextTag++ | ||
| if _, ok := tags[nextTag]; !ok { | ||
| break | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Skip NoTag and avoid spinning when the tag space is exhausted
The coordinator hands out nextTag verbatim. Once it wraps, it happily returns 0xFFFF, which is reserved as NoTag in 9P, so the client will send illegal tags. If every tag is in use, the for loop also spins forever because there is no free value to break on. Please skip NoTag explicitly and bail out (or wait) when you loop the full space so we never emit reserved tags or busy-loop.
Apply this diff to enforce the constraint:
case c.nextTag <- nextTag:
- for {
- nextTag++
- if _, ok := tags[nextTag]; !ok {
- break
- }
- }
+ start := nextTag
+ for {
+ nextTag++
+ if nextTag == NoTag {
+ nextTag++
+ }
+ if _, ok := tags[nextTag]; !ok {
+ break
+ }
+ if nextTag == start {
+ // All tags are in use; wait for ctx cancellation to avoid spinning.
+ <-ctx.Done()
+ return
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| case c.nextTag <- nextTag: | |
| for { | |
| nextTag++ | |
| if _, ok := tags[nextTag]; !ok { | |
| break | |
| } | |
| } | |
| } | |
| case c.nextTag <- nextTag: | |
| start := nextTag | |
| for { | |
| nextTag++ | |
| if nextTag == NoTag { | |
| nextTag++ | |
| } | |
| if _, ok := tags[nextTag]; !ok { | |
| break | |
| } | |
| if nextTag == start { | |
| // All tags are in use; wait for ctx cancellation to avoid spinning. | |
| <-ctx.Done() | |
| return | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In vendor/github.com/DeedleFake/p9/proto/client.go around lines 138 to 145, the
current loop that increments nextTag can return the reserved NoTag (0xFFFF) and
will spin forever if all tags are in use; update the loop to (1) treat NoTag
(0xFFFF) as invalid and skip it when encountered, and (2) detect a full cycle
(e.g., remember the starting tag and if incrementing wraps back to it) and bail
out instead of busy-waiting — either return an error or block/wait for a free
tag — so we never emit NoTag or spin indefinitely.
this will be moved to the 'crc setup' command as we should only enable file sharing and create the smb share for home dir when users have set the config option 'enable-shared-dirs' to 'true' this should remove at least of the warnings reported by virustotal for the MSI: https://www.virustotal.com/gui/file/31b402dcc1da24265074a21a26018d6cde8eef0b63c77a18f89eb079b6556790
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
pkg/crc/machine/start.go (1)
244-251: 9P mount: still non‑privileged, wrong argv, missing options/aname (tag)Mount needs root and proper 9P opts. Also split argv; passing
"chown core:core"and"9pfs 192.168.127.1"as single tokens will fail lookup. Use explicit mount with aname=mount.Tag and constants.Apply:
case "9p": - // change owner to core user to allow mounting to it as a non-root user - if _, _, err := sshRunner.RunPrivileged("Changing owner of mount directory", "chown core:core", mount.Target); err != nil { - return err - } - if _, _, err := sshRunner.Run("9pfs 192.168.127.1", mount.Target); err != nil { - return err - } + // change owner to core user to allow non-root access after mounting + if _, _, err := sshRunner.RunPrivileged("Changing owner of mount directory", "chown", "core:core", mount.Target); err != nil { + return err + } + opts := fmt.Sprintf("trans=tcp,port=%d,msize=%d,cache=loose,version=9p2000.L,aname=%s", + constants.Plan9TcpPort, constants.Plan9Msize, mount.Tag) + if _, _, err := sshRunner.RunPrivileged( + fmt.Sprintf("Mounting %s (9p tag %s)", mount.Target, mount.Tag), + "mount", "-t", "9p", "-o", opts, "192.168.127.1", mount.Target, + ); err != nil { + return err + }Optional follow-ups:
- Idempotency: skip if already mounted (e.g.,
mountpoint -q), or unmount/retry. I can propose a patch.
🧹 Nitpick comments (1)
pkg/fileserver/fs9p/server.go (1)
3-13: Ignore net.ErrClosed on shutdown in WaitForErrorClosing the listener causes proto.Serve to return net.ErrClosed; that shouldn’t surface as an error after Stop(). Handle it explicitly.
@@ -import ( +import ( + "errors" "fmt" "net" "os" "path/filepath" @@ -func (s *Server) WaitForError() error { - err := <-s.ErrChan - return err -} +func (s *Server) WaitForError() error { + if err, ok := <-s.ErrChan; ok { + if errors.Is(err, net.ErrClosed) { + return nil + } + return err + } + return nil +}Also applies to: 84-88
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
cmd/crc/cmd/daemon.go(5 hunks)cmd/crc/cmd/start.go(0 hunks)packaging/windows/product.wxs.template(0 hunks)pkg/crc/config/settings.go(1 hunks)pkg/crc/constants/constants.go(1 hunks)pkg/crc/machine/driver.go(0 hunks)pkg/crc/machine/driver_darwin.go(0 hunks)pkg/crc/machine/driver_linux.go(0 hunks)pkg/crc/machine/driver_windows.go(0 hunks)pkg/crc/machine/libhvee/driver_windows.go(2 hunks)pkg/crc/machine/start.go(2 hunks)pkg/drivers/libhvee/libhvee_windows.go(0 hunks)pkg/drivers/libhvee/powershell_windows.go(0 hunks)pkg/fileserver/fs9p/server.go(1 hunks)
💤 Files with no reviewable changes (8)
- pkg/crc/machine/driver_windows.go
- cmd/crc/cmd/start.go
- packaging/windows/product.wxs.template
- pkg/crc/machine/driver.go
- pkg/crc/machine/driver_darwin.go
- pkg/drivers/libhvee/powershell_windows.go
- pkg/crc/machine/driver_linux.go
- pkg/drivers/libhvee/libhvee_windows.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/crc/constants/constants.go
- pkg/crc/machine/libhvee/driver_windows.go
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: redbeam
PR: crc-org/crc#4866
File: pkg/fileserver/fs9p/shares.go:0-0
Timestamp: 2025-08-06T09:48:10.441Z
Learning: The vsock implementation in pkg/fileserver/fs9p/shares.go was temporarily removed because 9pfs doesn't fully support it yet. It will be added back when 9pfs provides proper vsock support.
🧬 Code graph analysis (4)
pkg/crc/machine/start.go (1)
pkg/os/exec.go (1)
RunPrivileged(48-59)
cmd/crc/cmd/daemon.go (3)
pkg/crc/constants/constants.go (2)
Plan9TcpPort(60-60)GetHomeDir(164-170)pkg/fileserver/fs9p/server.go (1)
New9pServer(29-53)pkg/crc/logging/logging.go (1)
Warnf(100-102)
pkg/crc/config/settings.go (2)
pkg/crc/config/validations.go (1)
ValidateBool(19-25)pkg/crc/config/callbacks.go (1)
SuccessfullyApplied(36-38)
pkg/fileserver/fs9p/server.go (3)
vendor/github.com/DeedleFake/p9/fs.go (2)
FileSystem(21-34)FSConnHandler(162-167)vendor/github.com/DeedleFake/p9/proto/server.go (1)
Serve(17-42)pkg/crc/constants/constants.go (1)
Plan9Msize(59-59)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: build (ubuntu-latest, 1.24)
- GitHub Check: build (macOS-14, 1.24)
- GitHub Check: build-qe (darwin, arm64)
- GitHub Check: build-qe (linux, amd64)
- GitHub Check: build (ubuntu-latest, 1.24)
- GitHub Check: build (ubuntu-22.04, 1.24)
- GitHub Check: build (macOS-13, 1.24)
- GitHub Check: Run OKD bundle with crc (1.24)
- GitHub Check: Konflux kflux-prd-rh02 / crc-binary-on-pull-request
🔇 Additional comments (2)
cmd/crc/cmd/daemon.go (2)
182-196: LGTM: use net.JoinHostPortSafer and clearer than manual fmt for host:port.
198-212: LGTM: host virtual IP listener uses net.JoinHostPortConsistent, correct.
| // 9p home directory sharing | ||
| if runtime.GOOS == "windows" { | ||
| listener9p, err := vn.Listen("tcp", net.JoinHostPort(configuration.GatewayIP, fmt.Sprintf("%d", constants.Plan9TcpPort))) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| server9p, err := fs9p.New9pServer(listener9p, constants.GetHomeDir()) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if err := server9p.Start(); err != nil { | ||
| return err | ||
| } | ||
| defer func() { | ||
| if err := server9p.Stop(); err != nil { | ||
| logging.Warnf("Error stopping 9p server: %v", err) | ||
| } | ||
| }() | ||
| go func() { | ||
| if err := server9p.WaitForError(); err != nil { | ||
| logging.Errorf("9p server error: %v", err) | ||
| } | ||
| }() | ||
| } |
There was a problem hiding this comment.
Respect EnableSharedDirs; don’t start 9P server when disabled
On Windows the 9P server starts even if EnableSharedDirs=false, still exposing the host home over TCP. Gate it on the config setting.
-// 9p home directory sharing
-if runtime.GOOS == "windows" {
+// 9p home directory sharing
+if runtime.GOOS == "windows" && config.Get(crcConfig.EnableSharedDirs).AsBool() {
listener9p, err := vn.Listen("tcp", net.JoinHostPort(configuration.GatewayIP, fmt.Sprintf("%d", constants.Plan9TcpPort)))
if err != nil {
return err
}
server9p, err := fs9p.New9pServer(listener9p, constants.GetHomeDir())
if err != nil {
return err
}
if err := server9p.Start(); err != nil {
return err
}
defer func() {
if err := server9p.Stop(); err != nil {
logging.Warnf("Error stopping 9p server: %v", err)
}
}()
go func() {
- if err := server9p.WaitForError(); err != nil {
+ if err := server9p.WaitForError(); err != nil {
logging.Errorf("9p server error: %v", err)
}
}()
}Optional: send server errors to errCh instead of only logging so the daemon exits on 9P failure.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // 9p home directory sharing | |
| if runtime.GOOS == "windows" { | |
| listener9p, err := vn.Listen("tcp", net.JoinHostPort(configuration.GatewayIP, fmt.Sprintf("%d", constants.Plan9TcpPort))) | |
| if err != nil { | |
| return err | |
| } | |
| server9p, err := fs9p.New9pServer(listener9p, constants.GetHomeDir()) | |
| if err != nil { | |
| return err | |
| } | |
| if err := server9p.Start(); err != nil { | |
| return err | |
| } | |
| defer func() { | |
| if err := server9p.Stop(); err != nil { | |
| logging.Warnf("Error stopping 9p server: %v", err) | |
| } | |
| }() | |
| go func() { | |
| if err := server9p.WaitForError(); err != nil { | |
| logging.Errorf("9p server error: %v", err) | |
| } | |
| }() | |
| } | |
| // 9p home directory sharing | |
| if runtime.GOOS == "windows" && config.Get(crcConfig.EnableSharedDirs).AsBool() { | |
| listener9p, err := vn.Listen("tcp", net.JoinHostPort(configuration.GatewayIP, fmt.Sprintf("%d", constants.Plan9TcpPort))) | |
| if err != nil { | |
| return err | |
| } | |
| server9p, err := fs9p.New9pServer(listener9p, constants.GetHomeDir()) | |
| if err != nil { | |
| return err | |
| } | |
| if err := server9p.Start(); err != nil { | |
| return err | |
| } | |
| defer func() { | |
| if err := server9p.Stop(); err != nil { | |
| logging.Warnf("Error stopping 9p server: %v", err) | |
| } | |
| }() | |
| go func() { | |
| if err := server9p.WaitForError(); err != nil { | |
| logging.Errorf("9p server error: %v", err) | |
| } | |
| }() | |
| } |
🤖 Prompt for AI Agents
In cmd/crc/cmd/daemon.go around lines 253 to 276, the 9P server is started
unconditionally on Windows which ignores the EnableSharedDirs setting; update
the logic to check the configuration flag (e.g., configuration.EnableSharedDirs
or the correct config field) and only create/listen/start the 9P server when
that flag is true, returning or skipping setup when false; keep the existing
defer/WaitForError handling but (optionally) forward server runtime errors into
the daemon errCh instead of only logging so the daemon can exit on 9P failure.
After the addition of 9p file sharing for Windows, we are removing SMB file sharing support (#4768).
|
@redbeam: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
4 participants
Fixes #4768
We are removing SMB file sharing support for Windows in favor of 9p. Some of the work has already been done by @anjannath, and it is included here (#4620).
Summary by CodeRabbit
New Features
Refactor
Chores