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

Add support to read zlib compressed files, like vmlinux.gz #103

Merged
merged 2 commits into from
Aug 12, 2024

Conversation

marcosps
Copy link
Collaborator

@marcosps marcosps commented Aug 6, 2024

This is useful for decompressing vmlinux files and kernel modules compressed using zlib/gzip.

Copy link
Collaborator

@giulianobelinassi giulianobelinassi left a comment

Choose a reason for hiding this comment

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

I am not particularly happy on how this is implemented. Let me elaborate:

  1. The magic number comparison could be in a separate function, either a static function in ElfObject class or a auxiliary function in NonLLVMMisc.cpp. Maybe even creating a separate class for this and return an enum with all known magic numbers.

  2. Be careful to initialize the file buffers correctly. read can fail in many ways, such as simple as it not being able to read the file because of permission errors.

  3. "Guessing" the deflated file size without any kind of fail mechanism is very dangerous. You should do it in a loop and call realloc if things go south.

  4. Make sure the provided library doesn't provide a way of stream-decompress the file without preloading the file on memory by yourself. This is usually much faster on today machines because of asynchronous disk operations: disk access is slow, CPU is blazing fast.

libcextract/ElfCXX.cpp Outdated Show resolved Hide resolved
libcextract/ElfCXX.cpp Outdated Show resolved Hide resolved
libcextract/ElfCXX.cpp Outdated Show resolved Hide resolved
libcextract/ElfCXX.cpp Outdated Show resolved Hide resolved
libcextract/ElfCXX.cpp Outdated Show resolved Hide resolved
libcextract/ElfCXX.cpp Outdated Show resolved Hide resolved
libcextract/ElfCXX.cpp Outdated Show resolved Hide resolved
libcextract/ElfCXX.cpp Outdated Show resolved Hide resolved
Copy link
Collaborator

@giulianobelinassi giulianobelinassi left a comment

Choose a reason for hiding this comment

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

There are minor things that needs to be solved.

libcextract/ElfCXX.cpp Outdated Show resolved Hide resolved
libcextract/ElfCXX.cpp Outdated Show resolved Hide resolved
libcextract/ElfCXX.cpp Show resolved Hide resolved

Elf *elf = elf_memory((char *)dest, ret);
if (elf == nullptr) {
free(dest);
Copy link
Collaborator

Choose a reason for hiding this comment

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

close(ElfFd).

Copy link
Collaborator

Choose a reason for hiding this comment

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

In this case the fd will leak. Once you decompressed the file you do not need the File Descriptor anymore. Furthermore, this function is called in ElfObject constructor. If this throws, then the destructor will not be called!

libcextract/ElfCXX.hh Outdated Show resolved Hide resolved
libcextract/NonLLVMMisc.cpp Outdated Show resolved Hide resolved
libcextract/NonLLVMMisc.cpp Show resolved Hide resolved
libcextract/NonLLVMMisc.hh Outdated Show resolved Hide resolved
libcextract/Passes.cpp Outdated Show resolved Hide resolved
libcextract/SymbolExternalizer.cpp Outdated Show resolved Hide resolved
Copy link
Collaborator

@giulianobelinassi giulianobelinassi left a comment

Choose a reason for hiding this comment

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

PLease fix the last file descriptor leak

{

unsigned char buf[4] = { 0 };

Copy link
Collaborator

Choose a reason for hiding this comment

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

If the file descriptor is not in the beginning of file, this will fail.


Elf *elf = elf_memory((char *)dest, ret);
if (elf == nullptr) {
free(dest);
Copy link
Collaborator

Choose a reason for hiding this comment

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

In this case the fd will leak. Once you decompressed the file you do not need the File Descriptor anymore. Furthermore, this function is called in ElfObject constructor. If this throws, then the destructor will not be called!

This class will contain a static method to detect which file we are
dealing with w.r.t ELF files.

Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
This is useful for decompressing vmlinux files and kernel modules
compressed using zlib/gzip.

Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
@giulianobelinassi giulianobelinassi merged commit 34af51b into SUSE:main Aug 12, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants