Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates virtiofs mount-source resolution so Linux can map a virtiofs tag to its Windows source path without making a query call back into the Windows service, enabling future work to represent drvfs/plan9/virtiofs shares as symlinks for Windows traversal.
Changes:
- Remove the
LxInitMessageQueryVirtioFsDeviceIPC message and its Windows service handler. - Persist a tag→source mapping on the Linux side via symlinks under
/run/wsl/virtiofsduring mount/remount. - Update
QueryVirtiofsMountSourceto resolve mount sources by reading the persisted symlink instead of calling the service.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/windows/service/exe/WslCoreVm.cpp | Removes handling for the virtiofs “query source by tag” message. |
| src/shared/inc/lxinitshared.h | Removes the query message type and its message struct from the shared IPC contract. |
| src/linux/init/drvfs.cpp | Adds symlink-based persistence of virtiofs tag→source mapping and uses it to resolve mount sources locally. |
You can also share your feedback on Copilot code review. Take the survey.
| std::string CanonicalSource{Source}; | ||
| UtilCanonicalisePathSeparator(CanonicalSource, PATH_SEP_NT); | ||
|
|
||
| UtilMkdirPath(VIRTIOFS_TAG_DIR, 0755); | ||
|
|
||
| auto LinkPath = std::format("{}/{}", VIRTIOFS_TAG_DIR, Tag); | ||
|
|
||
| // | ||
| // Remove any existing symlink for this tag before creating a new one. | ||
| // | ||
|
|
||
| unlink(LinkPath.c_str()); | ||
| if (symlink(CanonicalSource.c_str(), LinkPath.c_str()) < 0) | ||
| { |
There was a problem hiding this comment.
SaveVirtiofsTagMapping uses Tag to build a filesystem path and then calls unlink() on it, but it never validates that Tag is a GUID/safe filename. If Tag ever contains '/' or '..' (e.g., due to a malformed/malicious host response), this can lead to path traversal and unintended file deletion outside /run/wsl/virtiofs. Validate Tag (same GUID check as QueryVirtiofsMountSource) before constructing LinkPath, and refuse to write mappings for invalid tags.
There was a problem hiding this comment.
I think that would be a good idea, at least we could fail if we see a / or a . in the target
There was a problem hiding this comment.
Since we expect a GUID, I will just check for that
There was a problem hiding this comment.
This feedback is stale, I already ensure the tag is a guid:
//
// Validate the tag is a GUID to prevent path traversal.
//
const auto Guid = wsl::shared::string::ToGuid(Tag);
if (!Guid)
{
LOG_WARNING("Invalid virtiofs tag {}", Tag);
return;
}
d63b77c to
b05c97f
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
src/windows/service/exe/WslCoreVm.cpp:2634
- The service no longer handles the virtiofs query message. If an older guest init sends the legacy query (e.g., after host update but before guest/init update), this will now throw and tear down the request. To preserve mixed-version compatibility, consider keeping a handler that responds (even if implemented via the new mapping logic or by returning an explicit error code) rather than throwing on an unexpected MessageType.
else
{
THROW_HR_MSG(E_UNEXPECTED, "Unexpected MessageType %d", message->MessageType);
}
You can also share your feedback on Copilot code review. Take the survey.
b05c97f to
74f3165
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
You can also share your feedback on Copilot code review. Take the survey.
43fbefe to
8c104c7
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
src/windows/service/exe/WslCoreVm.cpp:2634
- Dropping handling for the legacy virtiofs query message means an older guest sending
LxInitMessageQueryVirtioFsDevicewill now hit the "Unexpected MessageType" path and fail the request. If backwards compatibility is required, keep a handler for this message type (even if new guests no longer use it) and respond using existing in-memory share state (e.g.,FindVirtioFsShare).
else
{
THROW_HR_MSG(E_UNEXPECTED, "Unexpected MessageType %d", message->MessageType);
}
You can also share your feedback on Copilot code review. Take the survey.
OneBlue
left a comment
There was a problem hiding this comment.
LGTM. I think I would have a preference for the mappings to be stored in a place where the users can't write (like a map in init), but that would be a bigger change so I think this OK
8c104c7 to
e586308
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
src/windows/service/exe/WslCoreVm.cpp:2634
VirtioFsWorkerno longer handlesLxInitMessageQueryVirtioFsDeviceand will now treat it as an unexpected message type. If an older guest/init (or any out-of-tree component) still sends this query, requests will start failing withE_UNEXPECTED. Consider keeping the handler for backward compatibility (even if new guests no longer use it), or at least returning a structured error response instead of throwing.
else if (message->MessageType == LxInitMessageRemountVirtioFsDevice)
{
std::wstring newTag;
const auto result = wil::ResultFromException([this, span, &newTag]() {
const auto* remountShare = gslhelpers::try_get_struct<LX_INIT_REMOUNT_VIRTIOFS_SHARE_MESSAGE>(span);
THROW_HR_IF(E_UNEXPECTED, !remountShare);
const std::string tag = wsl::shared::string::FromSpan(span, remountShare->TagOffset);
const auto tagWide = wsl::shared::string::MultiByteToWide(tag);
auto guestDeviceLock = m_guestDeviceLock.lock_exclusive();
const auto foundShare = FindVirtioFsShare(tagWide.c_str(), !remountShare->Admin);
THROW_HR_IF_MSG(E_UNEXPECTED, !foundShare.has_value(), "Unknown tag %ls", tagWide.c_str());
newTag = AddVirtioFsShare(remountShare->Admin, foundShare->Path.c_str(), foundShare->OptionsString().c_str());
});
respondWithTag(newTag, result);
}
else
{
THROW_HR_MSG(E_UNEXPECTED, "Unexpected MessageType %d", message->MessageType);
}
You can also share your feedback on Copilot code review. Take the survey.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
You can also share your feedback on Copilot code review. Take the survey.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
You can also share your feedback on Copilot code review. Take the survey.
OneBlue
left a comment
There was a problem hiding this comment.
LGTM. One minor comment
| std::string CanonicalSource{Source}; | ||
| UtilCanonicalisePathSeparator(CanonicalSource, PATH_SEP_NT); | ||
|
|
||
| UtilMkdirPath(VIRTIOFS_TAG_DIR, 0755); | ||
|
|
||
| auto LinkPath = std::format("{}/{}", VIRTIOFS_TAG_DIR, Tag); | ||
|
|
||
| // | ||
| // Remove any existing symlink for this tag before creating a new one. | ||
| // | ||
|
|
||
| unlink(LinkPath.c_str()); | ||
| if (symlink(CanonicalSource.c_str(), LinkPath.c_str()) < 0) | ||
| { |
There was a problem hiding this comment.
I think that would be a good idea, at least we could fail if we see a / or a . in the target
This avoids a call back to the service and will make it easier to do virtiofs to windows directory mapping. There is an upcoming change to the Linux plan9 server that will use this information to present drvfs / plan9 / virtiofs shares as symlinks so the Windows client can traverse them (current we return access denied in this case).