Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit f592aa85 authored by Hai Zhang's avatar Hai Zhang
Browse files

Rewrite AtomicDirectory.

The previous version didn't close its FDs for directories, and called
fsync() on the wrong FDs. To make sure a rename() is persisted, we
need to call fsync() on the FD of the parent directory, because file
names are stored in directory entries instead of inodes.

Also removed the need for dedicated native code by calling the Os
class directly.

Fixes: 139302541
Bug: 138866253
Test: presubmit
Change-Id: I67fe98811814acba5158d760766a2ef3b121225a
parent fa3d2016
Loading
Loading
Loading
Loading
+81 −72
Original line number Diff line number Diff line
@@ -17,15 +17,17 @@
package com.android.internal.os;

import android.annotation.NonNull;
import android.annotation.Nullable;
import android.os.FileUtils;
import android.system.ErrnoException;
import android.system.Os;
import android.system.OsConstants;
import android.util.ArrayMap;
import android.util.Log;

import com.android.internal.util.Preconditions;

import libcore.io.IoUtils;

import java.io.File;
import java.io.FileDescriptor;
import java.io.FileOutputStream;
import java.io.IOException;
import java.util.Arrays;
@@ -48,12 +50,13 @@ import java.util.Arrays;
 * backing directory when checking existence, making changes, and deleting.
 */
public final class AtomicDirectory {
    private final @NonNull ArrayMap<File, FileOutputStream> mOpenFiles = new ArrayMap<>();

    private static final String LOG_TAG = AtomicDirectory.class.getSimpleName();

    private final @NonNull File mBaseDirectory;
    private final @NonNull File mBackupDirectory;

    private int mBaseDirectoryFd = -1;
    private int mBackupDirectoryFd = -1;
    private final @NonNull ArrayMap<File, FileOutputStream> mOpenFiles = new ArrayMap<>();

    /**
     * Creates a new instance.
@@ -67,15 +70,16 @@ public final class AtomicDirectory {
    }

    /**
     * Gets the backup directory if present. This could be useful if you are
     * writing new state to the dir but need to access the last persisted state
     * at the same time. This means that this call is useful in between
     * {@link #startWrite()} and {@link #finishWrite()} or {@link #failWrite()}.
     * You should not modify the content returned by this method.
     * Gets the backup directory which may or may not exist. This could be
     * useful if you are writing new state to the directory but need to access
     * the last persisted state at the same time. This means that this call is
     * useful in between {@link #startWrite()} and {@link #finishWrite()} or
     * {@link #failWrite()}. You should not modify the content returned by this
     * method.
     *
     * @see #startRead()
     */
    public @Nullable File getBackupDirectory() {
    public @NonNull File getBackupDirectory() {
        return mBackupDirectory;
    }

@@ -90,7 +94,8 @@ public final class AtomicDirectory {
     */
    public @NonNull File startRead() throws IOException {
        restore();
        return getOrCreateBaseDirectory();
        ensureBaseDirectory();
        return mBaseDirectory;
    }

    /**
@@ -99,10 +104,7 @@ public final class AtomicDirectory {
     * @see #startRead()
     * @see #startWrite()
     */
    public void finishRead() {
        mBaseDirectoryFd = -1;
        mBackupDirectoryFd = -1;
    }
    public void finishRead() {}

    /**
     * Starts editing this directory. After calling this method you should
@@ -121,7 +123,8 @@ public final class AtomicDirectory {
     */
    public @NonNull File startWrite() throws IOException {
        backup();
        return getOrCreateBaseDirectory();
        ensureBaseDirectory();
        return mBaseDirectory;
    }

    /**
@@ -135,11 +138,11 @@ public final class AtomicDirectory {
     * @see #closeWrite(FileOutputStream)
     */
    public @NonNull FileOutputStream openWrite(@NonNull File file) throws IOException {
        if (file.isDirectory() || !file.getParentFile().equals(getOrCreateBaseDirectory())) {
            throw new IllegalArgumentException("Must be a file in " + getOrCreateBaseDirectory());
        if (file.isDirectory() || !file.getParentFile().equals(mBaseDirectory)) {
            throw new IllegalArgumentException("Must be a file in " + mBaseDirectory);
        }
        if (mOpenFiles.containsKey(file)) {
            throw new IllegalArgumentException("Already open file" + file.getCanonicalPath());
            throw new IllegalArgumentException("Already open file " + file.getAbsolutePath());
        }
        final FileOutputStream destination = new FileOutputStream(file);
        mOpenFiles.put(file, destination);
@@ -160,7 +163,7 @@ public final class AtomicDirectory {
        }
        mOpenFiles.removeAt(indexOfValue);
        FileUtils.sync(destination);
        IoUtils.closeQuietly(destination);
        FileUtils.closeQuietly(destination);
    }

    public void failWrite(@NonNull FileOutputStream destination) {
@@ -169,7 +172,7 @@ public final class AtomicDirectory {
            throw new IllegalArgumentException("Unknown file stream " + destination);
        }
        mOpenFiles.removeAt(indexOfValue);
        IoUtils.closeQuietly(destination);
        FileUtils.closeQuietly(destination);
    }

    /**
@@ -177,15 +180,15 @@ public final class AtomicDirectory {
     *
     * @see #startWrite()
     *
     * @throws IllegalStateException is some files are not closed.
     * @throws IllegalStateException if some files are not closed.
     */
    public void finishWrite() {
        throwIfSomeFilesOpen();
        fsyncDirectoryFd(mBaseDirectoryFd);

        syncDirectory(mBaseDirectory);
        syncParentDirectory();
        deleteDirectory(mBackupDirectory);
        fsyncDirectoryFd(mBackupDirectoryFd);
        mBaseDirectoryFd = -1;
        mBackupDirectoryFd = -1;
        syncParentDirectory();
    }

    /**
@@ -195,11 +198,12 @@ public final class AtomicDirectory {
     */
    public void failWrite() {
        throwIfSomeFilesOpen();

        try{
            restore();
        } catch (IOException ignored) {}
        mBaseDirectoryFd = -1;
        mBackupDirectoryFd = -1;
        } catch (IOException e) {
            Log.e(LOG_TAG, "Failed to restore in failWrite()", e);
        }
    }

    /**
@@ -213,29 +217,28 @@ public final class AtomicDirectory {
     * Deletes this directory.
     */
    public void delete() {
        boolean deleted = false;
        if (mBaseDirectory.exists()) {
            deleteDirectory(mBaseDirectory);
            fsyncDirectoryFd(mBaseDirectoryFd);
            deleted |= deleteDirectory(mBaseDirectory);
        }
        if (mBackupDirectory.exists()) {
            deleteDirectory(mBackupDirectory);
            fsyncDirectoryFd(mBackupDirectoryFd);
            deleted |= deleteDirectory(mBackupDirectory);
        }
        if (deleted) {
            syncParentDirectory();
        }
    }

    private @NonNull File getOrCreateBaseDirectory() throws IOException {
        if (!mBaseDirectory.exists()) {
    private void ensureBaseDirectory() throws IOException {
        if (mBaseDirectory.exists()) {
            return;
        }

        if (!mBaseDirectory.mkdirs()) {
                throw new IOException("Couldn't create directory " + mBaseDirectory);
            throw new IOException("Failed to create directory " + mBaseDirectory);
        }
        FileUtils.setPermissions(mBaseDirectory.getPath(),
                    FileUtils.S_IRWXU | FileUtils.S_IRWXG | FileUtils.S_IXOTH,
                    -1, -1);
        }
        if (mBaseDirectoryFd < 0) {
            mBaseDirectoryFd = getDirectoryFd(mBaseDirectory.getCanonicalPath());
        }
        return mBaseDirectory;
                FileUtils.S_IRWXU | FileUtils.S_IRWXG | FileUtils.S_IXOTH, -1, -1);
    }

    private void throwIfSomeFilesOpen() {
@@ -249,50 +252,56 @@ public final class AtomicDirectory {
        if (!mBaseDirectory.exists()) {
            return;
        }
        if (mBaseDirectoryFd < 0) {
            mBaseDirectoryFd = getDirectoryFd(mBaseDirectory.getCanonicalPath());
        }

        if (mBackupDirectory.exists()) {
            deleteDirectory(mBackupDirectory);
        }
        if (!mBaseDirectory.renameTo(mBackupDirectory)) {
            throw new IOException("Couldn't backup " + mBaseDirectory
                    + " to " + mBackupDirectory);
            throw new IOException("Failed to backup " + mBaseDirectory + " to " + mBackupDirectory);
        }
        mBackupDirectoryFd = mBaseDirectoryFd;
        mBaseDirectoryFd = -1;
        fsyncDirectoryFd(mBackupDirectoryFd);
        syncParentDirectory();
    }

    private void restore() throws IOException {
        if (!mBackupDirectory.exists()) {
            return;
        }
        if (mBackupDirectoryFd == -1) {
            mBackupDirectoryFd = getDirectoryFd(mBackupDirectory.getCanonicalPath());
        }

        if (mBaseDirectory.exists()) {
            deleteDirectory(mBaseDirectory);
        }
        if (!mBackupDirectory.renameTo(mBaseDirectory)) {
            throw new IOException("Couldn't restore " + mBackupDirectory
                    + " to " + mBaseDirectory);
            throw new IOException("Failed to restore " + mBackupDirectory + " to "
                    + mBaseDirectory);
        }
        mBaseDirectoryFd = mBackupDirectoryFd;
        mBackupDirectoryFd = -1;
        fsyncDirectoryFd(mBaseDirectoryFd);
        syncParentDirectory();
    }

    private static void deleteDirectory(@NonNull File file) {
        final File[] children = file.listFiles();
        if (children != null) {
            for (File child : children) {
                deleteDirectory(child);
    private static boolean deleteDirectory(@NonNull File directory) {
        return FileUtils.deleteContentsAndDir(directory);
    }
        }
        file.delete();

    private void syncParentDirectory() {
        syncDirectory(mBaseDirectory.getParentFile());
    }

    private static native int getDirectoryFd(String path);
    private static native void fsyncDirectoryFd(int fd);
    // Standard Java IO doesn't allow opening a directory (will throw a FileNotFoundException
    // instead), so we have to do it manually.
    private static void syncDirectory(@NonNull File directory) {
        String path = directory.getAbsolutePath();
        FileDescriptor fd;
        try {
            fd = Os.open(path, OsConstants.O_RDONLY, 0);
        } catch (ErrnoException e) {
            Log.e(LOG_TAG, "Failed to open " + path, e);
            return;
        }
        try {
            Os.fsync(fd);
        } catch (ErrnoException e) {
            Log.e(LOG_TAG, "Failed to fsync " + path, e);
        } finally {
            FileUtils.closeQuietly(fd);
        }
    }
}
+0 −1
Original line number Diff line number Diff line
@@ -194,7 +194,6 @@ cc_library_shared {
                "android_content_res_ObbScanner.cpp",
                "android_content_res_Configuration.cpp",
                "android_security_Scrypt.cpp",
                "com_android_internal_os_AtomicDirectory.cpp",
                "com_android_internal_os_ClassLoaderFactory.cpp",
                "com_android_internal_os_FuseAppLoop.cpp",
                "com_android_internal_os_Zygote.cpp",
+0 −2
Original line number Diff line number Diff line
@@ -227,7 +227,6 @@ extern int register_android_content_res_Configuration(JNIEnv* env);
extern int register_android_animation_PropertyValuesHolder(JNIEnv *env);
extern int register_android_security_Scrypt(JNIEnv *env);
extern int register_com_android_internal_content_NativeLibraryHelper(JNIEnv *env);
extern int register_com_android_internal_os_AtomicDirectory(JNIEnv *env);
extern int register_com_android_internal_os_ClassLoaderFactory(JNIEnv* env);
extern int register_com_android_internal_os_FuseAppLoop(JNIEnv* env);
extern int register_com_android_internal_os_Zygote(JNIEnv *env);
@@ -1602,7 +1601,6 @@ static const RegJNIRec gRegJNI[] = {
    REG_JNI(register_android_animation_PropertyValuesHolder),
    REG_JNI(register_android_security_Scrypt),
    REG_JNI(register_com_android_internal_content_NativeLibraryHelper),
    REG_JNI(register_com_android_internal_os_AtomicDirectory),
    REG_JNI(register_com_android_internal_os_FuseAppLoop),
};

+0 −66
Original line number Diff line number Diff line
/*
 * Copyright (C) 2018 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.
 */

#include <nativehelper/ScopedUtfChars.h>
#include "jni.h"

#include "core_jni_helpers.h"

namespace android {

static jint com_android_internal_os_AtomicDirectory_getDirectoryFd(JNIEnv* env,
        jobject /*clazz*/, jstring path) {
    ScopedUtfChars path8(env, path);
    if (path8.c_str() == NULL) {
        ALOGE("Invalid path: %s", path8.c_str());
        return -1;
    }
    int fd;
    if ((fd = TEMP_FAILURE_RETRY(open(path8.c_str(), O_DIRECTORY | O_RDONLY | O_CLOEXEC))) == -1) {
        ALOGE("Cannot open directory %s, error: %s\n", path8.c_str(), strerror(errno));
        return -1;
    }
    return fd;
}

static void com_android_internal_os_AtomicDirectory_fsyncDirectoryFd(JNIEnv* env,
        jobject /*clazz*/, jint fd) {
    if (TEMP_FAILURE_RETRY(fsync(fd)) == -1) {
        ALOGE("Cannot fsync directory %d, error: %s\n", fd, strerror(errno));
    }
}

/*
 * JNI registration.
 */
static const JNINativeMethod gRegisterMethods[] = {
    /* name, signature, funcPtr */
    { "fsyncDirectoryFd",
      "(I)V",
       (void*) com_android_internal_os_AtomicDirectory_fsyncDirectoryFd
    },
    { "getDirectoryFd",
      "(Ljava/lang/String;)I",
       (void*) com_android_internal_os_AtomicDirectory_getDirectoryFd
    },
};

int register_com_android_internal_os_AtomicDirectory(JNIEnv* env) {
    return RegisterMethodsOrDie(env, "com/android/internal/os/AtomicDirectory",
            gRegisterMethods, NELEM(gRegisterMethods));
}

}; // namespace android