-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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.
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
yara-4.3.0-rc1 test-pe and test-dotnet fail for the s390x platform #1855
Comments
@xambroz Could you paste the logs produced by the failing tests? The logs should be in the |
Hello Victor @plusvic
|
PR #1768 added RVA field to function details in PE module. The new code had the following line: ``` rva_address = yr_le64toh(import_descriptor->FirstThunk + (sizeof(uint64_t) * func_idx)); ``` The `yr_le64toh` should be used for converting the value of `import_descriptor->FirstThunk` from little-endian to the host's endianness *before* performing the add operation. However, the addition was performed before the conversion. This may be the cause of some test cases failing in big endian platforms.
Hi @xambroz, I think I've found the cause of at least one of the issues. Could you try the branch https://github.com/VirusTotal/yara/tree/fix_1855 and let me know if fails in the s390x platform? I think it may fix at lest the Fixing |
Hi @plusvic , I have used https://github.com/VirusTotal/yara/commit/90c43e24f0dedd130bea199e6c23094271c3f491.patch as a patch to 4.3.0 rc1. Full build log:
Seems OK for the other (all little-endian) platforms: |
I've updated https://github.com/VirusTotal/yara/tree/fix_1855 with a new change. It turns out that the issue I fixed before was replicated at some other part of the code. My first attempt fixed it for 64-bits PE files only, but now I've fixed it for 32-bits PE as well. Can you give it a second try? |
Cool ... test-pe failed, but there is progress, it is failing in another test.
Full log Other (little-endian) platforms unaffected. |
Not all issues has been fixed with 2b631d0. The remaining issues are going to be harder to debug. My guess is that they are related to the difference in endianness. @xambroz could you disable the test in |
Sorry for no reply ... I will test |
@xambroz could you disable the test in test-pe.c line 739 and run the test cases again. I'm interested in knowing all the tests in that must be disabled in s390x for the test suite to pass. Yep .. fedora granted me interactive access to the s390x machine I can walk through manually. |
So I took last commit 32ae80d as a base everything else except this test will pass. |
With the latest changes in bdd3980 this issue should be completely fixed. |
tested, works OK. Thank you. |
Describe the bug
Hello,
I just wanted to let you know that the test-pe and test-dotnet fail for the s390x platform (Big-Endian).
https://kojipkgs.fedoraproject.org/…log
There is probably something in the code, which is big-endian/little-endian specific and fails on the other architecture.
Same build for the x64 and others went fine for this 4.3.0 rc1 .
https://koji.fedoraproject.org/…987
Best regards
Michal Ambroz
To Reproduce
Steps to reproduce the behavior:
On the s390x platform run the checks
make check
Expected behavior
All code should deal with binaries in architecture independent way, and all test should pass.
Screenshots
Please complete the following information:
The text was updated successfully, but these errors were encountered: