Code review (#3114)

* Fix always-true conditions in sceNp module * gl_render_targets: useless check on unsigned variable, possible bug * fixed UB in crypto utility functions * copy-paste error in vk::init_default_resources * pass strings by const ref * Dont copy vectors. Make sure copies are not needed because functions are used in a multi-threaded context.
RPCS3 · Aug 1, 2017 · 607d248 · 607d248
1 parent 77f5832
commit 607d248
Show file tree

Hide file tree

Showing 16 changed files with 32 additions and 32 deletions.
diff --git a/Utilities/StrFmt.cpp b/Utilities/StrFmt.cpp
@@ -257,8 +257,8 @@ struct fmt::cfmt_src
 
 	void skip(std::size_t extra)
 	{
-		++sup += extra;
-		++args += extra;
+		sup += extra + 1;
+		args += extra + 1;
 	}
 
 	std::size_t fmt_string(std::string& out, std::size_t extra) const

diff --git a/rpcs3/Crypto/unedat.cpp b/rpcs3/Crypto/unedat.cpp
@@ -150,8 +150,8 @@ s64 decrypt_block(const fs::file* in, u8* out, EDAT_HEADER *edat, NPD_HEADER *np
 	const int metadata_section_size = ((edat->flags & EDAT_COMPRESSED_FLAG) != 0 || (edat->flags & EDAT_FLAG_0x20) != 0) ? 0x20 : 0x10;
 	const int metadata_offset = 0x100;
 
-	std::unique_ptr<u8> enc_data;
-	std::unique_ptr<u8> dec_data;
+	std::unique_ptr<u8[]> enc_data;
+	std::unique_ptr<u8[]> dec_data;
 	u8 hash[0x10] = { 0 };
 	u8 key_result[0x10] = { 0 };
 	u8 hash_result[0x14] = { 0 };
@@ -315,7 +315,7 @@ int decrypt_data(const fs::file* in, const fs::file* out, EDAT_HEADER *edat, NPD
 {
 	const int total_blocks = (int)((edat->file_size + edat->block_size - 1) / edat->block_size);
 	u64 size_left = (int)edat->file_size;
-	std::unique_ptr<u8> data(new u8[edat->block_size]);
+	std::unique_ptr<u8[]> data(new u8[edat->block_size]);
 
 	for (int i = 0; i < total_blocks; i++)
 	{
@@ -427,8 +427,8 @@ int check_data(unsigned char *key, EDAT_HEADER *edat, NPD_HEADER *npd, const fs:
 
 	long bytes_read = 0;
 	long bytes_to_read = metadata_size;
-	std::unique_ptr<u8> metadata(new u8[metadata_size]);
-	std::unique_ptr<u8> empty_metadata(new u8[metadata_size]);
+	std::unique_ptr<u8[]> metadata(new u8[metadata_size]);
+	std::unique_ptr<u8[]> empty_metadata(new u8[metadata_size]);
 
 	while (bytes_to_read > 0)
 	{
@@ -487,7 +487,7 @@ int check_data(unsigned char *key, EDAT_HEADER *edat, NPD_HEADER *npd, const fs:
 			if ((edat->flags & EDAT_FLAG_0x20) != 0) //Sony failed again, they used buffer from 0x100 with half size of real metadata.
 			{
 				int metadata_buf_size = block_num * 0x10;
-				std::unique_ptr<u8> metadata_buf(new u8[metadata_buf_size]);
+				std::unique_ptr<u8[]> metadata_buf(new u8[metadata_buf_size]);
 				f->seek(file_offset + metadata_offset);
 				f->read(metadata_buf.get(), metadata_buf_size);
 				sha1(metadata_buf.get(), metadata_buf_size, signature_hash);
@@ -516,7 +516,7 @@ int check_data(unsigned char *key, EDAT_HEADER *edat, NPD_HEADER *npd, const fs:
 		{
 			// Setup header signature hash.
 			memset(signature_hash, 0, 20);
-			std::unique_ptr<u8> header_buf(new u8[0xD8]);
+			std::unique_ptr<u8[]> header_buf(new u8[0xD8]);
 			f->seek(file_offset);
 			f->read(header_buf.get(), 0xD8);
 			sha1(header_buf.get(), 0xD8, signature_hash );
@@ -577,7 +577,7 @@ int validate_npd_hashes(const char* file_name, const u8* klicensee, NPD_HEADER *
 	int dev_hash_result = 0;
 
 	const int file_name_length = (int) strlen(file_name);
-	std::unique_ptr<u8> buf(new u8[0x30 + file_name_length]);
+	std::unique_ptr<u8[]> buf(new u8[0x30 + file_name_length]);
 
 	// Build the title buffer (content_id + file_name).
 	memcpy(buf.get(), npd->content_id, 0x30);

diff --git a/rpcs3/Crypto/utils.cpp b/rpcs3/Crypto/utils.cpp
@@ -118,7 +118,7 @@ void aesecb128_encrypt(unsigned char *key, unsigned char *in, unsigned char *out
 
 bool hmac_hash_compare(unsigned char *key, int key_len, unsigned char *in, int in_len, unsigned char *hash, int hash_len)
 {
-	std::unique_ptr<u8> out(new u8[key_len]);
+	std::unique_ptr<u8[]> out(new u8[key_len]);
 
 	sha1_hmac(key, key_len, in, in_len, out.get());
 
@@ -132,7 +132,7 @@ void hmac_hash_forge(unsigned char *key, int key_len, unsigned char *in, int in_
 
 bool cmac_hash_compare(unsigned char *key, int key_len, unsigned char *in, int in_len, unsigned char *hash, int hash_len)
 {
-	std::unique_ptr<u8> out(new u8[key_len]);
+	std::unique_ptr<u8[]> out(new u8[key_len]);
 
 	aes_context ctx;
 	aes_setkey_enc(&ctx, key, 128);

diff --git a/rpcs3/Emu/Cell/Modules/cellL10n.cpp b/rpcs3/Emu/Cell/Modules/cellL10n.cpp
@@ -161,7 +161,7 @@ bool _L10nCodeParse(s32 code, HostCode& retCode)
 #ifdef _MSC_VER
 
 // Use code page to transform std::string to std::wstring.
-s32 _OEM2Wide(HostCode oem_code, const std::string src, std::wstring& dst)
+s32 _OEM2Wide(HostCode oem_code, const std::string& src, std::wstring& dst)
 {
 	//Such length returned should include the '\0' character.
 	s32 length = MultiByteToWideChar(oem_code, 0, src.c_str(), -1, NULL, 0);
@@ -178,7 +178,7 @@ s32 _OEM2Wide(HostCode oem_code, const std::string src, std::wstring& dst)
 }
 
 // Use Code page to transform std::wstring to std::string.
-s32 _Wide2OEM(HostCode oem_code, const std::wstring src, std::string& dst)
+s32 _Wide2OEM(HostCode oem_code, const std::wstring& src, std::string& dst)
 {
 	//Such length returned should include the '\0' character.
 	s32 length = WideCharToMultiByte(oem_code, 0, src.c_str(), -1, NULL, 0, NULL, NULL);
@@ -195,7 +195,7 @@ s32 _Wide2OEM(HostCode oem_code, const std::wstring src, std::string& dst)
 }
 
 // Convert Codepage to Codepage (all char*)
-std::string _OemToOem(HostCode src_code, HostCode dst_code, const std::string str)
+std::string _OemToOem(HostCode src_code, HostCode dst_code, const std::string& str)
 {
 	std::wstring wide; std::string result;
 	_OEM2Wide(src_code, str, wide);

diff --git a/rpcs3/Emu/Cell/Modules/sceNp.cpp b/rpcs3/Emu/Cell/Modules/sceNp.cpp
@@ -972,7 +972,7 @@ s32 sceNpManagerGetOnlineId(vm::ptr<SceNpOnlineId> onlineId)
 		return SCE_NP_ERROR_OFFLINE;
 	}
 
-	if (g_psn_connection_status != SCE_NP_MANAGER_STATUS_LOGGING_IN || g_psn_connection_status != SCE_NP_MANAGER_STATUS_ONLINE)
+	if (g_psn_connection_status != SCE_NP_MANAGER_STATUS_LOGGING_IN && g_psn_connection_status != SCE_NP_MANAGER_STATUS_ONLINE)
 	{
 		return SCE_NP_ERROR_INVALID_STATE;
 	}
@@ -994,7 +994,7 @@ s32 sceNpManagerGetNpId(ppu_thread& ppu, vm::ptr<SceNpId> npId)
 		return SCE_NP_ERROR_OFFLINE;
 	}
 
-	if (g_psn_connection_status != SCE_NP_MANAGER_STATUS_LOGGING_IN || g_psn_connection_status != SCE_NP_MANAGER_STATUS_ONLINE)
+	if (g_psn_connection_status != SCE_NP_MANAGER_STATUS_LOGGING_IN && g_psn_connection_status != SCE_NP_MANAGER_STATUS_ONLINE)
 	{
 		return SCE_NP_ERROR_INVALID_STATE;
 	}
@@ -1082,7 +1082,7 @@ s32 sceNpManagerGetAccountRegion(vm::ptr<SceNpCountryCode> countryCode, vm::ptr<
 		return SCE_NP_ERROR_OFFLINE;
 	}
 
-	if (g_psn_connection_status != SCE_NP_MANAGER_STATUS_LOGGING_IN || g_psn_connection_status != SCE_NP_MANAGER_STATUS_ONLINE)
+	if (g_psn_connection_status != SCE_NP_MANAGER_STATUS_LOGGING_IN && g_psn_connection_status != SCE_NP_MANAGER_STATUS_ONLINE)
 	{
 		return SCE_NP_ERROR_INVALID_STATE;
 	}
@@ -1104,7 +1104,7 @@ s32 sceNpManagerGetAccountAge(vm::ptr<s32> age)
 		return SCE_NP_ERROR_OFFLINE;
 	}
 
-	if (g_psn_connection_status != SCE_NP_MANAGER_STATUS_LOGGING_IN || g_psn_connection_status != SCE_NP_MANAGER_STATUS_ONLINE)
+	if (g_psn_connection_status != SCE_NP_MANAGER_STATUS_LOGGING_IN && g_psn_connection_status != SCE_NP_MANAGER_STATUS_ONLINE)
 	{
 		return SCE_NP_ERROR_INVALID_STATE;
 	}
@@ -1126,7 +1126,7 @@ s32 sceNpManagerGetContentRatingFlag(vm::ptr<s32> isRestricted, vm::ptr<s32> age
 		return SCE_NP_ERROR_OFFLINE;
 	}
 
-	if (g_psn_connection_status != SCE_NP_MANAGER_STATUS_LOGGING_IN || g_psn_connection_status != SCE_NP_MANAGER_STATUS_ONLINE)
+	if (g_psn_connection_status != SCE_NP_MANAGER_STATUS_LOGGING_IN && g_psn_connection_status != SCE_NP_MANAGER_STATUS_ONLINE)
 	{
 		return SCE_NP_ERROR_INVALID_STATE;
 	}
@@ -1152,7 +1152,7 @@ s32 sceNpManagerGetChatRestrictionFlag(vm::ptr<s32> isRestricted)
 		return SCE_NP_ERROR_OFFLINE;
 	}
 
-	if (g_psn_connection_status != SCE_NP_MANAGER_STATUS_LOGGING_IN || g_psn_connection_status != SCE_NP_MANAGER_STATUS_ONLINE)
+	if (g_psn_connection_status != SCE_NP_MANAGER_STATUS_LOGGING_IN && g_psn_connection_status != SCE_NP_MANAGER_STATUS_ONLINE)
 	{
 		return SCE_NP_ERROR_INVALID_STATE;
 	}

diff --git a/rpcs3/Emu/Cell/Modules/sys_libc_.cpp b/rpcs3/Emu/Cell/Modules/sys_libc_.cpp
@@ -31,7 +31,7 @@ struct ps3_fmt_src
 
 	void skip(std::size_t extra)
 	{
-		++g_count += (u32)extra;
+		g_count += (u32)extra + 1;
 	}
 
 	std::size_t fmt_string(std::string& out, std::size_t extra) const

diff --git a/rpcs3/Emu/Cell/SPUASMJITRecompiler.cpp b/rpcs3/Emu/Cell/SPUASMJITRecompiler.cpp
@@ -166,7 +166,7 @@ void spu_recompiler::compile(spu_function_t& f)
 			dis_asm.dump_pc = m_pos;
 			dis_asm.disasm(m_pos);
 			compiler.comment(dis_asm.last_opcode.c_str());
-			log += dis_asm.last_opcode.c_str();
+			log += dis_asm.last_opcode;
 			log += '\n';
 		}
 

diff --git a/rpcs3/Emu/RSX/Common/VertexProgramDecompiler.cpp b/rpcs3/Emu/RSX/Common/VertexProgramDecompiler.cpp
@@ -373,7 +373,7 @@ void VertexProgramDecompiler::SetDSTSca(const std::string& code)
 	SetDST(true, code);
 }
 
-std::string VertexProgramDecompiler::NotZeroPositive(const std::string code)
+std::string VertexProgramDecompiler::NotZeroPositive(const std::string& code)
 {
 	return "max(" + code + ", 1.E-10)";
 }

diff --git a/rpcs3/Emu/RSX/Common/VertexProgramDecompiler.h b/rpcs3/Emu/RSX/Common/VertexProgramDecompiler.h
@@ -61,7 +61,7 @@ struct VertexProgramDecompiler
 	const std::vector<u32>& m_data;
 	ParamArray m_parr;
 
-	std::string NotZeroPositive(const std::string code);
+	std::string NotZeroPositive(const std::string& code);
 	std::string GetMask(bool is_sca);
 	std::string GetVecMask();
 	std::string GetScaMask();

diff --git a/rpcs3/Emu/RSX/D3D12/D3D12Buffer.cpp b/rpcs3/Emu/RSX/D3D12/D3D12Buffer.cpp
@@ -42,7 +42,7 @@ namespace
 		fmt::throw_exception("Wrong vector size %d" HERE, size);
 	}
 
-	u32 get_vertex_count(const std::vector<std::pair<u32, u32> > first_count_commands)
+	u32 get_vertex_count(const std::vector<std::pair<u32, u32> >& first_count_commands)
 	{
 		u32 vertex_count = 0;
 		for (const auto &pair : first_count_commands)

diff --git a/rpcs3/Emu/RSX/GL/GLRenderTargets.h b/rpcs3/Emu/RSX/GL/GLRenderTargets.h
@@ -340,7 +340,7 @@ class gl_render_targets : public rsx::surface_store<gl_render_target_traits>
 			return false;
 
 		u32 offset = texaddr - surface_address;
-		if (offset >= 0)
+		if (texaddr >= surface_address)
 		{
 			std::tie(is_subslice, x_offset, y_offset) = surface->get_texture_subresource(offset, scale_to_fit);
 			if (is_subslice)

diff --git a/rpcs3/Emu/RSX/GL/GLVertexBuffers.cpp b/rpcs3/Emu/RSX/GL/GLVertexBuffers.cpp
@@ -164,7 +164,7 @@ namespace
 		return std::make_tuple(element_count, mapping.second);
 	}
 
-	std::tuple<u32, u32, u32> upload_index_buffer(gsl::span<const gsl::byte> raw_index_buffer, void *ptr, rsx::index_array_type type, rsx::primitive_type draw_mode, const std::vector<std::pair<u32, u32>> first_count_commands, u32 initial_vertex_count)
+	std::tuple<u32, u32, u32> upload_index_buffer(gsl::span<const gsl::byte> raw_index_buffer, void *ptr, rsx::index_array_type type, rsx::primitive_type draw_mode, const std::vector<std::pair<u32, u32>>& first_count_commands, u32 initial_vertex_count)
 	{
 		u32 min_index, max_index, vertex_draw_count = initial_vertex_count;
 

diff --git a/rpcs3/Emu/RSX/RSXVertexProgram.h b/rpcs3/Emu/RSX/RSXVertexProgram.h
@@ -216,7 +216,7 @@ struct rsx_vertex_input
 	bool int_type;
 	u32 flags;		//Initially zero, to be optionally filled by the backend
 
-	bool operator==(const rsx_vertex_input other) const
+	bool operator==(const rsx_vertex_input& other) const
 	{
 		return location == other.location && size == other.size && frequency == other.frequency && is_modulo == other.is_modulo &&
 			is_array == other.is_array && int_type == other.int_type && flags == other.flags;

diff --git a/rpcs3/Emu/RSX/VK/VKCommonDecompiler.cpp b/rpcs3/Emu/RSX/VK/VKCommonDecompiler.cpp
@@ -163,7 +163,7 @@ namespace vk
 		rsc.maxFragmentUniformVectors = 16;
 		rsc.maxVertexOutputVectors = 16;
 		rsc.maxFragmentInputVectors = 15;
-		rsc.maxProgramTexelOffset = -8;
+		rsc.minProgramTexelOffset = -8;
 		rsc.maxProgramTexelOffset = 7;
 		rsc.maxClipDistances = 8;
 		rsc.maxComputeWorkGroupCountX = 65535;

diff --git a/rpcs3/Emu/RSX/VK/VKHelpers.h b/rpcs3/Emu/RSX/VK/VKHelpers.h
@@ -1463,6 +1463,6 @@ namespace vk
 	* dst_image must be in TRANSFER_DST_OPTIMAL layout and upload_buffer have TRANSFER_SRC_BIT usage flag.
 	*/
 	void copy_mipmaped_image_using_buffer(VkCommandBuffer cmd, VkImage dst_image,
-		const std::vector<rsx_subresource_layout> subresource_layout, int format, bool is_swizzled, u16 mipmap_count,
+		const std::vector<rsx_subresource_layout>& subresource_layout, int format, bool is_swizzled, u16 mipmap_count,
 		vk::vk_data_heap &upload_heap, vk::buffer* upload_buffer);
 }
diff --git a/rpcs3/Emu/RSX/VK/VKTexture.cpp b/rpcs3/Emu/RSX/VK/VKTexture.cpp
@@ -136,7 +136,7 @@ namespace vk
 	}
 
 	void copy_mipmaped_image_using_buffer(VkCommandBuffer cmd, VkImage dst_image,
-		const std::vector<rsx_subresource_layout> subresource_layout, int format, bool is_swizzled, u16 mipmap_count,
+		const std::vector<rsx_subresource_layout>& subresource_layout, int format, bool is_swizzled, u16 mipmap_count,
 		vk::vk_data_heap &upload_heap, vk::buffer* upload_buffer)
 	{
 		u32 mipmap_level = 0;