add tests for x86

angr / patcherex

Shellphish's automated patching engine, originally created for the Cyber Grand Challenge.

BSD 2-Clause "Simplified" License

251 stars 45 forks source link

add tests for x86 #28

Closed DennyDai closed 4 years ago

DennyDai commented 4 years ago

Tested on Ubuntu 18.04 LTS, binary compiled with gcc 7.5.0.

*It runs properly on the local x86 machine, but segfault will appear when using qemu-i386 to run the patched binary file. **Add{RO, RW, RWInit}DataPatch will fail when adding data of length 10,000.

fix clang cmd

fix inconsistent types of variable compile_flags https://github.com/angr/patcherex/blob/feat/ezpz/patcherex/patches.py#L73 https://github.com/angr/patcherex/blob/feat/ezpz/patcherex/utils.py#L740

twizmwazin commented 4 years ago

Should binaries be added here, rather than in the binaries repo? I see that many are already here, but in the interest of not having our repos blow up to hundreds of megabytes, it might be preferable to commit them there.

Also, I see nose being used. It would be preferable to use standard-library unittest or python's built in asserts in place of nose, as we are hoping to eventually drop that dependency.

antoniobianchi333 commented 4 years ago

@twizmwazin so, would you move the binaries to angr/binary? I think it makes sense.

DennyDai commented 4 years ago

Also, I see nose being used. It would be preferable to use standard-library unittest or python's built in asserts in place of nose, as we are hoping to eventually drop that dependency.

@twizmwazin Thank you for the advise, I've replaced the uses of nose with python's built in asserts statement.

twizmwazin commented 4 years ago

@twizmwazin so, would you move the binaries to angr/binary? I think it makes sense.

I think moving binaries to the binaries repo would make sense, and would be consistent with other angr projects. To load them, we use the assumption that the binaries repo is cloned into the same folder patcherex is cloned into.

DennyDai commented 4 years ago

I've moved binaries to angr/binaries https://github.com/angr/binaries/pull/54

antoniobianchi333 commented 4 years ago

@twizmwazin do you think this is good to be merged now?

twizmwazin commented 4 years ago

A few notes:

There is a print statement in there that should be removed
Unless there is a reason to go against convention, test names should ideally be snake_case
The run_all function can likely be replaced with unittest.main()
pep8 dictates that there should be two newlines between module-level imports/functions/statements

With those minor tweaks, lgtm

DennyDai commented 4 years ago

@twizmwazin Thank you for your review. I've fixed them.