airbus-seclab / cpu_rec

Recognize cpu instructions in an arbitrary binary file
Apache License 2.0
657 stars 60 forks source link

Improvements: reformat and modernize code base #18

Closed trou closed 9 months ago

trou commented 1 year ago

Style and reformatting:

Performance improvement:

trou commented 1 year ago

I know leaf is not implemented in python, but it's supported, and there are precompiled wheels. "pickle.load should never be done.", why ?

LRGH commented 1 year ago

Thank you Trou for this pull request, but I don't want to accept it as it is. Mainly because I don't like when there are constraints on the environment where the software is running.

Therefore I want to keep the possibility of using python2 and I don't want to depend on argparse which is not always installed with old python.

I like the idea of having the possibility to use lief if it is installed, but I want to keep the possibility of using elfesteem. I will probably give the priority to lief if both are installed.

I agree that performance improvements can be useful, and closing opened file is a good idea...

trou commented 1 year ago

Hello Louis,

Thank you Trou for this pull request, but I don't want to accept it as it is. Mainly because I don't like when there are constraints on the environment where the software is running.

Therefore I want to keep the possibility of using python2 and I don't want to depend on argparse which is not always installed with old python.

It's been included by default in python2.7. Do you have any example where there would be anyone wanting to run cpu_rec, which is only intended for interactive use, on an ancient distro where python 3 is not available?

I like the idea of having the possibility to use lief if it is installed, but I want to keep the possibility of using elfesteem. I will probably give the priority to lief if both are installed.

Does elfesteem support python3?

I agree that performance improvements can be useful, and closing opened file is a good idea...

trou commented 1 year ago

@KOLANICH please refrain from giving opinionated (and sometimes wrong) comments on projects you are not a developer. This is not helpful.

LRGH commented 1 year ago

It's been included by default in python2.7. Do you have any example where there would be anyone wanting to run cpu_rec, which is only intended for interactive use, on an ancient distro where python 3 is not available?

No, I don't have any example, because I don't have any example where there would be an old python available on the system where cpu_rec should analyse the files, and it would not be possible to copy those files to a more recent system with a recent python3. But I don't want to force people to use a recent python if it is not necessary. Very unusual contexts may happen. I don't remember why python 2.4 was the minimum version supported by cpu_rec, but I guess that it would have been too much work to make it available for python 2.3.

Does elfesteem support python3?

Yes. It supports python from 2.3, and the automatic testing (which I dont' run often, cf. https://github.com/LRGH/elfesteem/actions/runs/2616228164) runs against python 2.3, 2.7, 3.4, 3.9, 3.10. I did not update it for the most recent python3, but I don't see why it should not work. It may not work with python 3.0 to 3.3, but if I remember well, these versions were quite broken.

trou commented 1 year ago

You should somehow make it clear that the expected elfesteem version is yours, there are 2 others, which are not python3 compatible: serpilliere's and airbus-seclab's

LRGH commented 1 year ago

You should somehow make it clear that the expected elfesteem version is yours, there are 2 others, which are not python3 compatible: serpilliere's and airbus-seclab's

I thought that it was quite explicit, because I have linked this version. But indeed, some time ago I checked and it also works with the obsolete versions of elfesteem, therefore I don't want to add checks that makes it mandatory to use my version.

LRGH commented 9 months ago

I will soon add the option to use cpickle. I have created a github action for non regression and portability testing. It shows that cpu_rec is much faster with pypy than cpython or graalpy. I think that with pypy this optimization using cpickle loses its interest... but most people are using cpython.

LRGH commented 9 months ago

Done