<Swing Dev> [9] Review request for 7067885 FileChooser does not display soft link name if link is to nonexistent file/directory
Alexander Scherbatiy
alexandr.scherbatiy at oracle.com
Wed Sep 28 17:03:25 UTC 2016
The fix looks good to me.
The link to the webrev:
http://cr.openjdk.java.net/~alexsch/alok.sharma/7067885/webrev.00
I updated the email subject to follow the review request rule.
Thanks,
Alexandr.
On 26/09/16 09:30, Sharma, Alok Kumar (OSTL) wrote:
> Thanks Alexander for you review.
>
> Mercurial diff for updated source change:
>
> diff -r 021369229cfd src/java.desktop/share/classes/sun/awt/shell/ShellFolder.java
> --- a/src/java.desktop/share/classes/sun/awt/shell/ShellFolder.java Tue Sep 06 13:09:29 2016 -0400
> +++ b/src/java.desktop/share/classes/sun/awt/shell/ShellFolder.java Fri Sep 23 12:15:51 2016 +0530
> @@ -32,6 +32,7 @@
> import java.io.FileNotFoundException;
> import java.util.*;
> import java.util.concurrent.Callable;
> +import java.nio.file.*;
>
> /**
> * @author Michael Martak
> @@ -240,10 +241,11 @@
> * @exception FileNotFoundException if file does not exist
> */
> public static ShellFolder getShellFolder(File file) throws FileNotFoundException {
> + Path path = Paths.get(file.getPath());
> if (file instanceof ShellFolder) {
> return (ShellFolder)file;
> }
> - if (!file.exists()) {
> + if (!Files.exists(path,LinkOption.NOFOLLOW_LINKS)) {
> throw new FileNotFoundException();
> }
> return shellFolderManager.createShellFolder(file);
>
> Below is the explanation for fix.
>
> Problem description:
> FileChooser behavior when there is a soft link to a non-existent file/directory.
> Soft link to be displayed with an error/exception when attempt is made to
> open it. Instead the soft link name is not displayed in the file chooser.
>
> Analysis:
> JFileChooser() internally uses java.io.exists() to check whether file exists.
> In this scenario JFileChooser() checks dangling symlink existence using java.io.exists().
> java.io.exists() api does not handle dangling symlink and returns false.
>
> JFileChooser() expects exists() api to return true for dangling symlink but, java.io.exists() returns false.
>
>
> Code changes:
> We have made changes in JFilechooser to use Non-Blocking java.nio.exists() api which check for file existence.
>
> java.nio.exists() handles dangling symlinks it accepts second argument link option which
> determines whether to follow the link or not and returns true for dangling symlinks.
>
>
> Regards,
> Alok
>
> -----Original Message-----
> From: Alexandr Scherbatiy [mailto:alexandr.scherbatiy at oracle.com]
> Sent: Tuesday, September 20, 2016 8:19 PM
> To: Sharma, Alok Kumar (OSTL) <alok.sharma at hpe.com>
> Cc: swing-dev at openjdk.java.net
> Subject: Re: <Swing Dev> OpenJDK JDK-7067885 code changes for community review
>
> Hello Alok,
>
> Is it possible to update the line where File.exists() is used just converting the file to a Path and using java.nio.exists() check?
>
> Thanks,
> Alexandr.
>
> On 9/16/2016 1:20 PM, Alexey Ivanov wrote:
>> Hi Alok,
>>
>> This change should be discussed on swing-dev mailing list because you
>> modify behavior of javax.swing.JFileChooser, and on core-libs-dev
>> because you also modify java.io.File.
>>
>> I agree with Alan, using the new API appears to be a better
>> alternative than changing java.io.File.
>>
>>
>> Regards,
>> Alexey
>>
>> On 12.09.2016 19:08, Alan Bateman wrote:
>>> Best to follow-up on swing-dev as I assume the right thing is to
>>> update JFileChooser rather than changing java.io.File to be mutable.
>>> You did acknowledge near the end of your mail that the new file
>>> system API supports sym links so it may be that JFileChooser could
>>> use that.
>>>
>>> -Alan
>>>
>>>
>>> On 12/09/2016 08:41, Sharma, Alok Kumar (OSTL) wrote:
>>>> Following is the fix for JDK-7067885. I am not sure which mailing ID
>>>> to post this.
>>>>
>>>> Bug: FileChooser does not display soft link name if link is to
>>>> nonexistent file/directory
>>>> https://bugs.openjdk.java.net/browse/JDK-7067885
>>>>
>>>> This bug is unassigned. Can someone please look into these changes
>>>> and get back to me? Explanation of fix is at the end of the source
>>>> code diff.
>>>>
>>>> Mercurial diff for source change:
>>>> -------------------------------------------------------------------
>>>> diff -r 021369229cfd src/java.base/share/classes/java/io/File.java
>>>> --- a/src/java.base/share/classes/java/io/File.java Tue Sep 06
>>>> 13:09:29 2016 -0400
>>>> +++ b/src/java.base/share/classes/java/io/File.java Mon Sep 12
>>>> 11:27:07 2016 +0530
>>>> @@ -164,6 +164,27 @@
>>>> private final String path;
>>>>
>>>> /**
>>>> + * The flag indicates whether to follow the symlink.
>>>> + */
>>>> + private boolean follow_link = true;
>>>> +
>>>> + /**
>>>> + * Sets the follow_link of file.
>>>> + * description: Sets follow_link on or off.
>>>> + * @param follow_link the value that determines whether to
>>>> follow the symlink or not.
>>>> + * TRUE: file.io.exists() checks the file existence using
>>>> stat() which follows the symlink.
>>>> + *
>>>> + * FALSE: file.io.exists() checks the file existence using
>>>> lstat() if stat() fails to retrieve
>>>> + * the file info. lstat() if file is a symbolic link, then
>>>> it returns information
>>>> + * about the link itself.
>>>> + * @return Returns ZERO at success
>>>> + */
>>>> + public int set_follow_link(boolean follow_link) {
>>>> + this.follow_link=follow_link;
>>>> + return 0;
>>>> + }
>>>> +
>>>> + /**
>>>> * Enum type that indicates the status of a file path.
>>>> */
>>>> private static enum PathStatus { INVALID, CHECKED }; diff -r
>>>> 021369229cfd src/java.base/unix/native/libjava/UnixFileSystem_md.c
>>>> --- a/src/java.base/unix/native/libjava/UnixFileSystem_md.c Tue Sep
>>>> 06 13:09:29 2016 -0400
>>>> +++ b/src/java.base/unix/native/libjava/UnixFileSystem_md.c Mon Sep
>>>> 12 11:27:07 2016 +0530
>>>> @@ -51,6 +51,7 @@
>>>> #define dirent64 dirent
>>>> #define readdir64_r readdir_r
>>>> #define stat64 stat
>>>> +#define lstat64 lstat
>>>> #ifndef MACOSX
>>>> #define statvfs64 statvfs
>>>> #endif
>>>> @@ -62,6 +63,7 @@
>>>> jfieldID path;
>>>> } ids;
>>>>
>>>> +jfieldID ufs_follow_link;
>>>>
>>>> JNIEXPORT void JNICALL
>>>> Java_java_io_UnixFileSystem_initIDs(JNIEnv *env, jclass cls) @@
>>>> -70,6 +72,8 @@
>>>> if (!fileClass) return;
>>>> ids.path = (*env)->GetFieldID(env, fileClass,
>>>> "path", "Ljava/lang/String;");
>>>> + ufs_follow_link = (*env)->GetFieldID(env, fileClass,
>>>> + "follow_link", "Z");
>>>> }
>>>>
>>>> /* -- Path operations -- */
>>>> @@ -113,20 +117,42 @@
>>>> return JNI_FALSE;
>>>> }
>>>>
>>>> +static jboolean
>>>> +lstatMode(const char *path, int *mode) {
>>>> + struct stat64 sb;
>>>> + if (lstat64(path, &sb) == 0) {
>>>> + *mode = sb.st_mode;
>>>> + return JNI_TRUE;
>>>> + }
>>>> + return JNI_FALSE;
>>>> +}
>>>>
>>>> JNIEXPORT jint JNICALL
>>>> Java_java_io_UnixFileSystem_getBooleanAttributes0(JNIEnv *env,
>>>> jobject this,
>>>> jobject file) {
>>>> jint rv = 0;
>>>> + jint follow_link = 0;
>>>>
>>>> WITH_FIELD_PLATFORM_STRING(env, file, ids.path, path) {
>>>> int mode;
>>>> - if (statMode(path, &mode)) {
>>>> - int fmt = mode & S_IFMT;
>>>> - rv = (jint) (java_io_FileSystem_BA_EXISTS
>>>> - | ((fmt == S_IFREG) ?
>>>> java_io_FileSystem_BA_REGULAR : 0)
>>>> - | ((fmt == S_IFDIR) ?
>>>> java_io_FileSystem_BA_DIRECTORY : 0));
>>>> + follow_link = (*env)->GetBooleanField(env, file,
>>>> ufs_follow_link);
>>>> + if ( follow_link ) {
>>>> + if (statMode(path, &mode)) {
>>>> + int fmt = mode & S_IFMT;
>>>> + rv = (jint) (java_io_FileSystem_BA_EXISTS
>>>> + | ((fmt == S_IFREG) ?
>>>> java_io_FileSystem_BA_REGULAR : 0)
>>>> + | ((fmt == S_IFDIR) ?
>>>> java_io_FileSystem_BA_DIRECTORY : 0));
>>>> + }
>>>> + }
>>>> + else {
>>>> + if (lstatMode(path, &mode)) {
>>>> + int fmt = mode & S_IFMT;
>>>> + rv = (jint) (java_io_FileSystem_BA_EXISTS
>>>> + | ((fmt == S_IFREG) ?
>>>> java_io_FileSystem_BA_REGULAR : 0)
>>>> + | ((fmt == S_IFDIR) ?
>>>> java_io_FileSystem_BA_DIRECTORY : 0));
>>>> + }
>>>> }
>>>> } END_PLATFORM_STRING(env, path);
>>>> return rv;
>>>> diff -r 021369229cfd
>>>> src/java.desktop/share/classes/javax/swing/filechooser/FileSystemView.java
>>>> ---
>>>> a/src/java.desktop/share/classes/javax/swing/filechooser/FileSystemView.java
>>>> Tue Sep 06 13:09:29 2016 -0400
>>>> +++
>>>> b/src/java.desktop/share/classes/javax/swing/filechooser/FileSystemView.java
>>>> Mon Sep 12 11:27:07 2016 +0530
>>>> @@ -521,6 +521,7 @@
>>>> f = createFileSystemRoot(f);
>>>> }
>>>> try {
>>>> + f.set_follow_link(false);
>>>> f = ShellFolder.getShellFolder(f);
>>>> } catch (FileNotFoundException e) {
>>>> // Not a valid file (wouldn't show in native
>>>> file chooser)
>>>> ------------------------------------------
>>>>
>>>> Below is the explanation for fix.
>>>>
>>>> Problem description:
>>>> FileChooser behavior when there is a soft link to a non-existent
>>>> file/directory.
>>>> Soft link to be displayed with an error/exception when attempt is
>>>> made to
>>>> open it. Instead the soft link name is not displayed in the file
>>>> chooser.
>>>>
>>>> Analysis:
>>>> JFileChooser() internally uses java.io.exists() to check whether
>>>> file exists.
>>>> In this scenario JFileChooser() checks dangling symlink existence
>>>> using java.io.exists().
>>>> java.io.exists() api does not handle dangling symlink and returns
>>>> false.
>>>>
>>>> JFileChooser() expects exists() api to return true for dangling
>>>> symlink but, java.io.exists() returns false.
>>>>
>>>> We see that there are two exists() apis in JAVA java.io.exists() and
>>>> java.nio.exists().
>>>>
>>>> java.nio.exists() handles dangling symlinks it accepts second
>>>> argument link option which
>>>> determines whether to follow the link or not and returns true for
>>>> dangling symlinks.
>>>> java.io.exists() follows the symlink and returns false if target
>>>> file doesn't exist.
>>>>
>>>> We have enhanced the java.io.exists() api to return true if it is a
>>>> dangling symlink.
>>>>
>>>> Changes in code:
>>>>
>>>> New variable "follow_link" is introduced in
>>>> "jdk/src/java.base/share/classes/java/io/File.java", we check for
>>>> this flag in exists() code
>>>> if its set then we handle symlinks. In case of dangling symlink
>>>> java.io.exists() api checks file existence
>>>> symlink itself not the target file and returns true.
>>>>
>>>> JFileChooser() issue (JDK-7067885) gets resolved with these changes.
>>>>
>>>> Testing:
>>>> I have covered the testing for the below apis.
>>>> Jfilechooser
>>>> getShellFolder
>>>> FileSystemView
>>>> and ran /openjdk9/jdk/test/javax/swing/JFileChooser/* tests.
>>>>
More information about the swing-dev
mailing list