[GR-54914] Add some validation to methods in VirtualFileSystem.Builder

PullRequest: graalpython/3535

Author: tomasstupka

graalpython/com.oracle.graal.python.test.integration/src/org/graalvm/python/embedding/utils/test/VirtualFileSystemTest.java

@@ -60,6 +60,7 @@
 import java.nio.file.AccessMode;
 import java.nio.file.DirectoryStream;
 import java.nio.file.Files;
+import java.nio.file.InvalidPathException;
 import java.nio.file.NoSuchFileException;
 import java.nio.file.NotDirectoryException;
 import java.nio.file.Path;
@@ -494,15 +495,19 @@ private static void checkExtractedFile(Path extractedFile, String[] expectedCont
         }
     }
 
+    private static void checkException(Class<?> exType, Callable<Object> c) {
+        checkException(exType, c, null);
+    }
+
     private static void checkException(Class<?> exType, Callable<Object> c, String msg) {
         boolean gotEx = false;
         try {
             c.call();
         } catch (Exception e) {
-            assert e.getClass() == exType;
+            assertEquals(e.getClass(), exType);
             gotEx = true;
         }
-        assertTrue(msg, gotEx);
+        assertTrue(msg != null ? msg : "expected " + exType.getName(), gotEx);
     }
 
     @Test
@@ -863,6 +868,24 @@ public void externalResourcesBuilderTest() throws IOException {
         }
     }
 
+    @Test
+    public void vfsMountPointTest() {
+        if (IS_WINDOWS) {
+            checkException(IllegalArgumentException.class, () -> VirtualFileSystem.newBuilder().windowsMountPoint("test"));
+            checkException(IllegalArgumentException.class, () -> VirtualFileSystem.newBuilder().windowsMountPoint("test\\"));
+            checkException(IllegalArgumentException.class, () -> VirtualFileSystem.newBuilder().windowsMountPoint("\\test\\"));
+            checkException(IllegalArgumentException.class, () -> VirtualFileSystem.newBuilder().windowsMountPoint("\\test"));
+            checkException(IllegalArgumentException.class, () -> VirtualFileSystem.newBuilder().windowsMountPoint("X:\\test\\"));
+            checkException(InvalidPathException.class, () -> VirtualFileSystem.newBuilder().windowsMountPoint("X:\\test|test"));
+        } else {
+            checkException(IllegalArgumentException.class, () -> VirtualFileSystem.newBuilder().unixMountPoint("test"));
+            checkException(IllegalArgumentException.class, () -> VirtualFileSystem.newBuilder().unixMountPoint("test/"));
+            checkException(IllegalArgumentException.class, () -> VirtualFileSystem.newBuilder().unixMountPoint("/test/"));
+            checkException(IllegalArgumentException.class, () -> VirtualFileSystem.newBuilder().unixMountPoint("X:\\test"));
+            checkException(InvalidPathException.class, () -> VirtualFileSystem.newBuilder().unixMountPoint("/test/\0"));
+        }
+    }
+
     private static Builder addTestOptions(Builder builder) {
         return builder.option("engine.WarnInterpreterOnly", "false");
     }

graalpython/org.graalvm.python.embedding/src/org/graalvm/python/embedding/utils/VirtualFileSystem.java

@@ -134,8 +134,9 @@ public static final class Builder {
             return s.endsWith(".so") || s.endsWith(".dylib") || s.endsWith(".pyd") || s.endsWith(".dll") || s.endsWith(".ttf");
         };
 
-        private String windowsMountPoint = "X:\\graalpy_vfs";
-        private String unixMountPoint = "/graalpy_vfs";
+        private static final String DEFAULT_WINDOWS_MOUNT_POINT = "X:\\graalpy_vfs";
+        private String DEFAULT_UNIX_MOUNT_POINT = "/graalpy_vfs";
+        private Path mountPoint;
         private Predicate<Path> extractFilter = DEFAULT_EXTRACT_FILTER;
         private HostIO allowHostIO = HostIO.READ_WRITE;
         private boolean caseInsensitive = isWindows();
@@ -171,7 +172,9 @@ public Builder allowHostIO(HostIO b) {
          * will not be accessible.
          */
         public Builder windowsMountPoint(String s) {
-            windowsMountPoint = s;
+            if (isWindows()) {
+                mountPoint = getMountPointAsPath(s);
+            }
             return this;
         }
 
@@ -184,7 +187,9 @@ public Builder windowsMountPoint(String s) {
          * will not be accessible.
          */
         public Builder unixMountPoint(String s) {
-            unixMountPoint = s;
+            if (!isWindows()) {
+                mountPoint = getMountPointAsPath(s);
+            }
             return this;
         }
 
@@ -219,10 +224,21 @@ public Builder extractFilter(Predicate<Path> filter) {
         }
 
         public VirtualFileSystem build() {
-            return new VirtualFileSystem(extractFilter, windowsMountPoint, unixMountPoint, allowHostIO, resourceLoadingClass, caseInsensitive);
+            if (mountPoint == null) {
+                mountPoint = isWindows() ? Path.of(DEFAULT_WINDOWS_MOUNT_POINT) : Path.of(DEFAULT_UNIX_MOUNT_POINT);
+            }
+            return new VirtualFileSystem(extractFilter, mountPoint, allowHostIO, resourceLoadingClass, caseInsensitive);
         }
     }
 
+    private static Path getMountPointAsPath(String mp) {
+        Path mountPoint = Path.of(mp);
+        if (mp.endsWith(PLATFORM_SEPARATOR) || !mountPoint.isAbsolute()) {
+            throw new IllegalArgumentException(String.format("Virtual filesystem mount point must be set to an absolute path without a trailing separator: '%s'", mp));
+        }
+        return mountPoint;
+    }
+
     public static Builder newBuilder() {
         return new Builder();
     }
@@ -353,8 +369,7 @@ private void removeExtractDir() {
      * causing that no extraction will happen.
      */
     VirtualFileSystem(Predicate<Path> extractFilter,
-                    String windowsMountPoint,
-                    String unixMountPoint,
+                    Path mountPoint,
                     HostIO allowHostIO,
                     Class<?> resourceLoadingClass,
                     boolean caseInsensitive) {
@@ -365,15 +380,9 @@ private void removeExtractDir() {
         }
 
         this.caseInsensitive = caseInsensitive;
-        String mp = System.getenv("GRAALPY_VFS_MOUNT_POINT");
-        if (mp == null) {
-            mp = isWindows() ? windowsMountPoint : unixMountPoint;
-        }
-        this.mountPoint = Path.of(mp);
-        this.mountPointLowerCase = mp.toLowerCase(Locale.ROOT);
-        if (mp.endsWith(PLATFORM_SEPARATOR) || !mountPoint.isAbsolute()) {
-            throw new IllegalArgumentException("GRAALPY_VFS_MOUNT_POINT must be set to an absolute path without a trailing separator");
-        }
+        this.mountPoint = mountPoint;
+
+        this.mountPointLowerCase = mountPoint.toString().toLowerCase(Locale.ROOT);
 
         fine("VirtualFilesystem %s, allowHostIO: %s, resourceLoadingClass: %s, caseInsensitive: %s, extractOnStartup: %s%s",
                         mountPoint, allowHostIO.toString(), this.resourceLoadingClass.getName(), caseInsensitive, extractOnStartup, extractFilter != null ? "" : ", extractFilter: null");