Improve the FileNotFoundExceptions thrown by OSFileSystem.open.

When I improved the internals of java.io.File, I failed to keep
OSFileSystem.open (which uses the internals of java.io.File) in
sync, leading to misleading error reporting. java.io.File's
internals now include a trailing NUL, which is useful for the
native code but confuses Java if it tries to decode the byte[]
as a UTF-8 sequence.

This patch fixes the bug and also improves OSFileSystem.open's
error reporting to include the reason for the failure.

Bug: 2313271

4 files changed, 94 insertions, 92 deletions

diff --git a/include/ScopedByteArray.h b/include/ScopedByteArray.h
new file mode 100644
index 0000000..bcbee99
--- /dev/null
+++ b/include/ScopedByteArray.h
@@ -0,0 +1,53 @@
+/*
+ * Copyright (C) 2009 The Android Open Source Project
+
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef SCOPED_BYTE_ARRAY_H_included
+#define SCOPED_BYTE_ARRAY_H_included
+
+#include "JNIHelp.h"
+
+// A smart pointer that provides read-only access to a Java byte[].
+class ScopedByteArray {
+public:
+    ScopedByteArray(JNIEnv* env, jbyteArray byteArray)
+    : mEnv(env), mByteArray(byteArray), mBytes(NULL)
+    {
+        mBytes = env->GetByteArrayElements(mByteArray, NULL);
+    }
+
+    ~ScopedByteArray() {
+        if (mBytes) {
+            mEnv->ReleaseByteArrayElements(mByteArray, mBytes, JNI_ABORT);
+        }
+    }
+
+    const jbyte* bytes() const {
+        return mBytes;
+    }
+
+    // Element access.
+    const char& operator[](size_t n) const {
+        const char* array = reinterpret_cast<const char*>(mBytes);
+        return array[n];
+    }
+
+private:
+    JNIEnv* mEnv;
+    jbyteArray mByteArray;
+    jbyte* mBytes;
+};
+
+#endif  // SCOPED_BYTE_ARRAY_H_included
diff --git a/luni/src/main/java/org/apache/harmony/luni/platform/OSFileSystem.java b/luni/src/main/java/org/apache/harmony/luni/platform/OSFileSystem.java
index b7a62e2..13ca3ab 100644
--- a/luni/src/main/java/org/apache/harmony/luni/platform/OSFileSystem.java
+++ b/luni/src/main/java/org/apache/harmony/luni/platform/OSFileSystem.java
@@ -145,22 +145,11 @@ class OSFileSystem implements IFileSystem {
     public native void truncate(int fileDescriptor, long size) throws IOException;
     // END android-changed
 
-    public int open(byte[] fileName, int mode) throws FileNotFoundException {
-        if (fileName == null) {
+    public int open(byte[] utfPathBytes, int mode) throws FileNotFoundException {
+        if (utfPathBytes == null) {
             throw new NullPointerException();
         }
-        int handler = openImpl(fileName, mode);
-        if (handler < 0) {
-            try {
-                throw new FileNotFoundException(new String(fileName, "UTF-8"));
-            } catch (java.io.UnsupportedEncodingException e) {
-                // UTF-8 should always be supported, so throw an assertion
-                FileNotFoundException fnfe = new FileNotFoundException(new String(fileName));
-                e.initCause(fnfe);
-                throw new AssertionError(e);
-            }
-        }
-        return handler;
+        return openImpl(utfPathBytes, mode);
     }
 
     private native int openImpl(byte[] fileName, int mode);
diff --git a/luni/src/main/native/java_io_File.cpp b/luni/src/main/native/java_io_File.cpp
index f85daf8..905eae2 100644
--- a/luni/src/main/native/java_io_File.cpp
+++ b/luni/src/main/native/java_io_File.cpp
@@ -18,6 +18,7 @@
 #include "AndroidSystemNatives.h"
 #include "JNIHelp.h"
 #include "LocalArray.h"
+#include "ScopedByteArray.h"
 #include "ScopedFd.h"
 
 #include <string.h>
@@ -36,40 +37,8 @@
 // poor choices of where to divide the work between Java and native
 // code.
 
-class Path {
-public:
-    // The Java byte[] already contains a trailing NUL.
-    Path(JNIEnv* env, jbyteArray byteArray)
-    : mEnv(env), mByteArray(byteArray), mBytes(NULL)
-    {
-        mBytes = env->GetByteArrayElements(mByteArray, NULL);
-    }
-
-    ~Path() {
-        if (mBytes) {
-            mEnv->ReleaseByteArrayElements(mByteArray, mBytes, JNI_ABORT);
-        }
-    }
-
-    const jbyte* bytes() const {
-        return mBytes;
-    }
-
-    // Element access.
-    const char& operator[](size_t n) const {
-        const char* array = reinterpret_cast<const char*>(mBytes);
-        return array[n];
-    }
-
-private:
-    JNIEnv* mEnv;
-    jbyteArray mByteArray;
-    jbyte* mBytes;
-};
-
-
 static jbyteArray java_io_File_getCanonImpl(JNIEnv* env, jobject, jbyteArray pathBytes) {
-    Path path(env, pathBytes);
+    ScopedByteArray path(env, pathBytes);
     // The only thing this native code currently does is truncate the byte[] at
     // the first NUL.
     // TODO: this is completely pointless. we should do this in Java, or do all of getCanonicalPath in native code. (realpath(2)?)
@@ -80,12 +49,12 @@ static jbyteArray java_io_File_getCanonImpl(JNIEnv* env, jobject, jbyteArray pat
 }
 
 static jboolean java_io_File_deleteImpl(JNIEnv* env, jobject, jbyteArray pathBytes) {
-    Path path(env, pathBytes);
+    ScopedByteArray path(env, pathBytes);
     return (remove(&path[0]) == 0);
 }
 
 static bool doStat(JNIEnv* env, jbyteArray pathBytes, struct stat& sb) {
-    Path path(env, pathBytes);
+    ScopedByteArray path(env, pathBytes);
     return (stat(&path[0], &sb) == 0);
 }
 
@@ -132,22 +101,22 @@ static jboolean java_io_File_isFileImpl(JNIEnv* env, jobject, jbyteArray pathByt
 }
 
 static jboolean java_io_File_existsImpl(JNIEnv* env, jobject, jbyteArray pathBytes) {
-    Path path(env, pathBytes);
+    ScopedByteArray path(env, pathBytes);
     return (access(&path[0], F_OK) == 0);
 }
 
 static jboolean java_io_File_isReadableImpl(JNIEnv* env, jobject, jbyteArray pathBytes) {
-    Path path(env, pathBytes);
+    ScopedByteArray path(env, pathBytes);
     return (access(&path[0], R_OK) == 0);
 }
 
 static jboolean java_io_File_isWritableImpl(JNIEnv* env, jobject recv, jbyteArray pathBytes) {
-    Path path(env, pathBytes);
+    ScopedByteArray path(env, pathBytes);
     return (access(&path[0], W_OK) == 0);
 }
 
 static jbyteArray java_io_File_getLinkImpl(JNIEnv* env, jobject, jbyteArray pathBytes) {
-    Path path(env, pathBytes);
+    ScopedByteArray path(env, pathBytes);
 
     // We can't know how big a buffer readlink(2) will need, so we need to
     // loop until it says "that fit".
@@ -174,7 +143,7 @@ static jbyteArray java_io_File_getLinkImpl(JNIEnv* env, jobject, jbyteArray path
 }
 
 static jboolean java_io_File_setLastModifiedImpl(JNIEnv* env, jobject, jbyteArray pathBytes, jlong ms) {
-    Path path(env, pathBytes);
+    ScopedByteArray path(env, pathBytes);
     
     // We want to preserve the access time.
     struct stat sb;
@@ -190,7 +159,7 @@ static jboolean java_io_File_setLastModifiedImpl(JNIEnv* env, jobject, jbyteArra
 }
 
 static jboolean java_io_File_setReadOnlyImpl(JNIEnv* env, jobject recv, jbyteArray pathBytes) {
-    Path path(env, pathBytes);
+    ScopedByteArray path(env, pathBytes);
 
     struct stat sb;
     if (stat(&path[0], &sb) == -1) {
@@ -238,7 +207,7 @@ struct LinkedDirEntry {
 };
 
 static jobject java_io_File_listImpl(JNIEnv* env, jclass clazz, jbyteArray pathBytes) {
-    Path path(env, pathBytes);
+    ScopedByteArray path(env, pathBytes);
     
     ScopedReaddir dir(opendir(&path[0]));
     if (dir.dirp == NULL) {
@@ -302,13 +271,13 @@ static jobject java_io_File_listImpl(JNIEnv* env, jclass clazz, jbyteArray pathB
 }
 
 static jboolean java_io_File_mkdirImpl(JNIEnv* env, jobject, jbyteArray pathBytes) {
-    Path path(env, pathBytes);
+    ScopedByteArray path(env, pathBytes);
     // On Android, we don't want default permissions to allow global access.
     return (mkdir(&path[0], S_IRWXU) == 0);
 }
 
 static jboolean java_io_File_createNewFileImpl(JNIEnv* env, jobject, jbyteArray pathBytes) {
-    Path path(env, pathBytes);
+    ScopedByteArray path(env, pathBytes);
     // On Android, we don't want default permissions to allow global access.
     ScopedFd fd(open(&path[0], O_CREAT | O_EXCL, 0600));
     if (fd.get() != -1) {
@@ -324,8 +293,8 @@ static jboolean java_io_File_createNewFileImpl(JNIEnv* env, jobject, jbyteArray
 }
 
 static jboolean java_io_File_renameToImpl(JNIEnv* env, jobject, jbyteArray oldPathBytes, jbyteArray newPathBytes) {
-    Path oldPath(env, oldPathBytes);
-    Path newPath(env, newPathBytes);
+    ScopedByteArray oldPath(env, oldPathBytes);
+    ScopedByteArray newPath(env, newPathBytes);
     return (rename(&oldPath[0], &newPath[0]) == 0);
 }
 
diff --git a/luni/src/main/native/org_apache_harmony_luni_platform_OSFileSystem.cpp b/luni/src/main/native/org_apache_harmony_luni_platform_OSFileSystem.cpp
index 1e1893e..3100767 100644
--- a/luni/src/main/native/org_apache_harmony_luni_platform_OSFileSystem.cpp
+++ b/luni/src/main/native/org_apache_harmony_luni_platform_OSFileSystem.cpp
@@ -25,15 +25,12 @@
  * Common natives supporting the file system interface.
  */
 
-#define HyMaxPath 1024
-
 /* Values for HyFileOpen */
 #define HyOpenRead    1
 #define HyOpenWrite   2
 #define HyOpenCreate  4
 #define HyOpenTruncate  8
 #define HyOpenAppend  16
-#define HyOpenText    32
 /* Use this flag with HyOpenCreate, if this flag is specified then
  * trying to create an existing file will fail
  */
@@ -41,8 +38,11 @@
 #define HyOpenSync      128
 #define SHARED_LOCK_TYPE 1L
 
-#include "JNIHelp.h"
 #include "AndroidSystemNatives.h"
+#include "JNIHelp.h"
+#include "LocalArray.h"
+#include "ScopedByteArray.h"
+
 #include <assert.h>
 #include <errno.h>
 #include <fcntl.h>
@@ -76,18 +76,6 @@ static inline ssize_t sendfile(int out_fd, int in_fd, off_t *offset,
 }
 #endif
 
-static void convertToPlatform(char *path) {
-    char *pathIndex;
-
-    pathIndex = path;
-    while (*pathIndex != '\0') {
-        if (*pathIndex == '\\') {
-            *pathIndex = '/';
-        }
-        pathIndex++;
-    }
-}
-
 static int EsTranslateOpenFlags(int flags) {
     int realFlags = 0;
 
@@ -409,7 +397,7 @@ static jint harmony_io_truncate(JNIEnv* env, jobject, jint fd, jlong length) {
     return rc;
 }
 
-static jint harmony_io_openImpl(JNIEnv* env, jobject, jbyteArray path,
+static jint harmony_io_openImpl(JNIEnv* env, jobject, jbyteArray pathByteArray,
         jint jflags) {
     int flags = 0;
     int mode = 0;
@@ -442,21 +430,24 @@ static jint harmony_io_openImpl(JNIEnv* env, jobject, jbyteArray path,
 
     flags = EsTranslateOpenFlags(flags);
 
-    // TODO: clean this up when we clean up the java.io.File equivalent.
-    jsize length = env->GetArrayLength (path);
-    length = length < HyMaxPath - 1 ? length : HyMaxPath - 1;
-    char pathCopy[HyMaxPath];
-    env->GetByteArrayRegion (path, 0, length, (jbyte *)pathCopy);
-    pathCopy[length] = '\0';
-    convertToPlatform (pathCopy);
-
-    jint cc = TEMP_FAILURE_RETRY(open(pathCopy, flags, mode));
-    // TODO: chase up the callers of this and check they wouldn't rather
-    // have us throw a meaningful IOException right here.
-    if (cc < 0 && errno > 0) {
-        cc = -errno;
-    }
-    return cc;
+    ScopedByteArray path(env, pathByteArray);
+    jint rc = TEMP_FAILURE_RETRY(open(&path[0], flags, mode));
+    if (rc == -1) {
+        // Get the human-readable form of errno.
+        char buffer[80];
+        const char* reason = jniStrError(errno, &buffer[0], sizeof(buffer));
+
+        // Construct a message that includes the path and the reason.
+        // (pathByteCount already includes space for our trailing NUL.)
+        size_t pathByteCount = env->GetArrayLength(pathByteArray);
+        LocalArray<128> message(pathByteCount + 2 + strlen(reason) + 1);
+        snprintf(&message[0], message.size(), "%s (%s)", &path[0], reason);
+
+        // We always throw FileNotFoundException, regardless of the specific
+        // failure. (This appears to be true of the RI too.)
+        jniThrowException(env, "java/io/FileNotFoundException", &message[0]);
+    }
+    return rc;
 }
 
 static jint harmony_io_ioctlAvailable(JNIEnv*env, jobject, jint fd) {