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

Commit 3f15e584 authored by Colin Cross's avatar Colin Cross
Browse files

Use std::unique_ptr in FileDescriptorTable 

Simplify the memory management in FileDescriptorTable by using
std::unique_ptr to hold FileDescriptorInfo objects.  Would have
prevented b/231366539.

Test: adb shell dumpsys meminfo --unreachable com.android.deskclock
Change-Id: I45fb54f7d0b80471183087d4bf20fbe2a3cc2641
parent b8b8e215

core/jni/fd_utils.cpp

+14 −18
Original line number Original line Diff line number Diff line
@@ -17,6 +17,7 @@ 
#include "fd_utils.h"

#include "fd_utils.h"





#include <algorithm>

#include <algorithm>

#include <utility>





#include <fcntl.h>

#include <fcntl.h>

#include <grp.h>

#include <grp.h>
@@ -174,7 +175,7 @@ FileDescriptorAllowlist* FileDescriptorAllowlist::instance_ = nullptr; 
class FileDescriptorInfo {

class FileDescriptorInfo {

 public:

 public:

  // Create a FileDescriptorInfo for a given file descriptor.

  // Create a FileDescriptorInfo for a given file descriptor.

  static FileDescriptorInfo* CreateFromFd(int fd, fail_fn_t fail_fn);

  static std::unique_ptr<FileDescriptorInfo> CreateFromFd(int fd, fail_fn_t fail_fn);





  // Checks whether the file descriptor associated with this object refers to

  // Checks whether the file descriptor associated with this object refers to

  // the same description.

  // the same description.
@@ -213,7 +214,7 @@ class FileDescriptorInfo { 
  DISALLOW_COPY_AND_ASSIGN(FileDescriptorInfo);

  DISALLOW_COPY_AND_ASSIGN(FileDescriptorInfo);

};

};





FileDescriptorInfo* FileDescriptorInfo::CreateFromFd(int fd, fail_fn_t fail_fn) {

std::unique_ptr<FileDescriptorInfo> FileDescriptorInfo::CreateFromFd(int fd, fail_fn_t fail_fn) {

  struct stat f_stat;

  struct stat f_stat;

  // This should never happen; the zygote should always have the right set

  // This should never happen; the zygote should always have the right set

  // of permissions required to stat all its open files.

  // of permissions required to stat all its open files.
@@ -234,7 +235,7 @@ FileDescriptorInfo* FileDescriptorInfo::CreateFromFd(int fd, fail_fn_t fail_fn) 
                                            socket_name.c_str(), fd));

                                            socket_name.c_str(), fd));

    }

    }





    return new FileDescriptorInfo(fd);

    return std::unique_ptr<FileDescriptorInfo>(new FileDescriptorInfo(fd));

  }

  }





  // We only handle allowlisted regular files and character devices. Allowlisted

  // We only handle allowlisted regular files and character devices. Allowlisted
@@ -315,7 +316,8 @@ FileDescriptorInfo* FileDescriptorInfo::CreateFromFd(int fd, fail_fn_t fail_fn) 
  int open_flags = fs_flags & (kOpenFlags);

  int open_flags = fs_flags & (kOpenFlags);

  fs_flags = fs_flags & (~(kOpenFlags));

  fs_flags = fs_flags & (~(kOpenFlags));





  return new FileDescriptorInfo(f_stat, file_path, fd, open_flags, fd_flags, fs_flags, offset);

  return std::unique_ptr<FileDescriptorInfo>(

    new FileDescriptorInfo(f_stat, file_path, fd, open_flags, fd_flags, fs_flags, offset));

}

}





bool FileDescriptorInfo::RefersToSameFile() const {

bool FileDescriptorInfo::RefersToSameFile() const {
@@ -482,11 +484,11 @@ static std::unique_ptr<std::set<int>> GetOpenFdsIgnoring(const std::vector<int>& 
FileDescriptorTable* FileDescriptorTable::Create(const std::vector<int>& fds_to_ignore,

FileDescriptorTable* FileDescriptorTable::Create(const std::vector<int>& fds_to_ignore,

                                                 fail_fn_t fail_fn) {

                                                 fail_fn_t fail_fn) {

  std::unique_ptr<std::set<int>> open_fds = GetOpenFdsIgnoring(fds_to_ignore, fail_fn);

  std::unique_ptr<std::set<int>> open_fds = GetOpenFdsIgnoring(fds_to_ignore, fail_fn);

  std::unordered_map<int, FileDescriptorInfo*> open_fd_map;

  std::unordered_map<int, std::unique_ptr<FileDescriptorInfo>> open_fd_map;

  for (auto fd : *open_fds) {

  for (auto fd : *open_fds) {

    open_fd_map[fd] = FileDescriptorInfo::CreateFromFd(fd, fail_fn);

    open_fd_map[fd] = FileDescriptorInfo::CreateFromFd(fd, fail_fn);

  }

  }

  return new FileDescriptorTable(open_fd_map);

  return new FileDescriptorTable(std::move(open_fd_map));

}

}





static std::unique_ptr<std::set<int>> GetOpenFdsIgnoring(const std::vector<int>& fds_to_ignore,

static std::unique_ptr<std::set<int>> GetOpenFdsIgnoring(const std::vector<int>& fds_to_ignore,
@@ -535,9 +537,9 @@ void FileDescriptorTable::Restat(const std::vector<int>& fds_to_ignore, fail_fn_ 




// Reopens all file descriptors that are contained in the table.

// Reopens all file descriptors that are contained in the table.

void FileDescriptorTable::ReopenOrDetach(fail_fn_t fail_fn) {

void FileDescriptorTable::ReopenOrDetach(fail_fn_t fail_fn) {

  std::unordered_map<int, FileDescriptorInfo*>::const_iterator it;

  std::unordered_map<int, std::unique_ptr<FileDescriptorInfo>>::const_iterator it;

  for (it = open_fd_map_.begin(); it != open_fd_map_.end(); ++it) {

  for (it = open_fd_map_.begin(); it != open_fd_map_.end(); ++it) {

    const FileDescriptorInfo* info = it->second;

    const FileDescriptorInfo* info = it->second.get();

    if (info == nullptr) {

    if (info == nullptr) {

      return;

      return;

    } else {

    } else {
@@ -547,15 +549,11 @@ void FileDescriptorTable::ReopenOrDetach(fail_fn_t fail_fn) { 
}

}





FileDescriptorTable::FileDescriptorTable(

FileDescriptorTable::FileDescriptorTable(

    const std::unordered_map<int, FileDescriptorInfo*>& map)

  std::unordered_map<int, std::unique_ptr<FileDescriptorInfo>> map)

    : open_fd_map_(map) {

  : open_fd_map_(std::move(map)) {

}

}





FileDescriptorTable::~FileDescriptorTable() {

FileDescriptorTable::~FileDescriptorTable() {}

    for (auto& it : open_fd_map_) {

        delete it.second;

    }

}





void FileDescriptorTable::RestatInternal(std::set<int>& open_fds, fail_fn_t fail_fn) {

void FileDescriptorTable::RestatInternal(std::set<int>& open_fds, fail_fn_t fail_fn) {

  // ART creates a file through memfd for optimization purposes. We make sure

  // ART creates a file through memfd for optimization purposes. We make sure
@@ -569,7 +567,7 @@ void FileDescriptorTable::RestatInternal(std::set<int>& open_fds, fail_fn_t fail 
  // (b) they refer to the same file.

  // (b) they refer to the same file.

  //

  //

  // We'll only store the last error message.

  // We'll only store the last error message.

  std::unordered_map<int, FileDescriptorInfo*>::iterator it = open_fd_map_.begin();

  std::unordered_map<int, std::unique_ptr<FileDescriptorInfo>>::iterator it = open_fd_map_.begin();

  while (it != open_fd_map_.end()) {

  while (it != open_fd_map_.end()) {

    std::set<int>::const_iterator element = open_fds.find(it->first);

    std::set<int>::const_iterator element = open_fds.find(it->first);

    if (element == open_fds.end()) {

    if (element == open_fds.end()) {
@@ -580,7 +578,6 @@ void FileDescriptorTable::RestatInternal(std::set<int>& open_fds, fail_fn_t fail 
      // TODO(narayan): This will be an error in a future android release.

      // TODO(narayan): This will be an error in a future android release.

      // error = true;

      // error = true;

      // ALOGW("Zygote closed file descriptor %d.", it->first);

      // ALOGW("Zygote closed file descriptor %d.", it->first);

      delete it->second;

      it = open_fd_map_.erase(it);

      it = open_fd_map_.erase(it);

    } else {

    } else {

      // The entry from the file descriptor table is still open. Restat

      // The entry from the file descriptor table is still open. Restat
@@ -588,7 +585,6 @@ void FileDescriptorTable::RestatInternal(std::set<int>& open_fds, fail_fn_t fail 
      if (!it->second->RefersToSameFile()) {

      if (!it->second->RefersToSameFile()) {

        // The file descriptor refers to a different description. We must

        // The file descriptor refers to a different description. We must

        // update our entry in the table.

        // update our entry in the table.

        delete it->second;

        it->second = FileDescriptorInfo::CreateFromFd(*element, fail_fn);

        it->second = FileDescriptorInfo::CreateFromFd(*element, fail_fn);

      } else {

      } else {

        // It's the same file. Nothing to do here. Move on to the next open

        // It's the same file. Nothing to do here. Move on to the next open

core/jni/fd_utils.h

+3 −2
Original line number Original line Diff line number Diff line
@@ -17,6 +17,7 @@ 
#ifndef FRAMEWORKS_BASE_CORE_JNI_FD_UTILS_H_

#ifndef FRAMEWORKS_BASE_CORE_JNI_FD_UTILS_H_

#define FRAMEWORKS_BASE_CORE_JNI_FD_UTILS_H_

#define FRAMEWORKS_BASE_CORE_JNI_FD_UTILS_H_





#include <memory>

#include <set>

#include <set>

#include <string>

#include <string>

#include <unordered_map>

#include <unordered_map>
@@ -98,12 +99,12 @@ class FileDescriptorTable { 
  void ReopenOrDetach(fail_fn_t fail_fn);

  void ReopenOrDetach(fail_fn_t fail_fn);





 private:

 private:

  explicit FileDescriptorTable(const std::unordered_map<int, FileDescriptorInfo*>& map);

  explicit FileDescriptorTable(std::unordered_map<int, std::unique_ptr<FileDescriptorInfo>> map);





  void RestatInternal(std::set<int>& open_fds, fail_fn_t fail_fn);

  void RestatInternal(std::set<int>& open_fds, fail_fn_t fail_fn);





  // Invariant: All values in this unordered_map are non-NULL.

  // Invariant: All values in this unordered_map are non-NULL.

  std::unordered_map<int, FileDescriptorInfo*> open_fd_map_;

  std::unordered_map<int, std::unique_ptr<FileDescriptorInfo>> open_fd_map_;





  DISALLOW_COPY_AND_ASSIGN(FileDescriptorTable);

  DISALLOW_COPY_AND_ASSIGN(FileDescriptorTable);

};

};