bugGNU Octave - Bugs: bug #63265, rmdir fails to remove directory...

 
 

bug #63265: rmdir fails to remove directory containing read-only files

Submitter:  None
Submitted:  Mon 24 Oct 2022 11:22:52 AM UTC
 
Category:  Octave Function Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Unexpected Error or Warning
Status:  Fixed Assigned to:  None
Originator Name:  Originator Email:  -email is unavailable-
Open/Closed:  * Closed Release:  * dev
Operating System:  * Microsoft Windows Fixed Release:  8.1.0 (current stable)
Planned Release:  8.1.0 (current stable)
* Mandatory Fields

Add a New Comment (Rich Markup)
   

Comment Type & Canned Response:
       

 

( Jump to the original submission )

Mon 28 Nov 2022 06:07:25 PM UTC, comment #34: 

I see, thank you for your time.

Anonymous
Sat 26 Nov 2022 05:07:52 PM UTC, comment #33: 

If you are wondering why

  warning('off','MATLAB:rmpath:DirNotFound');

doesn't suppress warnings in Octave, it's because Octave doesn't currently handle warning IDs that begin with "MATLAB:".

John W. Eaton <jwe>
Project Administrator
Sat 26 Nov 2022 04:30:52 PM UTC, comment #32: 

Thank you for checking.

As far as this bug report goes, I'm going to close it as fixed.

If you have further questions regarding how to suppress warnings in Octave, the discourse forum might be the right place for this:
https://octave.discourse.group/

I'm not sure which minor software release you are referring to. But to avoid confusion by anyone reading this: There won't be a next minor release of Octave 7. The next released version of Octave will be the (major) release Octave 8.1.

Markus Mützel <mmuetzel>
Project Member
Fri 25 Nov 2022 01:39:36 AM UTC, comment #31: 

Oh, finally. I had scavenged everywhere for a nightly, only to eventually find out they just built stable.

Anyhow, as far as this bug is concerned removal of old PTB versions works fine now (ironically enough it's basically the only functionality not broken atm.. but that's on them to fix).

If you have still some time left though, I'd exhort you to check out why this process is letting warnings to print when it shouldn't
https://github.com/Psychtoolbox-3/Psychtoolbox-3/blob/3.0.18.13/Psychtoolbox/DownloadPsychtoolbox.m#L626-L628

This problem has been fixed in the stable version. The fix will
be available in the next minor software release. Thank you for
your bug report.

Anonymous
Sat 19 Nov 2022 01:22:59 PM UTC, comment #30: 

@Anonymous (original reporter): Octave 8 has moved to the stable branch recently and is now available as nightly builds here:
https://buildbot.octave.space/#/download

Could you please check and report if the issue is solved for you?

Markus Mützel <mmuetzel>
Project Member
Tue 01 Nov 2022 08:51:33 AM UTC, comment #29: 

@jwe: It might be that gnulib would have handled that for us. Alas, like you already mentioned, we can't use their replacement function if we'd like to support files with any Unicode characters. Having to re-implement some of gnulib's fixes one way or the other is probably a price we have to pay (because their file system functions only support ANSI characters on Windows).

I added a note to the NEWS file and pushed the patch from comment #10 here:
https://hg.savannah.gnu.org/hgweb/octave/rev/980059c3b129

Marking as ready for test.

Markus Mützel <mmuetzel>
Project Member
Mon 31 Oct 2022 08:05:14 PM UTC, comment #28: 

RE: comment #25:  Is this something that gnulib should be handling for us?  Or is gnulib already not doing what we need here because of unicode characters?

John W. Eaton <jwe>
Project Administrator
Mon 31 Oct 2022 07:50:28 PM UTC, comment #27: 

AFAICT microsoft could as well be using remove instead.

And I'm a real noob, but given my dumb test with matlab's delete FWIW I suspect they might be unsetting/resetting the files attributes in any case without extra checks (not that I can think to many/some situation where this would matter, but nonetheless).

Anonymous
Mon 31 Oct 2022 05:27:07 PM UTC, comment #26: 

Being the one who warned to be careful in comment #11, I'd say I'm entitled to conclude that sufficient care and thought has been applied so I'd second comment #25.
But IMO this change should be outlined clearly in the NEWS.

Philip Nienhuis <philipnienhuis>
Project Member
Mon 31 Oct 2022 10:16:34 AM UTC, comment #25: 

Given the recent information, I'm inclined to prefer the patch in comment #10 over the alternatives that we discussed.

On the one hand, Matlab doesn't seem to care about read-only file attributes either IIUC. On the other hand, `unlink` on Windows currently behaves differently than on Linux (and other platforms).
What gnulib does in those cases is to add wrappers that align the behavior of a function on the "deviating" platform (Windows) to the documented behavior (POSIX in this case). The patch in comment #10 essentially does that.

As an added bonus: That patch would be less intrusive than the discussed alternatives and easier to maintain.

Markus Mützel <mmuetzel>
Project Member
Sun 30 Oct 2022 01:00:28 PM UTC, comment #24: 

As I already said, Windows own rmdir will remove folders with r/o files (while delete on the individual files will error out).
That's why I was conceivably assuming that some function existed that handled this (and why matlab doing otherwise gets up my nose now)
The oldest documentation I could find isn't as relatively clear as the new one, but I timidly believe this isn't really a new behaviour.
https://web.archive.org/web/20020911071017/http://msdn.microsoft.com/library/en-us/fileio/base/createfile.asp
https://web.archive.org/web/20021027021545/http://www.msdn.microsoft.com/library/en-us/fileio/base/file_security_and_access_rights.asp

And of course take care means to delete indiscriminately if I'm talking about steamrolling everything.

Anonymous
Sun 30 Oct 2022 11:01:44 AM UTC, comment #23: 

What does "take care" in this context mean? Does it delete files even if the read-only attribute is set? Or does it "respect" the read-only attribute and error out when trying to delete such files?

Markus Mützel <mmuetzel>
Project Member
Sun 30 Oct 2022 02:41:29 AM UTC, comment #22: 

I'm curious, separate from what matlab's rmdir does, what does windows' own rmdir do in CMD?  is there any change between windows versions?

Nicholas Jankowski <nrjank>
Project Member
Sun 30 Oct 2022 12:06:20 AM UTC, comment #21: 

I just did a test with R2022b, and indeed "delete" seems to take care of individual read-only files too.
The calls as seen by the usual procmon don't even seem to differ between the normal and r/o cases (suggesting they might not even be doing any particular check, they just steamroll over everything and anything).
Not really a fan of this tbh, even though this is probably what they also use for rmdir.

TIL about unlink.
According to stackoverflow and msdn SHFileOperation could get you a very short and simple way to recursively delete non-empty folders and their content (but maybe that's too high level for your tastes, and alas maximum matlab compatibility would still require other adjustments anyway).

Anonymous
Sat 29 Oct 2022 07:59:02 PM UTC, comment #20: 

Just out of curiosity, what does Matlab's delete function do for files that have read-only attributes set?  If it is also ignores read-only file attributes, then we could make Octave's scripting language delete and rmdir functions compatible and leave unlink alone (both the internal C++ wrapper around the unlink system call and the Octave scripting language function) since users of unlink might expect its behavior to match the system call.

John W. Eaton <jwe>
Project Administrator
Sat 29 Oct 2022 07:46:59 PM UTC, comment #19: 

That is correct.

The patch in comment #10 modifies the `unlink` call on Windows to be more like the POSIX function (by removing that non-POSIX file attribute if applicable). But there were remarks that doing that might be risky.

Markus Mützel <mmuetzel>
Project Member
Sat 29 Oct 2022 07:41:26 PM UTC, comment #18: 

The unlink system call will remove read-only files, at least when "read-only" has the traditional Unix/POSIX meaning and the directory containing them is writable.

The problem with unlink not removing files that have some other non-POSIX "attributes" attached to it appears to be the problem here, correct?

John W. Eaton <jwe>
Project Administrator
Sat 29 Oct 2022 05:15:46 PM UTC, comment #17: 

A directory cannot be removed unless it is empty (on no platform I know of). See, e.g, [1] for the documentation of the system function `rmdir` in Windows.

Matlab's `rmdir` with the 's' parameter removes directories even if they are not empty. To be compatible, we need to recursively delete any files or folders inside the directory before deleting it.
How that is done is an implementation detail, and a user shouldn't need to worry about that. We are using the system function "unlink" because that works on "normal files", hard links and symbolic links. It has also the advantage that this function exists cross-platform.

We already established that we need to change the Octave function `rmdir` with the 's' parameter to also remove any read-only files to be Matlab-compatible. How that will be done, will also be an implementation detail. One way that would allow us to continue using the system function "unlink" is to remove the read-only attribute if necessary so that "unlink" can delete the file.

I'm not sure which other function you are hinting at. If you have any concrete pointers, please let us know.

[1]: https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/rmdir-wrmdir?view=msvc-170

Markus Mützel <mmuetzel>
Project Member
Sat 29 Oct 2022 03:27:59 PM UTC, comment #16: 

I'm not exactly sure what "unlink" has to do with anything here, anyway I'd just like to underline again the very fine nuance here.

The purported problem is not "the read only flag would mean that you can delete a file, just not edit it".
https://learn.microsoft.com/en-us/windows/win32/fileio/file-attribute-constants
It is "you cannot touch a read only file, but but you can do whatever you want (including full nuking) to its root folder".
Any generic function not being able to delete the individual r/o files seems correct behaviour.

I'll grant I don't know how matlab handles this then (they seem to query slightly different FileAttributes than Windows's own rmdir) but it doesn't seem so obvious that therefore they have to be taking care of unsetting flags themselves. Surely there's an OS api that is going to comfortably take care of this?

And I didn't really feel deceived by the error message (even though I'd appreciate if you could differentiate the totally unrelated ERROR_SHARING_VIOLATION case). It would totally make sense *if* I was trying to delete protected single files. Rmdir entails quite a different concept though.

Anonymous
Thu 27 Oct 2022 07:24:26 AM UTC, comment #15: 

In any case, either option of changing the prototype of octave::sys::unlink (to pass an optional flag to signal if read-only files should also be unlinked) or adding a new function for this particular purpose would be an API change.
Setting release to dev because of that.

Markus Mützel <mmuetzel>
Project Member
Thu 27 Oct 2022 05:57:55 AM UTC, comment #14: 

`unlink` is an Octave-only function. So, there is no Matlab-compatibility concern any way.
I'd guess that most functions are written with the behavior of the POSIX function in mind that doesn't know of a read-only file attribute. So, I don't know if there are many (or any?) places where we (intentionally) rely on the current behavior of `unlink` on Windows.

I agree that it might be a lower risk to just change the function(s) that are affected with compatibility issues. I was only wondering if that justifies the additional complexity.

Both approaches are fine with me.
I understand that the consensus is that we should add a version of `unlink` that ignores the read-only file attribute on Windows.

Markus Mützel <mmuetzel>
Project Member
Wed 26 Oct 2022 09:17:37 PM UTC, comment #13: 

On Unix systems deleting a file requires modifying the directory, not the file itself so it is not the readability of the file that matters, only whether there is write permission for the directory containing the file.

I agree with Philip that we shouldn't change the behavior of unlink.  Fixing the Matlab compatibility of rmdir on Windows seems reasonable if someone wants to work on that.

John W. Eaton <jwe>
Project Administrator
Wed 26 Oct 2022 09:00:46 PM UTC, comment #12: 

looking at it when considering installing and testing PsychToolbox, it requires SVN to be installed i believe to support its self managed installation/versioning. The need to go through installing SVN to hunt this bug down is partly why i stopped short of doing so.  But, that's really a question only the package developer can answer.

Nicholas Jankowski <nrjank>
Project Member
Wed 26 Oct 2022 08:45:33 PM UTC, comment #11: 

> Any opinions?  (on changing unlink's behavior)

PMFJI
I suppose there's quite a bit of Octave m-file code around that explicitly or implicitly (= invoked by other functions) relies on the current behavior of 'unlink' - i.e., that it won't delete read-only files. So be careful changing that.

Rather than change unlink's default behavior, we can change rmdir to be able to convey a flag to unlink to instruct it to also delete read-only files. After all it is rmdir that seems Matlab-incompatible, not unlink.

From the discussion it looks like the OP (and some commenters here) were deceived by rmdir's maybe too general error message "Permission denied" - maybe rmdir should more precisely tell that the files-to-be-deleted have a read-only attribute set; to be distinguished form "file in use" or so. Or rather tell exactly which file(s) couldn't be deleted (or just the first one to avoid a long printout) - that may help avoiding a lot of effort trying to recreate the issue.

Also, IIUC the installed PsychToolbox has an svn folder inside; why is that needed in the first place? I can't imagine it is used by the PsychToolbox code.

Philip Nienhuis <philipnienhuis>
Project Member
Wed 26 Oct 2022 07:09:51 PM UTC, comment #10: 

It looks like `mkdir` fails in this case because `unlink` fails to unlink the read-only file.

Reading the POSIX documentation of "unlink", it is not entirely clear to me what is supposed to happen in this case. Afaict, there is no one-to-one equivalent to a read-only file attribute on POSIX:
https://man7.org/linux/man-pages/man2/unlink.2.html

The documentation on MSDN is pretty clear that the Microsoft implementation of the function with a similar name is supposed to fail in this case:
https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/unlink-wunlink?view=msvc-170

> Each of these functions returns 0 if successful. Otherwise, the function returns -1 and sets errno to EACCES, which means the path specifies a read-only file or a directory, or to ENOENT, which means the file or path isn't found.


In any case, Matlab's `rmdir` doesn't seem to care about the read-only flag. So, this is at least a compatibility issue for that function.

The attached patch modifies `unlink` to also delete files that have their read-only attribute set. That also allow `rmdir` to remove directories that contain such files.

Alternatively, we could keep the current version of `unlink` that errors when trying to delete files with read-only attribute and add a new version that ignores the read-only attribute.

Any opinions?

(file #53909)

Markus Mützel <mmuetzel>
Project Member
Wed 26 Oct 2022 03:05:50 PM UTC, comment #9: 

Thanks. I can reproduce with that information.

Simpler steps to reproduce:

mkdir('test_dir');
system('touch ./test_dir/some_file');
system('attrib +r ./test_dir/some_file');
rmdir('./test_dir', 's')

Markus Mützel <mmuetzel>
Project Member
Wed 26 Oct 2022 02:22:47 PM UTC, comment #8: 

I can expect you to run the install script for a widely famous toolset...

But anyway, the thing here is just that rmdir fails for some reason.
It is not the script (since matlab has no problem), and it is not permissions (even if run octave as administrator I'm still getting this). What remains?
It's not even the file browser since that's opened in another folder (and yes, even if I close it nothing changes).

Counterintuitively me talking about windows calls is probably the most sloppy observation in this bug report...
Anyway, after further digging into that I found out the files in the svn folder are marked as read only.

Now then you are going to tell me, no shit I'm getting an error. But here's the crazy discovery I just made: removing a folder with r/o files inside is not the same thing of trying to delete the individual files (at least in Windows, you can see this difference even in cmd between "rmdir" and "del").

I cannot really claim that I totally understand how your sys::rmdir works then, but indeed I believe you are using the second approach rather than the former (that may also explain all those "rmpath: not found" warnings from line 627 that I get before the delete prompt).

Anonymous
Wed 26 Oct 2022 11:01:12 AM UTC, comment #7: 

You can't expect us to just run some file with a couple of hundred lines that might delete some files. You can tell us that it is save. But how do we know it really is?

Anyway. Those QueryDirectory operations might be:
https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/ntifs/nf-ntifs-ntquerydirectoryfile

IIUC, they need an open file handle. Those might very well be the reason why the directory can't be deleted.

I'm not exactly sure what is the reason for those QueryDirectory calls. But they might be triggered for updates of the file browser in Octave's GUI.
Do you still see the error if you close the file browser widget before running your function?

Markus Mützel <mmuetzel>
Project Member
Wed 26 Oct 2022 10:40:21 AM UTC, comment #6: 

A "different Windows setting" that I know irks many developers is ransomware protection.

Anyway, back to my problem.. darn isn't there a lot of misunderstandings.
I just mentioned my full context in the OP for the records. It was the reason I tried to run again the function, but I wasn't trying to imply the network error caused the issue (I mean, I didn't know to be honest, but now that I checked a totally smooth installation the error is still the same).

I get that STATUS_CANNOT_DELETE seems quite telling for *something else* to be causing troubles, but after monitoring the file and the rest of the folder tree ever since the Octave start.. there's nothing else (except perhaps some presumably innocent QueryDirectory operation) trying to access them.

Anonymous
Tue 25 Oct 2022 03:10:52 PM UTC, comment #5: 

just confirming as i read through the install function - it does a test folder creation and removal around line 561 and would have created an error sooner if it was an actual security permission issue. so i don't think that was the case here.

i don't suppose you saved any of the error messages during the failed install to see where that might have left off?

Nicholas Jankowski <nrjank>
Project Member
Tue 25 Oct 2022 03:04:56 PM UTC, comment #4: 

i suspect there weren't any odd permissions. there was probably just a 'still reported as in use' file that wasn't allowed to be deleted.  it's hard to tell what operation might have created such a situation however without being able to recreate exactly what happened during the 'failed' download.

Also, it's not necessarily true that there's "absolutely not a permission problem" inside your user folder. Windows security models vary greatly, and many do restrict certain types of operations within certain user folder locations. E.g., we have several bugs and discussions related to the inability to compile and install packages using the default location of the temp foldur under the user's profile, because different security software or windows settings restrict execution and compilation within the user temp space.

That said, while it could be something complex like that, I suspect the issue is just a non-released file preventing deletion. If the situation could be repeated we could maybe find out if it's an octave specific problem or not.

Nicholas Jankowski <nrjank>
Project Member
Tue 25 Oct 2022 02:52:14 PM UTC, comment #3: 

Yes, tinkering also included manually removing that folder with windows explorer (and indeed I don't see any particular reason why .svn should have odd permissions).

The script is just supposed to remove pre-existent versions of the folder it installs when it finds it.
It shouldn't really be all that hard for you to run (except you need to install sliksvn or something like that):

DownloadPsychtoolbox("C:/Users/User/Desktop/test")

Anonymous
Mon 24 Oct 2022 02:18:10 PM UTC, comment #2: 

do you know exactly what the script that you ran is trying to do?  Is it trying to use the pkg function to add/remove things? a Permission Denied error like that usually means Windows thinks some program is still using a file in that directory.  I've run into this on occasion with removing packages when for whatever reason I have to close/reopen octave for a 'pkg uninstall' to actually delete the folders. 

Nicholas Jankowski <nrjank>
Project Member
Mon 24 Oct 2022 11:57:11 AM UTC, comment #1: 

Are you able to manually delete that folder (e.g., using the Windows Explorer)?
Do other running programs keep any of those files or folders open? (You don't have permission to delete files or folders if there are still open handles to them.)

If you can't find which programs could be the culprit, try re-starting the computer. Does that make a difference?

Markus Mützel <mmuetzel>
Project Member
Mon 24 Oct 2022 11:22:52 AM UTC, original submission:  

https://github.com/Psychtoolbox-3/Psychtoolbox-3/blob/3.0.18.13/Psychtoolbox/DownloadPsychtoolbox.m

I was trying to install PTB-3, and due to whatever network error the download failed.
No biggie, and I tried to run the script again.. except I got this error.

Shall I delete the old Psychtoolbox folder and all its contents \n(recommended in most cases), (yes or no)? yes
Now we delete "Psychtoolbox" itself.
remove entire contents of C:\Users\xxxxxxxx\Downloads\octave-7.2.0-w64\psych\Psychtoolbox? (yes or no) yes
Error in RMDIR: Permission denied
If you want, you can delete the Psychtoolbox folder manually and rerun this script to recover.
error: rmdir
error: called from
    DownloadPsychtoolbox at line 647 column 13

It's absolutely not a permission problem since I'm just tinkering inside my own user folder.
And I tried with both slashes and (escaped) backslashes, if it could mean anything.

Anonymous

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #53903:  procmon.png added by None (213KiB - image/png - Process monitor events)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by nrjank (Posted a comment)
  • -email is unavailable- added by mmuetzel (Posted a comment)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only project members can vote.

     

     

     

     

    Follow 14 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2022-11-26 mmuetzel StatusReady For Test Fixed
        Open/ClosedOpen Closed
        Fixed ReleaseNone 8.1.0 (current stable)
        Planned ReleaseNone 8.1.0 (current stable)
    2022-11-01 mmuetzel StatusPatch Submitted Ready For Test
    2022-10-27 mmuetzel Release7.2.0 dev
    2022-10-26 mmuetzel Attached File- Added bug63265-unlink-read-only-files.patch, #53909
        StatusConfirmed Patch Submitted
    2022-10-26 mmuetzel CategoryNone Octave Function
        Item GroupNone Unexpected Error or Warning
        StatusNeed Info Confirmed
        SummaryRmdir fails to delete svn folder rmdir fails to remove directory containing read-only files
    2022-10-24 mmuetzel StatusNone Need Info
    2022-10-24 None Attached File- Added procmon.png, #53903

    Back to the top


    Powered by Savane 3.9