widevine-partner/android
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Refactor file_store to use smart pointers

Browse Source 
Bug: b/119276649
Merge from: http://go/wvgerrit/66367
Test: Android, CE CDM, Linux unit tests

The FileSystem interface as it exists expects an Open for a file and
then a Close when finished. However, the Close doesn't delete the file
itself and depending on the platform, the underlying impl_ as well,
leading to a memory leak. To fix this leak as well as harden against
future memory issues, this change refactors the interface to shift away
from raw pointers and towards smart pointers.

Change-Id: I7a7132ea95cd3775796a540f510b698f4f27dd24
This commit is contained in:
Srujan Gaddam
2018-11-14 16:59:00 -08:00
parent 5d360abd4b
commit 896ce2b5aa
15 changed files with 335 additions and 362 deletions
Download Patch File Download Diff File Expand all files Collapse all files

107
libwvdrmengine/cdm/util/src/file_store.cpp
View File

@@ -14,6 +14,8 @@
#include <sys/stat.h>
#include <sys/types.h>
#include <unistd.h>
#include <cstring>
#include <memory>
#include "file_utils.h"
#include "log.h"
@@ -60,65 +62,70 @@ std::string GetFileNameForIdentifier(const std::string path,
}
} // namespace
class File::Impl {
class FileImpl : public File {
public:
Impl(FILE* file, const std::string& file_path)
FileImpl(FILE* file, const std::string& file_path)
: file_(file), file_path_(file_path) {}
virtual ~Impl() {}
void FlushFile() {
fflush(file_);
fsync(fileno(file_));
}
~FileImpl() {
if (file_) {
FlushFile();
fclose(file_);
file_ = nullptr;
}
}
ssize_t Read(char* buffer, size_t bytes) override {
if (!buffer) {
LOGW("File::Read: buffer is empty");
return -1;
}
if (!file_) {
LOGW("File::Read: file not open");
return -1;
}
size_t len = fread(buffer, sizeof(char), bytes, file_);
if (len != bytes) {
LOGW("File::Read: fread failed: %d, %s", errno, strerror(errno));
}
return len;
}
ssize_t Write(const char* buffer, size_t bytes) override {
if (!buffer) {
LOGW("File::Write: buffer is empty");
return -1;
}
if (!file_) {
LOGW("File::Write: file not open");
return -1;
}
size_t len = fwrite(buffer, sizeof(char), bytes, file_);
if (len != bytes) {
LOGW("File::Write: fwrite failed: %d, %s", errno, strerror(errno));
}
FlushFile();
return len;
}
FILE* file_;
std::string file_path_;
};
File::File(Impl* impl) : impl_(impl) {}
File::~File() {
Close();
delete impl_;
}
void File::Close() {
if (impl_ && impl_->file_) {
fflush(impl_->file_);
fsync(fileno(impl_->file_));
fclose(impl_->file_);
impl_->file_ = NULL;
}
}
ssize_t File::Read(char* buffer, size_t bytes) {
if (impl_ && impl_->file_) {
size_t len = fread(buffer, sizeof(char), bytes, impl_->file_);
if (len == 0) {
LOGW("File::Read: fread failed: %d", errno);
}
return len;
}
LOGW("File::Read: file not open");
return -1;
}
ssize_t File::Write(const char* buffer, size_t bytes) {
if (impl_ && impl_->file_) {
size_t len = fwrite(buffer, sizeof(char), bytes, impl_->file_);
if (len == 0) {
LOGW("File::Write: fwrite failed: %d", errno);
}
return len;
}
LOGW("File::Write: file not open");
return -1;
}
class FileSystem::Impl {};
FileSystem::FileSystem() : FileSystem(EMPTY_ORIGIN, NULL) {}
FileSystem::FileSystem() : FileSystem(EMPTY_ORIGIN, nullptr) {}
FileSystem::FileSystem(const std::string& origin, void* /* extra_data */)
: origin_(origin) {}
FileSystem::~FileSystem() {}
File* FileSystem::Open(const std::string& in_name, int flags) {
std::unique_ptr<File> FileSystem::Open(const std::string& in_name, int flags) {
std::string open_flags;
std::string name = GetFileNameForIdentifier(in_name, identifier_);
@@ -146,11 +153,11 @@ File* FileSystem::Open(const std::string& in_name, int flags) {
FILE* file = fopen(name.c_str(), open_flags.c_str());
umask(old_mask);
if (!file) {
LOGW("File::Open: fopen failed: %d", errno);
return NULL;
LOGW("File::Open: fopen failed: %d, %s", errno, strerror(errno));
return nullptr;
}
return new File(new File::Impl(file, name));
return std::unique_ptr<File>(new FileImpl(file, name));
}
bool FileSystem::Exists(const std::string& path) {
@@ -175,9 +182,9 @@ bool FileSystem::List(const std::string& path,
return FileUtils::List(GetFileNameForIdentifier(path, origin_), filenames);
}
void FileSystem::SetOrigin(const std::string& origin) { origin_ = origin; }
void FileSystem::set_origin(const std::string& origin) { origin_ = origin; }
void FileSystem::SetIdentifier(const std::string& identifier) {
void FileSystem::set_identifier(const std::string& identifier) {
identifier_ = identifier;
}

32
libwvdrmengine/cdm/util/src/file_utils.cpp
View File

@@ -12,6 +12,7 @@
#include <sys/stat.h>
#include <sys/types.h>
#include <unistd.h>
#include <cstring>
#include "file_store.h"
#include "log.h"
@@ -28,7 +29,7 @@ bool FileUtils::Exists(const std::string& path) {
struct stat buf;
int res = stat(path.c_str(), &buf) == 0;
if (!res) {
LOGV("File::Exists: stat failed: %d", errno);
LOGV("File::Exists: stat failed: %d, %s", errno, strerror(errno));
}
return res;
}
@@ -53,7 +54,7 @@ bool FileUtils::Remove(const std::string& path) {
closedir(dir);
}
if (rmdir(path.c_str())) {
LOGW("File::Remove: rmdir failed: %d", errno);
LOGW("File::Remove: rmdir failed: %d, %s", errno, strerror(errno));
return false;
}
return true;
@@ -62,7 +63,7 @@ bool FileUtils::Remove(const std::string& path) {
if (wildcard_pos == std::string::npos) {
// Handle file deletion
if (unlink(path.c_str()) && (errno != ENOENT)) {
LOGW("File::Remove: unlink failed: %d", errno);
LOGW("File::Remove: unlink failed: %d, %s", errno, strerror(errno));
return false;
}
} else {
@@ -106,19 +107,22 @@ bool FileUtils::Remove(const std::string& path) {
bool FileUtils::Copy(const std::string& src, const std::string& dest) {
struct stat stat_buf;
if (stat(src.c_str(), &stat_buf)) {
LOGV("File::Copy: file %s stat error: %d", src.c_str(), errno);
LOGV("File::Copy: file %s stat error: %d, %s", src.c_str(), errno,
strerror(errno));
return false;
}
int fd_src = open(src.c_str(), O_RDONLY);
if (fd_src < 0) {
LOGW("File::Copy: unable to open file %s: %d", src.c_str(), errno);
LOGW("File::Copy: unable to open file %s: %d, %s", src.c_str(), errno,
strerror(errno));
return false;
}
int fd_dest = open(dest.c_str(), O_WRONLY | O_CREAT, stat_buf.st_mode);
if (fd_dest < 0) {
LOGW("File::Copy: unable to open file %s: %d", dest.c_str(), errno);
LOGW("File::Copy: unable to open file %s: %d, %s", dest.c_str(), errno,
strerror(errno));
close(fd_src);
return false;
}
@@ -126,8 +130,8 @@ bool FileUtils::Copy(const std::string& src, const std::string& dest) {
off_t offset = 0;
bool status = true;
if (sendfile(fd_dest, fd_src, &offset, stat_buf.st_size) < 0) {
LOGV("File::Copy: unable to copy %s to %s: %d", src.c_str(), dest.c_str(),
errno);
LOGV("File::Copy: unable to copy %s to %s: %d, %s", src.c_str(),
dest.c_str(), errno, strerror(errno));
status = false;
}
@@ -143,13 +147,15 @@ bool FileUtils::List(const std::string& path, std::vector<std::string>* files) {
}
if (!FileUtils::Exists(path)) {
LOGV("File::List: path %s does not exist: %d", path.c_str(), errno);
LOGV("File::List: path %s does not exist: %d, %s", path.c_str(), errno,
strerror(errno));
return false;
}
DIR* dir = opendir(path.c_str());
if (dir == NULL) {
LOGW("File::List: unable to open directory %s: %d", path.c_str(), errno);
LOGW("File::List: unable to open directory %s: %d, %s", path.c_str(), errno,
strerror(errno));
return false;
}
@@ -193,7 +199,8 @@ bool FileUtils::CreateDirectory(const std::string& path_in) {
path[pos] = '\0';
if (mkdir(path.c_str(), 0700) != 0) {
if (errno != EEXIST) {
LOGW("File::CreateDirectory: mkdir failed: %d\n", errno);
LOGW("File::CreateDirectory: mkdir failed: %d, %s\n", errno,
strerror(errno));
return false;
}
}
@@ -204,7 +211,8 @@ bool FileUtils::CreateDirectory(const std::string& path_in) {
if (path[size - 1] != kDirectoryDelimiter) {
if (mkdir(path.c_str(), 0700) != 0) {
if (errno != EEXIST) {
LOGW("File::CreateDirectory: mkdir failed: %d\n", errno);
LOGW("File::CreateDirectory: mkdir failed: %d, %s\n", errno,
strerror(errno));
return false;
}
}