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.

Sign up for GitHub

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

Jump to bottom

Corruption check is complete #281

Merged
merged 14 commits into from
May 9, 2018
Merged

Corruption check is complete #281

merged 14 commits into from
May 9, 2018

Conversation

ladislav-zezula
Copy link
Contributor

No description provided.

Zezula Ladislav added 6 commits April 18, 2018 10:41
Intermediate commit
ffef28a
Merge branch 'master' into corruption_check
31fafae
FileInfo now supports loader error as if it was Windows loader
92b69d0 
Fixed long loading times on samples with extremely long import/export directories
Cosmetics
b5fda91 
4Spaces -> 1Tab
4spaces->1tab
6cb9ace
Final branch of pelib included
45aac50 
Re-ran regression tests for fileinfo
@metthal metthal self-requested a review April 23, 2018 12:59
metthal
metthal requested changes Apr 23, 2018
View reviewed changes
Copy link
Member

@metthal metthal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your PR. I have reviewed your changes and added few comments here and there. Just some minor cosmetic things. Otherwise LGTM.

deps/pelib/CMakeLists.txt Outdated
URL https://github.com/avast-tl/pelib/archive/e93eaa7c150f4608a5d02a67f5edc9e54456fe24.zip
URL_HASH SHA256=2ffd7e89451c980a1af6d24d4f6dfbb69a660b06ad5de44c481f6431e21de394
URL https://github.com/avast-tl/pelib/archive/19f9a34c0dd10657303e15f7a9d03de9d177e6be.zip
# URL https://github.com/avast-tl/pelib/archive/e93eaa7c150f4608a5d02a67f5edc9e54456fe24.zip
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, remove these lines which are commented out. Also please add URL_HASH with the hash of the new archive so CMake can check whether it downloaded correct archive.

src/fileinfo/file_information/file_information.h Outdated
@@ -37,7 +37,7 @@ class FileInformation
std::string endianness; ///< endianness
std::string manifest; ///< XML manifest
std::string compactManifest; ///< compact version of XML manifest
FileHeader header; ///< file header
FileHeader header; ///< file header
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spaces.

src/fileinfo/file_information/file_information.h Outdated
@@ -400,7 +400,8 @@ class FileInformation
std::string getNumberOfLoadedSegmentsStr(std::ios_base &(* format)(std::ios_base &)) const;
const LoadedSegment& getLoadedSegment(std::size_t index) const;
const std::string& getLoaderStatusMessage() const;
/// @}
const retdec::fileformat::LoaderErrorInfo & getLoaderErrorInfo() const;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spaces.

src/fileinfo/file_information/file_information.h Outdated
@@ -492,6 +493,7 @@ class FileInformation
void setSignatureVerified(bool verified);
void setLoadedBaseAddress(unsigned long long baseAddress);
void setLoaderStatusMessage(const std::string& statusMessage);
void setLoaderErrorInfo(const retdec::fileformat::LoaderErrorInfo & ldrErrInfo);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spaces.

src/fileinfo/file_information/file_information_types/loader_info.h Outdated
public:
retdec::fileformat::LoaderErrorInfo _ldrErrInfo;

public:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing 1 TAB.

@ladislav-zezula
Copy link
Contributor Author

Fixed in commit 440ddde.

@ladislav-zezula
Copy link
Contributor Author

With commit ba7787d, I also fixed problems related to delay import loading and parsing.

metthal
metthal requested changes Apr 30, 2018
View reviewed changes
Copy link
Member

@metthal metthal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added 3 more comments after your latest changes. Two of them are just cosmetics but the issue with pelib/CMakeLists.txt needs to be taken care of. We just can't refer to git branches of other projects. If we somehow broke pelib in master branch tomorrow, retdec would be broken too without any change because CMake would download the most recent master.

deps/pelib/CMakeLists.txt Outdated
@@ -8,8 +8,7 @@ if(CMAKE_CXX_COMPILER)
endif()

ExternalProject_Add(pelib-project
URL https://github.com/avast-tl/pelib/archive/e93eaa7c150f4608a5d02a67f5edc9e54456fe24.zip
URL_HASH SHA256=2ffd7e89451c980a1af6d24d4f6dfbb69a660b06ad5de44c481f6431e21de394
URL https://github.com/avast-tl/pelib/archive/master.zip
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please do not use branches as reference points. Branches can change over time. Use only commit hashes or tags. Also please add missing URL_HASH.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in commit cab9451

include/retdec/fileformat/file_format/file_format.h Outdated
LoaderErrorInfo() : loaderErrorCode(0), loaderError(nullptr), loaderErrorUserFriendly(nullptr)
{}

std::uint32_t loaderErrorCode; // Loader error code, cast to uint32_t
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are tabs in-between name of the attribute and comment. Please use spaces here. We use tabs only for indentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in commit cab9451

include/retdec/fileformat/file_format/file_format.h Outdated
@@ -67,6 +81,7 @@ class FileFormat : public retdec::utils::ByteValueStorage, private retdec::utils
PdbInfo *pdbInfo; ///< information about related PDB debug file
CertificateTable *certificateTable; ///< table of certificates
Format fileFormat; ///< format of input file
LoaderErrorInfo _ldrErrInfo; ///< loader error (e.g. Windows loader error for PE files)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are tabs in-between name of the attribute and comment. Please use spaces here. We use tabs only for indentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in commit cab9451

@metthal metthal merged commit 7138895 into master May 9, 2018
@metthal metthal deleted the corruption_check branch May 10, 2018 08:47
@s3rvac s3rvac mentioned this pull request May 10, 2018
Closed
This was referenced May 27, 2018
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@metthal metthal metthal approved these changes

Assignees

@metthal metthal

Labels
C-fileinfo new-feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ladislav-zezula @metthal @s3rvac