[libcamera-devel,1/2] libcamera: lib_loader: add shared library loader

1183 diff mbox series

Implement a class to load a struct IPAModuleInfo of a given symbol name
from a .so shared object library.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
---
I will remove and clean up the comments in lib_loader.h, but I've left
them there for now, although I would like comments on the API (and the
commented out unload()).

I will also add logging later, as well as checking (guessing) if the
bitness is acceptable or not.

The generics to allow duck-typing of the ELF structs was the best I
could think of with my limited C++ knowledge of how to deal with the
different size and field layouts of the 32-bit and 64-bit structs.

 src/libcamera/include/lib_loader.h |  59 ++++++++++
 src/libcamera/lib_loader.cpp       | 175 +++++++++++++++++++++++++++++
 src/libcamera/meson.build          |   2 +
 3 files changed, 236 insertions(+)
 create mode 100644 src/libcamera/include/lib_loader.h
 create mode 100644 src/libcamera/lib_loader.cpp

Hi Paul,

Thank you for the patch.

On Fri, May 10, 2019 at 07:22:34PM -0400, Paul Elder wrote:
> Implement a class to load a struct IPAModuleInfo of a given symbol name
> from a .so shared object library.
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> ---
> I will remove and clean up the comments in lib_loader.h, but I've left
> them there for now, although I would like comments on the API (and the
> commented out unload()).
> 
> I will also add logging later, as well as checking (guessing) if the
> bitness is acceptable or not.

As long as later means in a new version of this series, before it gets
merged, sure :-) I'll already comment on what is below.

> The generics to allow duck-typing of the ELF structs was the best I
> could think of with my limited C++ knowledge of how to deal with the
> different size and field layouts of the 32-bit and 64-bit structs.
> 
>  src/libcamera/include/lib_loader.h |  59 ++++++++++
>  src/libcamera/lib_loader.cpp       | 175 +++++++++++++++++++++++++++++
>  src/libcamera/meson.build          |   2 +
>  3 files changed, 236 insertions(+)
>  create mode 100644 src/libcamera/include/lib_loader.h
>  create mode 100644 src/libcamera/lib_loader.cpp
> 
> diff --git a/src/libcamera/include/lib_loader.h b/src/libcamera/include/lib_loader.h
> new file mode 100644
> index 0000000..d8eb100
> --- /dev/null
> +++ b/src/libcamera/include/lib_loader.h
> @@ -0,0 +1,59 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * lib_loader.h - load shared libraries at runtime
> + */
> +#ifndef __LIBCAMERA_LIB_LOADER_H__
> +#define __LIBCAMERA_LIB_LOADER_H__
> +
> +#include <string>
> +
> +namespace libcamera {
> +
> +class LibLoader

We don't foresee right now the need for a secure .so loader other than
for the IPA modules. I would thus name this IPAModule (and rename files
and comments accordingly).

> +{
> +public:
> +	struct IPAModuleInfo {
> +		char name[256];
> +		unsigned int version;
> +	};

You can move this structure definition out of the class. I foresee the
need to split it to a separate file that will be included by IPA module
implementations, while the IPAModule class is a wrapper around the .so
used internally in libcamera only and should not be exposed to IPA
implementations (which may be closed source).

> +
> +	// doesn't actually do anything?

We use C-style comments in libcamera.

You seem puzzled by why the constructor doesn't do much. Constructor in
C++ can only return an error through exceptions. We don't use exceptions
for various reasons, so a constructor can never fail. This leads to two
possible implementations:

- A constructor that just stores data internally, and an init()-like
  function that then performs all the initialisation.

- A constructor that performs full initialisation, and an isValid()-like
  method that then checks if the initialisation was successful.

Both patterns are valid.

> +	explicit LibLoader(const std::string &lib_path);

camelCase for variable names. I know you'll get used to it after we
hammer it down :-)

Good use of the explicit keyword by the way.

> +
> +	// if you try it while it's already loaded it'll just load it again,
> +	// no big deal

Possibly no big deal, but it's not very efficient. How about loading the
module information in the constructor instead and removing this method ?

> +	/* returns 0 on success, err code on failure */

Documentation goes to the .cpp file. There's plenty of examples in
libcamera :-)

> +	int loadIPAModuleInfo(const std::string &symbol);
> +
> +	bool isLoaded() const;
> +
> +	// just a getter (or returns nullptr if module hasn't been loaded)
> +	struct IPAModuleInfo getIPAModuleInfo() const;

How do you return nullptr when you return a structure, not a pointer ?
:-) You should return a const reference to the info_ member, not a copy.
The caller can then decide to make a copy if needed (for instance if it
needs to modify the structure before passing it to another function), or
just use the reference if it only needs to inspect the data. This is a
usual pattern to save copy operations (references have a cost similar
to pointers).

Note that our getters don't start with a get prefix (except in a few
cases when the get prefix is part of an existing vocabulary, such as
getFormat() and setFormat() for V4L2 devices).

> +
> +	// necessary to allow multiple users of an instance of LibLoader
> +	/* returns 0 on success, err code on failure, pos num of others loading the lib */
> +	//int unload();

Why do you think this is needed ?

> +
> +	//void (*)() resolve(const std::string &symbol);
> +	// resolve(const std::string &lib_path, const std::string symbol)
> +
> +private:
> +	struct IPAModuleInfo info_;
> +
> +	bool loaded_;
> +	std::string lib_path_;
> +
> +	int bitclass_;

camCase here too.

> +
> +	int loadElfIdent(int fd);

I find loadELFIdent more readable, but the coding style guide mentions
that acronyms are not fully capitalised. If you wrote Elf because of the
coding style guide, be informed that this would be an acceptable
exception in my opinion.

> +
> +	template<class elfHeader, class secHeader, class symHeader>
> +	int loadSymbol(struct IPAModuleInfo *dst, int fd,
> +		       const std::string &symbol);
> +};
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_LIB_LOADER_H__ */
> diff --git a/src/libcamera/lib_loader.cpp b/src/libcamera/lib_loader.cpp
> new file mode 100644
> index 0000000..d7788d6
> --- /dev/null
> +++ b/src/libcamera/lib_loader.cpp
> @@ -0,0 +1,175 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * lib_loader.cpp - load shared libraries at runtime
> + */
> +
> +#include "lib_loader.h"
> +
> +#include <elf.h>
> +#include <errno.h>
> +#include <fcntl.h>
> +#include <string.h>
> +#include <sys/mman.h>
> +#include <sys/stat.h>
> +#include <sys/types.h>
> +#include <unistd.h>
> +
> +#include <iostream>
> +
> +#include "log.h"
> +
> +namespace libcamera {
> +

Welcome to the joys of writing documentation :-) You need a \file,
\class and documentation for all public and protected members of the
class.

> +LOG_DECLARE_CATEGORY(LibLoader)
> +
> +LibLoader::LibLoader(const std::string &lib_path)
> +	: lib_path_(lib_path)
> +{
> +}
> +
> +int LibLoader::loadElfIdent(int fd)
> +{
> +	int ret = lseek(fd, 0, SEEK_SET);
> +	if (ret < 0)
> +		return errno;

The file has just been opened, is this needed ?

> +
> +	unsigned char e_ident[EI_NIDENT];

I would say camelCase here too, but as this is a term that comes from
the Elf(32|64)_Ehdr structure, it can be an exception.

> +	ret = read(fd, e_ident, EI_NIDENT);
> +	if (ret < 0)
> +		return errno;

This should be return -errno if you want a negative error code.

> +
> +	if (!(e_ident[EI_MAG0] == ELFMAG0 &&
> +	      e_ident[EI_MAG1] == ELFMAG1 &&
> +	      e_ident[EI_MAG2] == ELFMAG2 &&
> +	      e_ident[EI_MAG3] == ELFMAG3) ||
> +	    e_ident[EI_VERSION] != EV_CURRENT)

Wouldn't it be simplier to write

	if (e_ident[EI_MAG0] != ELFMAG0 ||
	    e_ident[EI_MAG1] != ELFMAG1 ||
	    e_ident[EI_MAG2] != ELFMAG2 ||
	    e_ident[EI_MAG3] != ELFMAG3 ||
	    e_ident[EI_VERSION] != EV_CURRENT)

> +		return -1;

Please use explicit error codes. Maybe -ENOEXEC here ?

> +
> +	if (e_ident[EI_CLASS] == ELFCLASS32 ||
> +	    e_ident[EI_CLASS] == ELFCLASS64)
> +		bitclass_ = e_ident[EI_CLASS];
> +	else
> +		return -1;
> +
> +	int a = 1;
> +	bool little_endian = *(char *)&a == 1;
> +	if (!(e_ident[EI_DATA] == ELFDATA2LSB ||
> +	      e_ident[EI_DATA] == ELFDATA2MSB) ||
> +	    !(e_ident[EI_DATA] == (little_endian ? ELFDATA2LSB : ELFDATA2MSB)))
> +		return -1;

This seems a bit complicated.

	int a = 1;
	unsigned char endianness = *(char *)&a == 1 ? ELFDATA2LSB : ELFDATA2MSB;
	if (e_ident[EI_DATA] != endianness)
		return -ENOEXEC;

But you should also use C++-style casts:

	int a = 1;
	unsigned char endianness = *static_cast<(char *>(&a) == 1
				 ? ELFDATA2LSB : ELFDATA2MSB;
	if (e_ident[EI_DATA] != endianness)
		return -ENOEXEC;

To avoid the cast, you could also write

	/* Verify that the ELF endianness matches the host's. */
	const union {
		uint32_t i;
		uint8_t c[4];
	} e = { .c = { ELFDATA2LSB, 0, 0, ELFDATA2MSB } };

	if (e_ident[EI_DATA] != e.i & 0xff)
		return -ENOEXEC;

> +
> +	return 0;
> +}
> +
> +template<class elfHeader, class secHeader, class symHeader>

Clever use of templates :-) I'm not sure I would have thought about it.

The class names should use CamelCase.

> +int LibLoader::loadSymbol(struct LibLoader::IPAModuleInfo *dst, int fd,
> +			  const std::string &symbol)

If you want to make this function generic as its name implies, it should
take a void pointer and a size for the destination memory:

	int fd, const char *symbol, void *data, size_t size

(note that I've replaced std::string with a char *, as you only use
symbol.c_str() here, and the caller passes a hardcoded string literal,
so there's no point in converting back and forth)

> +{
> +	unsigned long soSize = lseek(fd, 0, SEEK_END);

How about using stat() instead ?

> +	unsigned char *map = (unsigned char *)mmap(NULL, soSize, PROT_READ,
> +						   MAP_PRIVATE, fd, 0);

static_cast<unsigned char *>()

(or char * instead of unsigned char * as you only use map to do pointer
arithmetics). I'm afraid the same applies to all other casts below.

> +	if (map == MAP_FAILED)
> +		return errno;

	return -errno;

> +
> +	elfHeader *eHdr = (elfHeader *)map;

You will have to be a bit more careful than that, and verify that the
file size is big enough, here and below, to avoid overflows in case of a
corrupt (or even maliciously crafted) ELF file.

One option would be to create a function (or macro) that takes map,
offset, size and file size and return a pointer to map + offset if
offset + size < file size (with an appropriate overflow check on offset
+ size) and nullptr otherwise (behold the template magic):

template<typename T>
typename std::remove_extent<T>::type *elfPointer(void *map, off_t offset,
						 size_t fileSize)
{
	size_t size = offset + sizeof(T);
	if (size > fileSize || size < sizeof(T))
		return nullptr;

	return reinterpret_cast<typename std::remove_extent<T>::type *>
		(static_cast<char *>(map) + offset);
}

...

	elfHeader *eHdr = elfPointer<elfHeader>(map, 0, soSize);
	if (!eHdr)
		return -ENOEXEC;

	off_t offset = eHdr->e_shoff + eHdr->e_shentsize * eHdr->e_shstrndx;
	secHeader *sHdr = elfPointer<secHeader>(map, offset, soSize);
	if (!sHdr)
		return -ENOEXEC;

	off_t shnameoff = sHdr->sh_offset;

	/* Locate the .dynsym section. */
	bool found = false;
	secHeader *dynsym;
	for (unsigned int i = 0; i < eHdr->e_shnum; i++) {
		offset = eHdr->e_shoff + eHdr->e_shentsize * i;
		sHdr = elfPointer<secHeader>(map, offset, soSize);
		if (!sHdr)
			return -ENOEXEC;

		char *name = elfPointer<char[8]>(map, shnameoff + sHdr->sh_name,
						 soSize);
		if (!name)
			return -ENOEXEC;

		if (sHdr->sh_type == SHT_DYNSYM && !strcmp(name, ".dynsym")) {
			found = true;
			dynsym = sHdr;
			break;
		}
	}

and so on. Other options are possible too, but please care about both
core readability and safety.

> +
> +	unsigned long shnameoff =
> +		((secHeader *)(map + eHdr->e_shoff +
> +			       eHdr->e_shentsize * eHdr->e_shstrndx))
> +			->sh_offset;
> +
> +	// walk section header table, find .dynsym
> +	bool found = false;
> +	secHeader *dynsym;
> +	for (unsigned int i = 0; i < eHdr->e_shnum; i++) {
> +		unsigned long i_shoff = eHdr->e_shoff + eHdr->e_shentsize * i;
> +		secHeader *sHdr = (secHeader *)(map + i_shoff);
> +
> +		char *name = (char *)map + shnameoff + sHdr->sh_name;
> +
> +		if (sHdr->sh_type == SHT_DYNSYM && !strcmp(name, ".dynsym")) {
> +			found = true;
> +			dynsym = sHdr;
> +			break;
> +		}
> +	}
> +
> +	if (!found)
> +		return -1;
> +
> +	unsigned long dynsym_nameoff =
> +		((secHeader *)(map + eHdr->e_shoff +
> +			       eHdr->e_shentsize * dynsym->sh_link))
> +			->sh_offset;
> +
> +	// walk dynsym symbol table, find desired symbol
> +	found = false;
> +	symHeader *target_symbol;
> +	unsigned int dynsym_num = dynsym->sh_size / dynsym->sh_entsize;
> +	for (unsigned int i = 0; i < dynsym_num; i++) {
> +		unsigned long i_symoff = dynsym->sh_offset + dynsym->sh_entsize * i;
> +		symHeader *sym = (symHeader *)(map + i_symoff);
> +
> +		char *name = (char *)map + dynsym_nameoff + sym->st_name;
> +
> +		if (!strcmp(name, symbol.c_str()) &&
> +		    sym->st_info & STB_GLOBAL &&
> +		    sym->st_size == sizeof(struct IPAModuleInfo)) {
> +			found = true;
> +			target_symbol = sym;
> +			break;
> +		}
> +	}
> +
> +	if (!found)
> +		return -1;
> +
> +	secHeader *target_section =
> +		(secHeader *)(map + eHdr->e_shoff +
> +			      target_symbol->st_shndx * eHdr->e_shentsize);
> +	unsigned long final_addr = target_section->sh_offset +
> +				   (target_symbol->st_value - target_section->sh_addr);
> +	memcpy(dst, map + final_addr, sizeof(struct IPAModuleInfo));

munmap() is missing, both in the success and error cases. You could move
it to the caller to make error handling easier and pass the mapped
address and size instead of the fd. This would have the advantage of
making the mapped .so available to loadElfIdent() too.

> +
> +	return 0;
> +}
> +
> +int LibLoader::loadIPAModuleInfo(const std::string &symbol)
> +{
> +	int fd = open(lib_path_.c_str(), O_RDONLY);
> +	if (fd < 0)
> +		return fd;
> +
> +	int ret = loadElfIdent(fd);
> +	if (ret)
> +		goto close;
> +
> +	if (bitclass_ == ELFCLASS32)
> +		ret = loadSymbol<Elf32_Ehdr, Elf32_Shdr, Elf32_Sym>(&info_, fd, symbol);
> +	else if (bitclass_ == ELFCLASS64)
> +		ret = loadSymbol<Elf64_Ehdr, Elf64_Shdr, Elf64_Sym>(&info_, fd, symbol);
> +	else
> +		ret = -1;

This case is not possible as loadElfIdent() returns an error if the
bitclass is not valid.

> +	if (ret)
> +		goto close;
> +
> +	loaded_ = true;
> +
> +close:
> +	close(fd);
> +	return ret;
> +}
> +
> +bool LibLoader::isLoaded() const
> +{
> +	return loaded_;
> +}

I would turn this into an isValid() function that returns true if the
ELF file could be parsed successfully and the IPAModuleInfo loaded, and
false otherwise.

> +
> +struct LibLoader::IPAModuleInfo LibLoader::getIPAModuleInfo() const
> +{
> +	return info_;
> +}
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index 8796f49..4e7ab10 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -10,6 +10,7 @@ libcamera_sources = files([
>      'event_notifier.cpp',
>      'formats.cpp',
>      'geometry.cpp',
> +    'lib_loader.cpp',
>      'log.cpp',
>      'media_device.cpp',
>      'media_object.cpp',
> @@ -31,6 +32,7 @@ libcamera_headers = files([
>      'include/device_enumerator_udev.h',
>      'include/event_dispatcher_poll.h',
>      'include/formats.h',
> +    'include/lib_loader.h',
>      'include/log.h',
>      'include/media_device.h',
>      'include/media_object.h',