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.

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

libclamav/pe: Use endian wrapper in more places. #814

Merged
merged 1 commit into from
Mar 17, 2023

Conversation

sebastianas
Copy link
Contributor

A few user of VirtualAddress and Size in cli_exe_info::pe_image_data_dir don't use the endian wrapper while other places do. This leads to testsuite failures on big endian machines.

Use the endian wrapper in all places across pe.c for the two members.

Signed-off-by: Sebastian Andrzej Siewior sebastian@breakpoint.cc

@val-ms
Copy link
Contributor

val-ms commented Jan 26, 2023

This looks really good to me. Thank you @sebastianas! I will spend more time with it, testing, etc as soon as I am able.

@val-ms
Copy link
Contributor

val-ms commented Mar 9, 2023

This change makes sense to me. Thank you for tracking down the endianness issues and submitting this PR.

I almost feel like it would make more sense to do it when we populate the peinfo->dirs
here:

read = fmap_readn(map, peinfo->dirs, at, data_dirs_size);

in the same way that we do with peinfo->nsections, here:

peinfo->nsections = EC16(file_hdr->NumberOfSections);

or peinfo->e_lfanew, here:

peinfo->e_lfanew = EC32(peinfo->e_lfanew);

Just for consistency it feels better to assume that all fields in the struct cli_exe_info peinfo have been switched to the correct endianess.

If it's not too much trouble, can you change it to fix the endianness when populating peinfo->dirs? If you're swamped, I can give it a go and tack it on to this branch. You've contributed a whole ton recently.

Best,
Micah

A few user of VirtualAddress and Size in cli_exe_info::pe_image_data_dir
don't use the endian wrapper while other places do. This leads to
testsuite failures on big endian machines.

Convert the content of struct pe_image_data_dir to native format so that
that the EC32() conversation can be removed.

Signed-off-by: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
Copy link
Contributor

@val-ms val-ms left a comment

Choose a reason for hiding this comment

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

This looks great to me. I tried to be as thorough as I could in reviewing the code changes.
Testing went well (at least on the little endian devices we have in our pipelines). Will merge.

@val-ms val-ms merged commit d2a0bb8 into Cisco-Talos:main Mar 17, 2023
@sebastianas
Copy link
Contributor Author

sebastianas commented Mar 23, 2023 via email

@val-ms
Copy link
Contributor

val-ms commented Mar 23, 2023

Sure let's do that. But I did find that we messed up something and coverity is angry:
image

And for the other variable:
image

I had put in a ticket for us to try to solve that next week, but if you want to help you're welcome to.
When we get that fix we can cherry-pick both commits for PR targeting the dev/1.0.2 branch.

@val-ms val-ms added the 🍒cherry-pick-candidate A PR that should be backported once approved. label Mar 23, 2023
@sebastianas sebastianas deleted the big_endian branch June 27, 2024 19:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🍒cherry-pick-candidate A PR that should be backported once approved.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants