Visual Basic Information Parsing, PCode detection #440

JakubPruzinec · 2018-12-13T12:13:42Z

To be completed.

JakubPruzinec · 2019-02-11T16:02:19Z

I've implemented extraction of some VB metada features:

project info
code info
object table dump
extern table dump
guids, COM clsids and other COM related info
and so on.

#138

metthal

I've added some comments into your changes. Please have a look at them.

metthal · 2019-02-18T16:54:43Z

include/retdec/fileformat/types/visual_basic/visual_basic_structures.h

+
+	}
+
+	std::size_t headerSize()


Shouldn't this be static?

metthal · 2019-02-18T16:56:14Z

include/retdec/fileformat/types/visual_basic/visual_basic_structures.h

+	std::uint32_t helpFileOffset;            ///< offset to the string containing name of the Help file
+	std::uint32_t projNameOffset;            ///< offset to the string containing project's name
+
+	VBHeader()


I believe it's dangerous to leave the member variables uninitialized.

metthal · 2019-02-18T16:58:04Z

include/retdec/utils/conversion.h

@@ -300,6 +300,7 @@ void double10ToDouble8(std::vector<unsigned char> &dest,

 unsigned short byteSwap16(unsigned short val);
 unsigned int byteSwap32(unsigned int val);
+unsigned long byteSwap64(unsigned long val);


unsigned long is not 64-bit type when using MSVC.

metthal · 2019-02-18T17:01:39Z

src/fileformat/file_format/pe/pe_format.cpp

+		return;
+	}
+
+	vbh.signature = *reinterpret_cast<std::uint32_t *>(&bytes.data()[offset]); offset += sizeof(vbh.signature);


This line is recurring in your changes. I was thinking whether it would be possible for you to either create a function that would do this or even better, move DynamicBuffer from src/unpacker to utils and use it. It basically does what you do but it abstracts away all the casting and also maintains endianess, so you don't have to do byte swaps afterwards.

metthal · 2019-02-18T17:03:14Z

src/fileinfo/file_information/file_information.cpp

@@ -781,6 +781,345 @@ bool FileInformation::hasRichHeaderRecords() const
 	return richHeader.hasRecords();
 }

+


Remove these whitespaces

metthal · 2019-02-18T17:04:33Z

src/utils/string.cpp

+	std::string result;
+	if (!bytes)
+	{
+		return "";


Replace "" with {}. It is much more efficient.

For more information, see e.g. https://stackoverflow.com/questions/26587110/returning-an-empty-string-efficient-way-in-c

metthal · 2019-02-18T17:10:47Z

src/utils/string.cpp

+		}
+		else
+		{
+			const std::size_t maxC = std::pow(2, sizeof(std::string::value_type) * CHAR_BIT) - 1;


If you are doing power of 2 then it's much better to use bit shift rather than std::pow().

metthal

Thank you for taking your time and going through our comments. I just have a few other things. One is more serious and that is using namespace in the header files on multiple places. That is considered a bad practice. The other comment is just about the way you calculate the power of 2, because it's unusual that it is calculated from 2 and not from 1.

metthal · 2019-02-23T20:59:56Z

include/retdec/unpacker/signature.h

-#include "retdec/unpacker/dynamic_buffer.h"
+#include "retdec/utils/dynamic_buffer.h"
+
+using namespace retdec::utils;


Do not put using namespace in header files. By including this file, you bring the whole namespace into all included files and this can cause you a headache or two.

More details (in Czech): https://cs-blog.petrzemek.net/2013-05-25-zajimavosti-z-cpp-proc-nedavat-using-do-hlavickovych-souboru

metthal · 2019-02-23T21:01:09Z

src/unpackertool/plugins/mpress/mpress.h

 #include "retdec/unpacker/plugin.h"

 #define mpress_plugin plugin(retdec::unpackertool::mpress::MpressPlugin)

+using namespace retdec::utils;
