Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Fix local files with UTF8 names #1246

Merged
merged 1 commit into from
Jul 18, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion packager/app/mpd_generator.cc
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
// https://developers.google.com/open-source/licenses/bsd

#include <iostream>
#include <locale>

#include "packager/app/mpd_generator_flags.h"
#include "packager/app/vlog_flags.h"
Expand All @@ -20,7 +21,6 @@
#if defined(OS_WIN)
#include <codecvt>
#include <functional>
#include <locale>
#endif // defined(OS_WIN)

DEFINE_bool(licenses, false, "Dump licenses.");
Expand Down Expand Up @@ -142,12 +142,20 @@ int wmain(int argc, wchar_t* argv[], wchar_t* envp[]) {
delete[] utf8_args;
});
std::wstring_convert<std::codecvt_utf8<wchar_t>> converter;

for (int idx = 0; idx < argc; ++idx) {
std::string utf8_arg(converter.to_bytes(argv[idx]));
utf8_arg += '\0';
utf8_argv[idx] = new char[utf8_arg.size()];
memcpy(utf8_argv[idx], &utf8_arg[0], utf8_arg.size());
}

// Because we just converted wide character args into UTF8, and because
// std::filesystem::u8path is used to interpret all std::string paths as
// UTF8, we should set the locale to UTF8 as well, for the transition point
// to C library functions like fopen to work correctly with non-ASCII paths.
std::setlocale(LC_ALL, ".UTF8");

return shaka::MpdMain(argc, utf8_argv.get());
}
#else
Expand Down
10 changes: 9 additions & 1 deletion packager/app/packager_main.cc
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

#include <gflags/gflags.h>
#include <iostream>
#include <locale>

#include "packager/app/ad_cue_generator_flags.h"
#include "packager/app/crypto_flags.h"
Expand Down Expand Up @@ -34,7 +35,6 @@
#if defined(OS_WIN)
#include <codecvt>
#include <functional>
#include <locale>
#endif // defined(OS_WIN)

DEFINE_bool(dump_stream_info, false, "Dump demuxed stream info.");
Expand Down Expand Up @@ -575,12 +575,20 @@ int wmain(int argc, wchar_t* argv[], wchar_t* envp[]) {
delete[] utf8_args;
});
std::wstring_convert<std::codecvt_utf8<wchar_t>> converter;

for (int idx = 0; idx < argc; ++idx) {
std::string utf8_arg(converter.to_bytes(argv[idx]));
utf8_arg += '\0';
utf8_argv[idx] = new char[utf8_arg.size()];
memcpy(utf8_argv[idx], &utf8_arg[0], utf8_arg.size());
}

// Because we just converted wide character args into UTF8, and because
// std::filesystem::u8path is used to interpret all std::string paths as
// UTF8, we should set the locale to UTF8 as well, for the transition point
// to C library functions like fopen to work correctly with non-ASCII paths.
std::setlocale(LC_ALL, ".UTF8");

return shaka::PackagerMain(argc, utf8_argv.get());
}
#else
Expand Down
10 changes: 6 additions & 4 deletions packager/file/file.cc
Original file line number Diff line number Diff line change
Expand Up @@ -73,8 +73,8 @@ bool DeleteLocalFile(const char* file_name) {

bool WriteLocalFileAtomically(const char* file_name,
const std::string& contents) {
const std::filesystem::path file_path(file_name);
const std::filesystem::path dir_path = file_path.parent_path();
const auto file_path = std::filesystem::u8path(file_name);
const auto dir_path = file_path.parent_path();

std::string temp_file_name;
if (!TempFilePath(dir_path.string(), &temp_file_name))
Expand All @@ -83,7 +83,8 @@ bool WriteLocalFileAtomically(const char* file_name,
return false;

std::error_code ec;
std::filesystem::rename(temp_file_name, file_name, ec);
auto temp_file_path = std::filesystem::u8path(temp_file_name);
std::filesystem::rename(temp_file_path, file_name, ec);
if (ec) {
LOG(ERROR) << "Failed to replace file '" << file_name << "' with '"
<< temp_file_name << "', error: " << ec;
Expand Down Expand Up @@ -405,7 +406,8 @@ bool File::IsLocalRegularFile(const char* file_name) {
return false;

std::error_code ec;
return std::filesystem::is_regular_file(real_file_name, ec);
auto real_file_path = std::filesystem::u8path(real_file_name);
return std::filesystem::is_regular_file(real_file_path, ec);
}

std::string File::MakeCallbackFileName(
Expand Down
8 changes: 4 additions & 4 deletions packager/file/file_test_util.cc
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@ std::string generate_unique_temp_path() {
// Generate a unique name for a temporary file, using standard library
// routines, to avoid a circular dependency on any of our own code for
// generating temporary files. The template must end in 6 X's.
std::filesystem::path temp_path_template =
(std::filesystem::temp_directory_path() / "packager-test.XXXXXX");
auto temp_path_template =
std::filesystem::temp_directory_path() / "packager-test.XXXXXX";
std::string temp_path_template_string = temp_path_template.string();
#if defined(OS_WIN)
// _mktemp will modify the string passed to it to reflect the generated name
Expand All @@ -36,15 +36,15 @@ std::string generate_unique_temp_path() {

void delete_file(const std::string& path) {
std::error_code ec;
std::filesystem::remove(path, ec);
std::filesystem::remove(std::filesystem::u8path(path), ec);
// Ignore errors.
}

TempFile::TempFile() : path_(generate_unique_temp_path()) {}

TempFile::~TempFile() {
std::error_code ec;
std::filesystem::remove(path_, ec);
std::filesystem::remove(std::filesystem::u8path(path_), ec);
// Ignore errors.
}

Expand Down
56 changes: 54 additions & 2 deletions packager/file/file_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
#include <sys/stat.h>

#include <filesystem>
#include <locale>

#include "absl/flags/declare.h"
#include "packager/file/file.h"
Expand All @@ -31,13 +32,14 @@ void WriteFile(const std::string& path, const std::string& data) {

void DeleteFile(const std::string& path) {
std::error_code ec;
std::filesystem::remove(path, ec);
std::filesystem::remove(std::filesystem::u8path(path), ec);
// Ignore errors.
}

int64_t FileSize(const std::string& path) {
std::error_code ec;
int64_t file_size = std::filesystem::file_size(path, ec);
int64_t file_size =
std::filesystem::file_size(std::filesystem::u8path(path), ec);
if (ec) {
return -1;
}
Expand Down Expand Up @@ -65,6 +67,17 @@ namespace shaka {

class LocalFileTest : public testing::Test {
protected:
static std::string original_locale_;

static void SetUpTestSuite() {
original_locale_ = setlocale(LC_ALL, NULL);
setlocale(LC_ALL, ".UTF8");
}

static void TearDownTestSuite() {
setlocale(LC_ALL, original_locale_.c_str());
}

void SetUp() override {
data_.resize(kDataSize);
for (int i = 0; i < kDataSize; ++i)
Expand Down Expand Up @@ -97,6 +110,9 @@ class LocalFileTest : public testing::Test {
std::string local_file_name_;
};

// static
std::string LocalFileTest::original_locale_;

TEST_F(LocalFileTest, ReadNotExist) {
// Remove test file if it exists.
DeleteFile(local_file_name_no_prefix_);
Expand Down Expand Up @@ -233,6 +249,42 @@ TEST_F(LocalFileTest, IsLocalRegular) {
ASSERT_TRUE(File::IsLocalRegularFile(local_file_name_.c_str()));
}

TEST_F(LocalFileTest, UnicodePath) {
// Delete the temp file already created.
DeleteFile(local_file_name_no_prefix_);

// Modify the local file name for this test to include non-ASCII characters.
// This is used in TearDown() to clean up the file we create in the test.
const std::string unicode_suffix = "από.txt";
local_file_name_ += unicode_suffix;
local_file_name_no_prefix_ += unicode_suffix;

// Write file using File API.
File* file = File::Open(local_file_name_.c_str(), "w");
ASSERT_TRUE(file != NULL);
EXPECT_EQ(kDataSize, file->Write(&data_[0], kDataSize));

// Check the size.
EXPECT_EQ(kDataSize, file->Size());
ASSERT_TRUE(file->Close());

// Open file using File API.
file = File::Open(local_file_name_.c_str(), "r");
ASSERT_TRUE(file != NULL);

// Read the entire file.
std::string read_data(kDataSize, 0);
EXPECT_EQ(kDataSize, file->Read(&read_data[0], kDataSize));

// Verify EOF.
uint8_t single_byte;
EXPECT_EQ(0, file->Read(&single_byte, sizeof(single_byte)));
ASSERT_TRUE(file->Close());

// Compare data written and read.
EXPECT_EQ(data_, read_data);
}

class ParamLocalFileTest : public LocalFileTest,
public ::testing::WithParamInterface<uint8_t> {};

Expand Down
2 changes: 1 addition & 1 deletion packager/file/file_util.cc
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ std::string TempFileName() {
} // namespace

bool TempFilePath(const std::string& temp_dir, std::string* temp_file_path) {
std::filesystem::path temp_dir_path(temp_dir);
auto temp_dir_path = std::filesystem::u8path(temp_dir);
*temp_file_path = (temp_dir_path / TempFileName()).string();
return true;
}
Expand Down
8 changes: 5 additions & 3 deletions packager/file/local_file.cc
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,8 @@ int64_t LocalFile::Size() {
}

std::error_code ec;
int64_t file_size = std::filesystem::file_size(file_name(), ec);
auto file_path = std::filesystem::u8path(file_name());
int64_t file_size = std::filesystem::file_size(file_path, ec);
if (ec) {
LOG(ERROR) << "Cannot get file size, error: " << ec;
return -1;
Expand Down Expand Up @@ -112,7 +113,7 @@ bool LocalFile::Tell(uint64_t* position) {
LocalFile::~LocalFile() {}

bool LocalFile::Open() {
std::filesystem::path file_path(file_name());
auto file_path = std::filesystem::u8path(file_name());

// Create upper level directories for write mode.
if (file_mode_.find("w") != std::string::npos) {
Expand All @@ -133,9 +134,10 @@ bool LocalFile::Open() {
}

bool LocalFile::Delete(const char* file_name) {
auto file_path = std::filesystem::u8path(file_name);
std::error_code ec;
// On error (ec truthy), remove() will return false anyway.
return std::filesystem::remove(file_name, ec);
return std::filesystem::remove(file_path, ec);
}

} // namespace shaka
2 changes: 1 addition & 1 deletion packager/media/base/container_names_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ TEST(ContainerNamesTest, CheckFixedStrings) {

// Determine the container type of a specified file.
void TestFile(MediaContainerName expected, const std::string& name) {
std::filesystem::path path = GetTestDataFilePath(name);
auto path = GetTestDataFilePath(name);
std::vector<uint8_t> data = ReadTestDataFile(name);
ASSERT_FALSE(data.empty());

Expand Down
6 changes: 3 additions & 3 deletions packager/media/test/test_data_util.cc
Original file line number Diff line number Diff line change
Expand Up @@ -11,21 +11,21 @@ namespace media {

// Returns a file path for a file in the media/test/data directory.
std::filesystem::path GetTestDataFilePath(const std::string& name) {
std::filesystem::path data_dir(TEST_DATA_DIR);
auto data_dir = std::filesystem::u8path(TEST_DATA_DIR);
return data_dir / name;
}

// Returns a file path for a file in the media/app/test/testdata directory.
std::filesystem::path GetAppTestDataFilePath(const std::string& name) {
std::filesystem::path data_dir(TEST_DATA_DIR);
auto data_dir = std::filesystem::u8path(TEST_DATA_DIR);
auto app_data_dir =
data_dir.parent_path().parent_path() / "app" / "test" / "testdata";
return app_data_dir / name;
}

// Reads a test file from media/test/data directory and returns its content.
std::vector<uint8_t> ReadTestDataFile(const std::string& name) {
std::filesystem::path path = GetTestDataFilePath(name);
auto path = GetTestDataFilePath(name);

FILE* f = fopen(path.string().c_str(), "rb");
if (!f) {
Expand Down