Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Address or document racy nature of Windows unlink() emulation #16

Open
rcombs opened this issue Dec 29, 2018 · 1 comment
Open

Address or document racy nature of Windows unlink() emulation #16

rcombs opened this issue Dec 29, 2018 · 1 comment

Comments

@rcombs
Copy link

rcombs commented Dec 29, 2018

Context:

result<void> out = relink(dirh, randomname);
if(!out)
{
// If something else is using it, we may not be able to rename
// This error also annoyingly appears if the file has delete on close set on it already
if(out.error().value() == static_cast<int>(0xC0000043) /*STATUS_SHARING_VIOLATION*/)
{
LLFIO_LOG_WARN(this, "Failed to rename entry to random name to simulate immediate unlinking due to STATUS_SHARING_VIOLATION, skipping");
}
else
{
return std::move(out).error();
}
}
}
// No point marking it for deletion if it's already been so
if(!(h.flags() & flag::unlink_on_first_close))
{
// Hide the item in Explorer and the command line
{
IO_STATUS_BLOCK isb = make_iostatus();
FILE_BASIC_INFORMATION fbi{};
memset(&fbi, 0, sizeof(fbi));
fbi.FileAttributes = FILE_ATTRIBUTE_HIDDEN;
NTSTATUS ntstat = NtSetInformationFile(h.native_handle().h, &isb, &fbi, sizeof(fbi), FileBasicInformation);
if(STATUS_PENDING == ntstat)
{
ntstat = ntwait(h.native_handle().h, isb, d);
}
(void) ntstat;
}
// Mark the item as delete on close
IO_STATUS_BLOCK isb = make_iostatus();
FILE_DISPOSITION_INFORMATION fdi{};
memset(&fdi, 0, sizeof(fdi));
fdi._DeleteFile = 1u;
NTSTATUS ntstat = NtSetInformationFile(duph, &isb, &fdi, sizeof(fdi), FileDispositionInformation);

When the win_disable_unlink_emulation flag is not set, on Windows prior to Windows 10 1709, llfio will handle an unlink() via 3 separate syscalls: one to rename the file to something random, one to mark the file as hidden, and one to mark it as deleted.

This results in two windows during which an application or system crash could result in inconsistent (and potentially very harmful) results. If the crash occurs between the rename and the mark-as-hidden, then the user could possibly see the file and delete it later (though they're less likely to if it's in some obscure application-data directory), but if it occurs between the mark-as-hidden and the delete, it may be fairly difficult to remove the file, which could be problematic, especially if it's large.

Personally, I don't think the mark-as-hidden is a good choice here, particularly since the randomized name is briefly visible anyway. I'm not sure if the period between the rename and the mark-as-deleted is possible to address short of not making unlink emulation the default, but if it's not going to be addressed, then the documentation should indicate that this is a possible failure mode, and how developers should attempt to recover from it if possible (i.e. by searching application-data directories where they regularly delete files for entries with .deleted extensions and removing them).

@ned14
Copy link
Owner

ned14 commented Dec 29, 2018

Thing is, I didn't used to mark as hidden before setting delete on close, and then I got complaints about files named hex.deleted being all over the place. Another alternative is to rename files into a temporary directory before setting delete on close, but there is no guarantee that a suitable temporary directory exists on the same volume.

None of the options available here are good. I think the current choice is the least worst on older windows given the competing factors. The current behavior is already documented in the flags, but I think you're right it's not documented in the unlink API docs currently. So I'll leave this open to remind me to fix that when I get back to Europe next week. Thanks for reporting this issue!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants