Skip to content

Add support of RemoveDirectoryOptions::RemoveReadOnlyDirectory which remove read-only directories#617

Open
dimhotepus wants to merge 4 commits intomicrosoft:masterfrom
dimhotepus:user/dzmitry/removedirectoryrecursive-readonly-directories
Open

Add support of RemoveDirectoryOptions::RemoveReadOnlyDirectory which remove read-only directories#617
dimhotepus wants to merge 4 commits intomicrosoft:masterfrom
dimhotepus:user/dzmitry/removedirectoryrecursive-readonly-directories

Conversation

@dimhotepus
Copy link
Copy Markdown
Contributor

@dimhotepus dimhotepus commented Feb 21, 2026

Closes #565


// Fail if the directory does not have read-only set, likely just an ACL problem
DWORD directoryAttr = ::GetFileAttributesW(path.get());
RETURN_LAST_ERROR_IF(!WI_IsFlagSet(directoryAttr, FILE_ATTRIBUTE_READONLY));
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Presumably GetLastError won't contain failure because you just called GetFileAttributesW right before this (which probably succeeded). And even if it did fail, you probably want the original error from the call to RemoveDirectoryW

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Followed the similar approach for file deletion:

RETURN_LAST_ERROR_IF(!WI_IsFlagSet(fileAttr, FILE_ATTRIBUTE_READONLY));

Comment on lines +395 to +398
if (directoryAttr == 0)
{
directoryAttr = FILE_ATTRIBUTE_NORMAL;
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this necessary? Presumably the attributes have FILE_ATTRIBUTE_DIRECTORY set at a minimum.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Followed similar approach as when file is deleted:

WI_ClearFlag(fileAttr, FILE_ATTRIBUTE_READONLY);

Set FILE_ATTRIBUTE_DIRECTORY instead, looks like SetFileAttributesW accepts it for directory.

@dunhor
Copy link
Copy Markdown
Member

dunhor commented Feb 24, 2026

@ChrisGuzak and @jonwis for their thoughts.

This jumps out as potentially concerning because it's changing behavior of something that an existing application could theoretically be depending on. The safest route would be to introduce a new flag - RemoveDirectoryOptions::RemoveReadOnlyDirectory - and possibly add a new RemoveDirectoryOptions::RemoveReadOnlyFile that aliases RemoveDirectoryOptions::RemoveReadOnly.

That said, looking around at existing uses of RemoveDirectoryOptions::RemoveReadOnly, there doesn't appear to be that many instances outside of test code, so this might just be okay.

@dunhor
Copy link
Copy Markdown
Member

dunhor commented Feb 24, 2026

The impact is presumably limited to just the directories themselves, so all of the directory contents would still get deleted, so the impact from, say, a security perspective, seems low.

@jonwis
Copy link
Copy Markdown
Member

jonwis commented Feb 25, 2026

Yeah, I like that additional flag for "... and your little readonly dirs, too!" mode. Thanks for the change.

@dimhotepus dimhotepus force-pushed the user/dzmitry/removedirectoryrecursive-readonly-directories branch from 90f6268 to 03136d5 Compare March 29, 2026 23:31
@dimhotepus dimhotepus changed the title RemoveDirectoryOptions::RemoveReadOnly should remove both read-only files and directories Add support of RemoveDirectoryOptions::RemoveReadOnlyDirectory which remove read-only directories Mar 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

RemoveDirectoryRecursive fails if directory has read-only attribute

3 participants