Removing the dependency on the TianoCompress tool

[yeggor]

Hi. In efi_decompress function decompressing is done with the TianoCompress tool:

BIOSUtilities/common/comp_efi.py

Line 41 in 6de50c4

subprocess.run([get_tiano_path(), '-d', in_path, '-o', out_path, '-q', comp_type], check=True, stdout=subprocess.DEVNULL)

It can easily be avoided by using efi_compressor.EfiDecompress from uefi-firmware-parser: https://github.com/theopolis/uefi-firmware-parser/blob/963ce5c8ab5a83719b2f6666d717ef405f818c2b/uefi_firmware/compression/EfiCompressor.c.

[yeggor]

Here is proposed changes: yeggor@3bd5d76

[platomav]

Hi,

Thank you for the suggestion. I reviewed the proposed EFI decompression alternative, and I don't think it's a better solution.

The originated code of uefi-firmware-parser's efi_decompress is also written in C (not pure python) and it seems to be some weird python-wrapped hybrid of sorts. That is problematic for multiple reasons.

It is not official, introduces much higher risk of problems due to its modifications, it is based on older TianoCompress source code and requires C compilation before usage. Basically, even installing uefi-firmware-parser via pip requires compiling that external C code, thus additional instructions and steps are needed for this repo as dependencies, which is not worth requesting from end users. It is especially annoying under Windows, which does not come with C compilers installed by default and the usual VisualStudio approach requires large downloads and gigabytes of disk usage.

EFI compression was weird up until some years ago because there was the original (broken I think) v1.0 and the proper UEFI v1.1 and no clear way to distinguish between the two when decompressing. Some tools would assume v1.0, others v1.1 and thus various implementations were causing confusion. You can see such issues mentioned at Chipsec for example and how they also switched to the official EDK2 tools, when these became available.

The official (EDK2) source code solved these issues by creating one utility which can handle both versions (with --uefi parameter for v1.1) and has been tested in the field for a few years now without problems (afaik). At least my own tests with EFI compressed samples for this repo, they have never failed with EDK2 TianoCompress, unlike some earlier implementations (e.g. UEFIRomExtract etc).

In general, there is no benefit in switching to a custom, older & untested implementation from a python (dependency) utility, which will also require C compilation upon its installation. As long as an official Windows binary exists and building it on linux is very simple (git clone https://github.com/tianocore/edk2.git, cd edk2/BaseTools/Source/C, make, install ./bin/TianoCompress /url/local/bin), I don't see the reason for switching to something else.

For me, an interesting alternative would have been a pure python implementation which is cross-OS compatible, but no such thing exists, so the current TianoCompress usage is reliable and generally good enough. Similarly, for LZNT1 compression, I use this nice pure python module because it is not Windows-only and does not require any external libraries/compiling. Its only issue is how slow it is, but that is an acceptable downside for the other two positives.

By the way, I'm in the process of re-factoring this repo, so I won't be merging MRs based on the current code, until I have the new one in a more ready/tested state at a separate branch. One change I want to implement for these external executables, is their detection from $PATH, when possible. So if you do an "install TianoCompress /usr/local/bin" or similar, it will find it there and not force you to place it at the "externals" folder.

[yeggor]

Thanks for the detailed reply!

I've tested the TianoCompress bindings from uefi-firmware on windows, linux and mac. It worked just fine. In addition, I've tested the modified AMI_UCP_Extract.py on several dozen samples. All the samples were unpacked correctly.

But I agree with you. In order to unpack a single file, using bindings instead of an executable does not bring significant performance or stability improvements. So it is probably best to leave it as it is.

[platomav]

Ah, these are c-python bindings, yeap, that makes sense now. Thanks for that tip and for testing the suggested MR changes beforehand. Based on our discussion, I'll go ahead and close this issue now. 😉