Description Mikael Sterner 2020-05-29 02:50:39 EDT

Created attachment 283070 [details]
Patch that reintroduces old test as fallback

The fix for bug 558612 seems to have caused a regression when the configuration is in a location which is writable but for which access cannot be determined by Files.isWritable(). For such locations, Files.isWritable() is documented to return false, which the new code falsely assumes to mean that write access would be denied.

Repeat on Windows:
1. Create a folder C:\tmp\secretivefolder where reading permissions is denied*.
2. Launch (4.15+) eclipse.exe -configuration C:/tmp/secretivefolder
3. Note that an error message about write access appears

Expected: The launcher should happily accept this secretive location, like earlier versions do (e.g. 4.7.2).

*One way to create a secretive folder:
1. Go into "Properties" and click "Advanced"
2. Change the owner to someone else, e.g. the local "Administrators" group (since if you own the folder yourself, you can always read permissions)
3. Add an explicit deny rule for yourself for "Read permissions"
4. Apply

Of course, the regression wasn't noticed by creating strange permissions like this but rather because of the configuration location being in a home directory on a Samba file share that apparently effectively was secretive about its permissions.

I have attached the fix I applied locally. With this fix, canWrite() still uses Files.isWritable() as an early exit opportunity when it returns true, for which its documentation is less ambiguous, but after that it also tries the old algorithm from before the fix for bug 558612.

I believe that the proposed fix strikes a good balance between keeping the behavior of the fix for bug 558612 for 99% of all cases, but still not regressing in which configuration locations are accepted (and also being true to the letter of the Files.isWritable() documentation). I.e. if your folder is secretive about its permissions, you'll have to live with Eclipse creating a file inside it to find out for itself. Notably, if Files.isWritable() returns false due to the folder actually not being writable (also 99% of such cases), trying to write into it shouldn't do any harm since it will fail.

PS. The code brought back from the dead contains a comment that was questioned in bug 5444263, and a spelling error ("writtable"). I didn't touch either to make it clear that it's a revert, but maybe now is a good chance to sneak in a correction/clarification.