Skip to content
This repository was archived by the owner on Mar 4, 2025. It is now read-only.

Do not follow Reparse Points during deletion and copy #303

Merged
craigwi merged 4 commits into from
Mar 1, 2022
Merged

Do not follow Reparse Points during deletion and copy #303

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1bb4c90
Solves to problem of winfile following reparse point during deletion …
Feb 10, 2022
f22b500
Also skip one entry in the destination. Not needed during deletion, b…
Feb 15, 2022
3329aeb
Adapt to #297. This makes the situation much easier
Feb 24, 2022
e9cd9da
Don't follow Reparse Points by using OPER_RMDIR during delete
Feb 27, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 16 additions & 2 deletions src/wfcopy.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1442,6 +1442,13 @@ GetNextPair(PCOPYROOT pcr, LPTSTR pFrom,
if (pcr->bFastMove)
goto FastMoveSkipDir;
#endif
// Check if we should skip an entry because it was e.g. an reparse point
if (pDTA->fd.dwFileAttributes & ( ATTR_SYMBOLIC | ATTR_JUNCTION) ) {
RemoveLast(pcr->szDest);
dwOp = OPER_RMDIR;
goto ReturnPair;
}

pcr->cDepth++;
pDTA++;

Expand Down Expand Up @@ -1685,7 +1692,14 @@ GetNextPair(PCOPYROOT pcr, LPTSTR pFrom,
goto ReturnPair;
}

//
// Return reparse point and delete it via OPER_RMDIR
if (dwFunc == FUNC_DELETE && pDTA->fd.dwFileAttributes & (ATTR_SYMBOLIC | ATTR_JUNCTION)) {
Copy link
Copy Markdown
Contributor

@craigwi craigwi Feb 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Skip" doesn't seem like the right point; we are returning the reparse point as a directory, but not recursing.

Copy link
Copy Markdown
Contributor Author

@schinagl schinagl Feb 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A better comment would be
'Return reparse point and delete it via OPER_RMDIR'

pcr->fRecurse = FALSE;
dwOp = OPER_RMDIR;
goto ReturnPair;
}

//
// Directory: operation is recursive.
//
pcr->fRecurse = TRUE;
Expand Down Expand Up @@ -2723,7 +2737,7 @@ WFMoveCopyDriverThread(LPVOID lpParameter)
goto CancelWholeOperation;
}
#endif
break;
break;

Copy link
Copy Markdown
Contributor

@craigwi craigwi Feb 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this change (to unlink) not in the case 'OPER_RMDIR | FUNC_DELETE' as indicated by the comment above this code?

Copy link
Copy Markdown
Contributor Author

@schinagl schinagl Feb 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Short answer: Because when it recursivley runs it only comes along OPER_MKDIR | FUNC_DELETE, but not OPER_RMDIR | FUNC_DELETE. (which I agree, sounds weird)

Long answer: From trying to understand the code, I guess it all started with a 'recursive runner', which was meant to perform a copy of a complete tree. This involves OPER_MKDIR on the destination.
Later on the code was extended with recursive delete, and the 'recursive runnner' already available for copy was 'abused' to delete recursivley.
That's why it comes along here during recursive delete, but not with OPER_RMDIR.

OPER_RMDIR is only used for non-recursive directory deletion.... I assume.

Copy link
Copy Markdown
Contributor

@malxau malxau Feb 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fwiw, the first time I looked at this block I had to run it under the debugger to understand it too.

Although the labels OPER_MKDIR and OPER_RMDIR are really misleading, for delete I think the first one means "confirm the action" and the second means "do the action." When recursing, confirmation happens before recursion, but actual deletion has to happen afterwards (since child objects need to be removed before the parent can be.) For link deletion, it doesn't matter, since there's no recursion.

I've been playing around with this code and I think if the first section is changed from

goto SkipThisFile;

to

dwOp = OPER_RMDIR;
goto ReturnPair;

then this block can be removed. But I've only tried the most trivial test cases, so it's possible I'm missing something.

Also, I've been trying to find a case where this matters. I think it matters with a non-fast move (across volumes), where the OPER_MKDIR phase creates the target, and the OPER_RMDIR phase deletes the source. I think the code in this PR won't handle that case, since it creates the target, leaves the source, then fails to remove the parent.

Copy link
Copy Markdown
Contributor Author

@schinagl schinagl Feb 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thx, this makes the code more lean.
I would have never guessed that OPER_RMDIR is used during 'SLOW_MOVE'. I only saw that during crawling down OPER_MKDIR was used.

One more thing came up, when I retested copying trees with Symbolic links: As long as one has SeCreateSymbolicLinkPrivilege CreateDirectoryEx works fine and it 'copies' symbolic links.
(Un)fortunatley I do have this enabled due to LinkShellExtension development on my home PC, but ... tada.. not on my notebook. Anyhow I upgraded MKDir to create a symbolic link in developer mode.

@malxau : Is there a non documented way that CreateDirectoryEx can be enabled to respect SYMBOLIC_LINK_FLAG_ALLOW_UNPRIVILEGED_CREATE? If not I hope my solution is sufficient.

Copy link
Copy Markdown
Contributor

@malxau malxau Feb 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ouch, that hurts. No, I don't think there's a way to make CreateDirectoryEx work in this case. Traditionally (without SYMBOLIC_LINK_FLAG_ALLOW_UNPRIVILEGED_CREATE), kernel32/kernelbase would attempt to enable SeCreateSymbolicLinkPrivilege and if that fails, fail the operation. With SYMBOLIC_LINK_FLAG_ALLOW_UNPRIVILEGED_CREATE, it continues if SeCreateSymbolicLinkPrivilege cannot be enabled, effectively letting kernel decide how to proceed. It looks like both CreateDirectoryEx and the CopyFile family are both capable of creating symbolic links but weren't updated to continue if the privilege can't be enabled, and both of these do tons of processing in usermode, so it's not possible to get them to perform their task without this privilege (well, without hooking RtlAcquirePrivilege and altering its return code.)

case IDNO:
case IDCANCEL:
Expand Down