From d6e9b96e2f16dd7aa37e580e5c2b10c15fc82426 Mon Sep 17 00:00:00 2001 From: Lioncash Date: Thu, 19 Jul 2018 09:53:35 -0400 Subject: [PATCH 1/6] fsp_srv: Respect write length in Write() Previously we were just copying the data whole-sale, even if the length was less than the total data size. This effectively makes the actual_data vector useless, which is likely not intended. Instead, amend this to only copy the given length amount of data. At the same time, we can avoid zeroing out the data before using it by passing iterators to the constructor instead of a size. --- src/core/hle/service/filesystem/fsp_srv.cpp | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/core/hle/service/filesystem/fsp_srv.cpp b/src/core/hle/service/filesystem/fsp_srv.cpp index 1b003bd843..e3f237c5c7 100644 --- a/src/core/hle/service/filesystem/fsp_srv.cpp +++ b/src/core/hle/service/filesystem/fsp_srv.cpp @@ -3,6 +3,8 @@ // Refer to the license.txt file included. #include +#include + #include "common/logging/log.h" #include "common/string_util.h" #include "core/core.h" @@ -133,17 +135,16 @@ private: return; } - std::vector data = ctx.ReadBuffer(); - std::vector actual_data(length); + const std::vector data = ctx.ReadBuffer(); ASSERT_MSG( data.size() <= length, "Attempting to write more data than requested (requested={:016X}, actual={:016X}).", length, data.size()); - std::copy(data.begin(), data.end(), actual_data.begin()); // Write the data to the Storage backend - auto written = backend->WriteBytes(data, offset); + std::vector actual_data(data.begin(), data.begin() + length); + const auto written = backend->WriteBytes(std::move(actual_data), offset); ASSERT_MSG(written == length, "Could not write all bytes to file (requested={:016X}, actual={:016X}).", length, From 6be342118abbc68296c38b91b7558cbeb30ad4c0 Mon Sep 17 00:00:00 2001 From: Lioncash Date: Thu, 19 Jul 2018 09:57:00 -0400 Subject: [PATCH 2/6] fsp_srv: Resolve sign-mismatch warnings in assertion comparisons --- src/core/hle/service/filesystem/fsp_srv.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/core/hle/service/filesystem/fsp_srv.cpp b/src/core/hle/service/filesystem/fsp_srv.cpp index e3f237c5c7..c093c1183a 100644 --- a/src/core/hle/service/filesystem/fsp_srv.cpp +++ b/src/core/hle/service/filesystem/fsp_srv.cpp @@ -138,15 +138,15 @@ private: const std::vector data = ctx.ReadBuffer(); ASSERT_MSG( - data.size() <= length, + static_cast(data.size()) <= length, "Attempting to write more data than requested (requested={:016X}, actual={:016X}).", length, data.size()); // Write the data to the Storage backend std::vector actual_data(data.begin(), data.begin() + length); - const auto written = backend->WriteBytes(std::move(actual_data), offset); + const std::size_t written = backend->WriteBytes(std::move(actual_data), offset); - ASSERT_MSG(written == length, + ASSERT_MSG(static_cast(written) == length, "Could not write all bytes to file (requested={:016X}, actual={:016X}).", length, written); From 910ad2e11048f4d53751b556d6a2e3284909a892 Mon Sep 17 00:00:00 2001 From: Lioncash Date: Thu, 19 Jul 2018 10:03:15 -0400 Subject: [PATCH 3/6] fsp_srv: Add missing includes Gets rid of relying on indirect inclusions. --- src/core/hle/service/filesystem/fsp_srv.cpp | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/core/hle/service/filesystem/fsp_srv.cpp b/src/core/hle/service/filesystem/fsp_srv.cpp index c093c1183a..6ef022ce0c 100644 --- a/src/core/hle/service/filesystem/fsp_srv.cpp +++ b/src/core/hle/service/filesystem/fsp_srv.cpp @@ -3,8 +3,13 @@ // Refer to the license.txt file included. #include +#include +#include #include +#include +#include "common/assert.h" +#include "common/common_types.h" #include "common/logging/log.h" #include "common/string_util.h" #include "core/core.h" From f317080f406c940198debdcc2deb1669707742f1 Mon Sep 17 00:00:00 2001 From: Lioncash Date: Thu, 19 Jul 2018 10:04:13 -0400 Subject: [PATCH 4/6] fsp_srv: Make IStorage constructor explicit Prevents implicit conversions. --- src/core/hle/service/filesystem/fsp_srv.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/hle/service/filesystem/fsp_srv.cpp b/src/core/hle/service/filesystem/fsp_srv.cpp index 6ef022ce0c..4cb2d6909b 100644 --- a/src/core/hle/service/filesystem/fsp_srv.cpp +++ b/src/core/hle/service/filesystem/fsp_srv.cpp @@ -33,7 +33,7 @@ enum class StorageId : u8 { class IStorage final : public ServiceFramework { public: - IStorage(FileSys::VirtualFile backend_) + explicit IStorage(FileSys::VirtualFile backend_) : ServiceFramework("IStorage"), backend(std::move(backend_)) { static const FunctionInfo functions[] = { {0, &IStorage::Read, "Read"}, {1, nullptr, "Write"}, {2, nullptr, "Flush"}, From 3e9b79e088223459891ed07a747c411a6a8921d3 Mon Sep 17 00:00:00 2001 From: Lioncash Date: Thu, 19 Jul 2018 10:15:00 -0400 Subject: [PATCH 5/6] fsp_srv: Remove unnecessary std::vector construction in IDirectory's Read() function We were using a second std::vector as a buffer to convert another std::vector's data into a byte sequence, however we can just use pointers to the original data and use them directly with WriteBuffer, which avoids copying the data at all into a separate std::vector. We simply cast the pointers to u8* (which is allowed by the standard, given std::uint8_t is an alias for unsigned char on platforms that we support). --- src/core/hle/service/filesystem/fsp_srv.cpp | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/src/core/hle/service/filesystem/fsp_srv.cpp b/src/core/hle/service/filesystem/fsp_srv.cpp index 4cb2d6909b..673eaabf0d 100644 --- a/src/core/hle/service/filesystem/fsp_srv.cpp +++ b/src/core/hle/service/filesystem/fsp_srv.cpp @@ -4,6 +4,7 @@ #include #include +#include #include #include #include @@ -229,23 +230,20 @@ private: LOG_DEBUG(Service_FS, "called, unk=0x{:X}", unk); // Calculate how many entries we can fit in the output buffer - u64 count_entries = ctx.GetWriteBufferSize() / sizeof(FileSys::Entry); + const u64 count_entries = ctx.GetWriteBufferSize() / sizeof(FileSys::Entry); // Cap at total number of entries. - u64 actual_entries = std::min(count_entries, entries.size() - next_entry_index); + const u64 actual_entries = std::min(count_entries, entries.size() - next_entry_index); - // Read the data from the Directory backend - std::vector entry_data(entries.begin() + next_entry_index, - entries.begin() + next_entry_index + actual_entries); + // Determine data start and end + const auto* begin = reinterpret_cast(entries.data() + next_entry_index); + const auto* end = reinterpret_cast(entries.data() + next_entry_index + actual_entries); + const auto range_size = static_cast(std::distance(begin, end)); next_entry_index += actual_entries; - // Convert the data into a byte array - std::vector output(entry_data.size() * sizeof(FileSys::Entry)); - std::memcpy(output.data(), entry_data.data(), output.size()); - // Write the data to memory - ctx.WriteBuffer(output); + ctx.WriteBuffer(begin, range_size); IPC::ResponseBuilder rb{ctx, 4}; rb.Push(RESULT_SUCCESS); From 6c1ba02e0cacfd66cf36d40a4b0f9794bc2ca26e Mon Sep 17 00:00:00 2001 From: Lioncash Date: Thu, 19 Jul 2018 11:01:05 -0400 Subject: [PATCH 6/6] fsp_srv: Remove unnecessary vector construction in IFile's Write() function We can avoid constructing a std::vector here by simply passing a pointer to the original data and the size of the copy we wish to perform to the backend's Write() function instead, avoiding copying the data where it's otherwise not needed. --- src/core/hle/service/filesystem/fsp_srv.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/core/hle/service/filesystem/fsp_srv.cpp b/src/core/hle/service/filesystem/fsp_srv.cpp index 673eaabf0d..e7ffb6bd16 100644 --- a/src/core/hle/service/filesystem/fsp_srv.cpp +++ b/src/core/hle/service/filesystem/fsp_srv.cpp @@ -149,8 +149,9 @@ private: length, data.size()); // Write the data to the Storage backend - std::vector actual_data(data.begin(), data.begin() + length); - const std::size_t written = backend->WriteBytes(std::move(actual_data), offset); + const auto write_size = + static_cast(std::distance(data.begin(), data.begin() + length)); + const std::size_t written = backend->Write(data.data(), write_size, offset); ASSERT_MSG(static_cast(written) == length, "Could not write all bytes to file (requested={:016X}, actual={:016X}).", length,