Plugins: Determine correct pointer size automatically, without the need of specific headers and variables #3262

davidebeatrici · 2017-11-11T23:25:37Z

No description provided.

hacst

Makes the code quite a bit more convenient. Not exactly fond of the linux way of checking for 64bit but that probably can't be helped if we want this feature.

See comments.

hacst · 2017-11-14T19:37:21Z

plugins/mumble_plugin_linux.h

@@ -23,7 +19,8 @@
 #include "mumble_plugin.h"

 pid_t pPid;
-static PTR_TYPE_CONCRETE pModule;
+bool is64Bit;


There is no bool in C

I think we should create an enum.

Actually... no. It's better if we change the variable to an integer, otherwise we can't use the inline if statement.

hacst · 2017-11-14T19:39:56Z

plugins/mumble_plugin_linux.h

@@ -23,7 +19,8 @@
 #include "mumble_plugin.h"

 pid_t pPid;
-static PTR_TYPE_CONCRETE pModule;
+bool is64Bit;
+static procptr_t pModule;


Not sure why this is static to be honest. We only include it in one TU and having it multiple times would almost always lead to mistakes as you'd have to initialize is separately from each TU. Without static at least the linker would complain. Any ideas?

I asked myself too why this is static, but I decided to left it as it was.

hacst · 2017-11-14T20:01:40Z

plugins/mumble_plugin_linux.h

+	std::ifstream ifs;
+	ifs.open(ss.str(), std::ifstream::binary);
+
+	std::string elf;


istreams have a read method that does what you want (http://www.cplusplus.com/reference/istream/istream/read/):

char elf[5]; ifs.read(elf, 5);

Don't forget to check whether the stream is ok after than.

hacst · 2017-11-14T20:03:59Z

plugins/mumble_plugin_linux.h

+	ifs.close();
+
+	if (!(elf.at(0) == 0x7f && elf.at(1) == 'E' && elf.at(2) == 'L' && elf.at(3) == 'F')) {
+		return -1;


Does -1 indicate failure to identify then? Please document the function.

hacst · 2017-11-14T20:10:11Z

plugins/mumble_plugin_win32_ptr_type.h

-	if (!hProcess) {
+	int result = checkProcessIs64Bit(hProcess);
+	if (result == -1) {
+		return false;


Leaking hProcess?

Yes, I think it's better if we create a dedicated handle in checkProcessIs64Bit(), so that the global one needs only PROCESS_VM_READ.

That just gives us another handle that we mustn't leak ;) I would just close the handle instead of leaking it in the error cases.

Of course, but in addition to that it's better to use a dedicated handle for IsWow64Process(), as it needs a specific permission which we use only one time.

hacst · 2017-11-14T20:10:42Z

plugins/mumble_plugin_win32_ptr_type.h

+	is64Bit = result != 0;
+
+	pModule=getModuleAddr(modname ? modname : procname);
+	if (!pModule) {
 		dwPid = 0;


Leaking hProcess?

Yes, I'm going to move it above OpenProcess().

hacst · 2017-11-14T20:15:21Z

plugins/mumble_plugin_linux.h

+	// The first 4 bytes constitute the magical number in ASCII: 0x7F 45 4c 46.
+
+	std::stringstream ss;
+	ss << std::string("/proc/");


Why the std::string wrapping? Just causes a needless alloc. Pretty sure there's a const char* overload.

I took the code from getModuleAddr(), which was written by @mkrautz.

How would you do it?

ss << "/proc/";

Should work I think.

Just tried, it works.

hacst · 2017-11-14T20:18:05Z

plugins/mumble_plugin_linux.h

-T peekProc(PTR_TYPE base) {
-	T v = 0;
-	if (!peekProc(base, reinterpret_cast<T *>(&v), sizeof(T))) {
+procptr_t peekProcPtr(procptr_t base) {


Should document that this relies on initialize.

In a new pull request, as this one is focused on the automatic pointer size, not on cleaning up.

The dependency was introduced by making it rely in the 64bit check so imho documenting that belongs into the PR

No, every peekProc() function relies on initialize().

hacst · 2017-11-15T21:57:38Z

plugins/mumble_plugin_linux.h

+	std::ifstream ifs;
+	ifs.open(ss.str(), std::ifstream::binary);
+	if (ifs.is_open()) {
+		ifs.read(elf, 5);


Missing check for failure. I guess in this case we could just not check and instead make sure elf is zero initialized. Bad habit to get into though.

hacst · 2017-11-15T21:58:46Z

plugins/mumble_plugin_linux.h

+	char elf[5];
+
+	std::stringstream ss;
+	ss << std::string("/proc/");


ss << "/proc/";

hacst · 2017-11-15T21:58:52Z

plugins/mumble_plugin_linux.h

+	std::stringstream ss;
+	ss << std::string("/proc/");
+	ss << static_cast<unsigned long>(pid);
+	ss << std::string("/exe");


hacst · 2017-11-15T22:01:31Z

plugins/mumble_plugin_win32_ptr_type.h

+
+	BOOL isWow64Process;
+	if (!IsWow64Process(handle, &isWow64Process)) {
+		return -1;


leaking handle

Kissaki · 2017-11-18T12:22:22Z

plugins/mumble_plugin_linux.h

+	std::ifstream ifs;
+	ifs.open(ss.str(), std::ifstream::binary);
+	if (ifs.is_open()) {
+		ifs.read(elf, 5);


Should sizeof elf. I believe we have a define for array sizeof somewhere.

Kissaki · 2017-11-18T12:23:35Z

plugins/mumble_plugin_linux.h

@@ -23,7 +19,8 @@
 #include "mumble_plugin.h"

 pid_t pPid;
-static PTR_TYPE_CONCRETE pModule;
+int is64Bit;


Named like a bool. Should be a bool.

Yes, it should, but C doesn't have a boolean data type, as pointed out by @hacst.

Kissaki · 2017-11-18T12:26:59Z

plugins/mumble_plugin_win32_ptr_type.h

-static inline PTR_TYPE_CONCRETE getModuleAddr(DWORD pid, const wchar_t *modname) {
+static inline int checkProcessIs64Bit(const DWORD pid)
+{
+	// This function returns 0 if the process is 32-bit and 1 if it's 64-bit.


Should be a method comment!?

hacst

Apart from my minor comment LGTM. Please make sure to test on Windows and Linux but then I see no reason for you not to land it without another review iteration.

hacst · 2017-11-19T20:22:18Z

plugins/mumble_plugin_linux.h

+	ifs.read(elf, LENGTH_OF(elf));
+	ifs.close();
+
+	if (ifs.gcount() != 5) {


Should also use the macro.

…cific headers and variables This commit removes the architecture-specific headers, by keeping only the OS-specific ones. The different headers were needed to keep the legacy Windows header, after we created the "procptr32_t" (4 bytes) and "procptr64_t" (8 bytes) variables. We created these variables because the "peekProc" functions read as many bytes as the variable can hold. A pointer is 4 bytes on 32 bit platforms and 8 bytes on 64 bit ones. Now there's a new variable, called "procptr_t" and with a size of 8 bytes (unsigned long long). We had a "peekProc" template function which returned the value stored at the specified memory address, but it has been used only to read pointers so far. Since we needed a new function that checks the process architecture and sets the correct size of the memory to read, I decided to "recycle" it.

This commit removes the legacy header, with the new one taking its name.

davidebeatrici added client positional audio labels Nov 11, 2017

davidebeatrici force-pushed the plugins-automatic-architecture-detection branch 3 times, most recently from 7bb33c2 to a601e52 Compare November 12, 2017 22:08

davidebeatrici changed the title ~~Plugins: automatic process architecture detection~~ Plugins: Determine correct pointer size automatically, without the need of specific headers and variables Nov 12, 2017

davidebeatrici force-pushed the plugins-automatic-architecture-detection branch from a601e52 to 222298c Compare November 13, 2017 00:42

davidebeatrici requested review from mkrautz, Kissaki and hacst November 13, 2017 19:03

davidebeatrici force-pushed the plugins-automatic-architecture-detection branch 2 times, most recently from 4ad5944 to 755b853 Compare November 13, 2017 19:45

hacst suggested changes Nov 14, 2017

View reviewed changes

davidebeatrici force-pushed the plugins-automatic-architecture-detection branch 2 times, most recently from ad7f74e to ce1df33 Compare November 14, 2017 21:58

hacst suggested changes Nov 15, 2017

View reviewed changes

Kissaki requested changes Nov 18, 2017

View reviewed changes

hacst approved these changes Nov 19, 2017

View reviewed changes

davidebeatrici added 3 commits November 19, 2017 21:48

Replace legacy header with the new one

f397c40

This commit removes the legacy header, with the new one taking its name.

Accomplish changes in all the plugins

2a51c7b

davidebeatrici force-pushed the plugins-automatic-architecture-detection branch from 0656e1f to 2a51c7b Compare November 19, 2017 20:49

davidebeatrici merged commit fba1d65 into mumble-voip:master Nov 19, 2017

davidebeatrici mentioned this pull request Sep 1, 2022

Does not build on i586 (GTAV plugin fails) #5849

Closed

Plugins: Determine correct pointer size automatically, without the need of specific headers and variables #3262

Plugins: Determine correct pointer size automatically, without the need of specific headers and variables #3262

Conversation

davidebeatrici commented Nov 11, 2017

hacst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hacst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment