[{"id":1595,"web_url":"https://patchwork.libcamera.org/comment/1595/","msgid":"<20190511174837.GA13043@pendragon.ideasonboard.com>","date":"2019-05-11T17:48:37","subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: lib_loader: add shared\n\tlibrary loader","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nThank you for the patch.\n\nOn Fri, May 10, 2019 at 07:22:34PM -0400, Paul Elder wrote:\n> Implement a class to load a struct IPAModuleInfo of a given symbol name\n> from a .so shared object library.\n> \n> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>\n> ---\n> I will remove and clean up the comments in lib_loader.h, but I've left\n> them there for now, although I would like comments on the API (and the\n> commented out unload()).\n> \n> I will also add logging later, as well as checking (guessing) if the\n> bitness is acceptable or not.\n\nAs long as later means in a new version of this series, before it gets\nmerged, sure :-) I'll already comment on what is below.\n\n> The generics to allow duck-typing of the ELF structs was the best I\n> could think of with my limited C++ knowledge of how to deal with the\n> different size and field layouts of the 32-bit and 64-bit structs.\n> \n>  src/libcamera/include/lib_loader.h |  59 ++++++++++\n>  src/libcamera/lib_loader.cpp       | 175 +++++++++++++++++++++++++++++\n>  src/libcamera/meson.build          |   2 +\n>  3 files changed, 236 insertions(+)\n>  create mode 100644 src/libcamera/include/lib_loader.h\n>  create mode 100644 src/libcamera/lib_loader.cpp\n> \n> diff --git a/src/libcamera/include/lib_loader.h b/src/libcamera/include/lib_loader.h\n> new file mode 100644\n> index 0000000..d8eb100\n> --- /dev/null\n> +++ b/src/libcamera/include/lib_loader.h\n> @@ -0,0 +1,59 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * lib_loader.h - load shared libraries at runtime\n> + */\n> +#ifndef __LIBCAMERA_LIB_LOADER_H__\n> +#define __LIBCAMERA_LIB_LOADER_H__\n> +\n> +#include <string>\n> +\n> +namespace libcamera {\n> +\n> +class LibLoader\n\nWe don't foresee right now the need for a secure .so loader other than\nfor the IPA modules. I would thus name this IPAModule (and rename files\nand comments accordingly).\n\n> +{\n> +public:\n> +\tstruct IPAModuleInfo {\n> +\t\tchar name[256];\n> +\t\tunsigned int version;\n> +\t};\n\nYou can move this structure definition out of the class. I foresee the\nneed to split it to a separate file that will be included by IPA module\nimplementations, while the IPAModule class is a wrapper around the .so\nused internally in libcamera only and should not be exposed to IPA\nimplementations (which may be closed source).\n\n> +\n> +\t// doesn't actually do anything?\n\nWe use C-style comments in libcamera.\n\nYou seem puzzled by why the constructor doesn't do much. Constructor in\nC++ can only return an error through exceptions. We don't use exceptions\nfor various reasons, so a constructor can never fail. This leads to two\npossible implementations:\n\n- A constructor that just stores data internally, and an init()-like\n  function that then performs all the initialisation.\n\n- A constructor that performs full initialisation, and an isValid()-like\n  method that then checks if the initialisation was successful.\n\nBoth patterns are valid.\n\n> +\texplicit LibLoader(const std::string &lib_path);\n\ncamelCase for variable names. I know you'll get used to it after we\nhammer it down :-)\n\nGood use of the explicit keyword by the way.\n\n> +\n> +\t// if you try it while it's already loaded it'll just load it again,\n> +\t// no big deal\n\nPossibly no big deal, but it's not very efficient. How about loading the\nmodule information in the constructor instead and removing this method ?\n\n> +\t/* returns 0 on success, err code on failure */\n\nDocumentation goes to the .cpp file. There's plenty of examples in\nlibcamera :-)\n\n> +\tint loadIPAModuleInfo(const std::string &symbol);\n> +\n> +\tbool isLoaded() const;\n> +\n> +\t// just a getter (or returns nullptr if module hasn't been loaded)\n> +\tstruct IPAModuleInfo getIPAModuleInfo() const;\n\nHow do you return nullptr when you return a structure, not a pointer ?\n:-) You should return a const reference to the info_ member, not a copy.\nThe caller can then decide to make a copy if needed (for instance if it\nneeds to modify the structure before passing it to another function), or\njust use the reference if it only needs to inspect the data. This is a\nusual pattern to save copy operations (references have a cost similar\nto pointers).\n\nNote that our getters don't start with a get prefix (except in a few\ncases when the get prefix is part of an existing vocabulary, such as\ngetFormat() and setFormat() for V4L2 devices).\n\n> +\n> +\t// necessary to allow multiple users of an instance of LibLoader\n> +\t/* returns 0 on success, err code on failure, pos num of others loading the lib */\n> +\t//int unload();\n\nWhy do you think this is needed ?\n\n> +\n> +\t//void (*)() resolve(const std::string &symbol);\n> +\t// resolve(const std::string &lib_path, const std::string symbol)\n> +\n> +private:\n> +\tstruct IPAModuleInfo info_;\n> +\n> +\tbool loaded_;\n> +\tstd::string lib_path_;\n> +\n> +\tint bitclass_;\n\ncamCase here too.\n\n> +\n> +\tint loadElfIdent(int fd);\n\nI find loadELFIdent more readable, but the coding style guide mentions\nthat acronyms are not fully capitalised. If you wrote Elf because of the\ncoding style guide, be informed that this would be an acceptable\nexception in my opinion.\n\n> +\n> +\ttemplate<class elfHeader, class secHeader, class symHeader>\n> +\tint loadSymbol(struct IPAModuleInfo *dst, int fd,\n> +\t\t       const std::string &symbol);\n> +};\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_LIB_LOADER_H__ */\n> diff --git a/src/libcamera/lib_loader.cpp b/src/libcamera/lib_loader.cpp\n> new file mode 100644\n> index 0000000..d7788d6\n> --- /dev/null\n> +++ b/src/libcamera/lib_loader.cpp\n> @@ -0,0 +1,175 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * lib_loader.cpp - load shared libraries at runtime\n> + */\n> +\n> +#include \"lib_loader.h\"\n> +\n> +#include <elf.h>\n> +#include <errno.h>\n> +#include <fcntl.h>\n> +#include <string.h>\n> +#include <sys/mman.h>\n> +#include <sys/stat.h>\n> +#include <sys/types.h>\n> +#include <unistd.h>\n> +\n> +#include <iostream>\n> +\n> +#include \"log.h\"\n> +\n> +namespace libcamera {\n> +\n\nWelcome to the joys of writing documentation :-) You need a \\file,\n\\class and documentation for all public and protected members of the\nclass.\n\n> +LOG_DECLARE_CATEGORY(LibLoader)\n> +\n> +LibLoader::LibLoader(const std::string &lib_path)\n> +\t: lib_path_(lib_path)\n> +{\n> +}\n> +\n> +int LibLoader::loadElfIdent(int fd)\n> +{\n> +\tint ret = lseek(fd, 0, SEEK_SET);\n> +\tif (ret < 0)\n> +\t\treturn errno;\n\nThe file has just been opened, is this needed ?\n\n> +\n> +\tunsigned char e_ident[EI_NIDENT];\n\nI would say camelCase here too, but as this is a term that comes from\nthe Elf(32|64)_Ehdr structure, it can be an exception.\n\n> +\tret = read(fd, e_ident, EI_NIDENT);\n> +\tif (ret < 0)\n> +\t\treturn errno;\n\nThis should be return -errno if you want a negative error code.\n\n> +\n> +\tif (!(e_ident[EI_MAG0] == ELFMAG0 &&\n> +\t      e_ident[EI_MAG1] == ELFMAG1 &&\n> +\t      e_ident[EI_MAG2] == ELFMAG2 &&\n> +\t      e_ident[EI_MAG3] == ELFMAG3) ||\n> +\t    e_ident[EI_VERSION] != EV_CURRENT)\n\nWouldn't it be simplier to write\n\n\tif (e_ident[EI_MAG0] != ELFMAG0 ||\n\t    e_ident[EI_MAG1] != ELFMAG1 ||\n\t    e_ident[EI_MAG2] != ELFMAG2 ||\n\t    e_ident[EI_MAG3] != ELFMAG3 ||\n\t    e_ident[EI_VERSION] != EV_CURRENT)\n\n> +\t\treturn -1;\n\nPlease use explicit error codes. Maybe -ENOEXEC here ?\n\n> +\n> +\tif (e_ident[EI_CLASS] == ELFCLASS32 ||\n> +\t    e_ident[EI_CLASS] == ELFCLASS64)\n> +\t\tbitclass_ = e_ident[EI_CLASS];\n> +\telse\n> +\t\treturn -1;\n> +\n> +\tint a = 1;\n> +\tbool little_endian = *(char *)&a == 1;\n> +\tif (!(e_ident[EI_DATA] == ELFDATA2LSB ||\n> +\t      e_ident[EI_DATA] == ELFDATA2MSB) ||\n> +\t    !(e_ident[EI_DATA] == (little_endian ? ELFDATA2LSB : ELFDATA2MSB)))\n> +\t\treturn -1;\n\nThis seems a bit complicated.\n\n\tint a = 1;\n\tunsigned char endianness = *(char *)&a == 1 ? ELFDATA2LSB : ELFDATA2MSB;\n\tif (e_ident[EI_DATA] != endianness)\n\t\treturn -ENOEXEC;\n\nBut you should also use C++-style casts:\n\n\tint a = 1;\n\tunsigned char endianness = *static_cast<(char *>(&a) == 1\n\t\t\t\t ? ELFDATA2LSB : ELFDATA2MSB;\n\tif (e_ident[EI_DATA] != endianness)\n\t\treturn -ENOEXEC;\n\nTo avoid the cast, you could also write\n\n\t/* Verify that the ELF endianness matches the host's. */\n\tconst union {\n\t\tuint32_t i;\n\t\tuint8_t c[4];\n\t} e = { .c = { ELFDATA2LSB, 0, 0, ELFDATA2MSB } };\n\n\tif (e_ident[EI_DATA] != e.i & 0xff)\n\t\treturn -ENOEXEC;\n\n> +\n> +\treturn 0;\n> +}\n> +\n> +template<class elfHeader, class secHeader, class symHeader>\n\nClever use of templates :-) I'm not sure I would have thought about it.\n\nThe class names should use CamelCase.\n\n> +int LibLoader::loadSymbol(struct LibLoader::IPAModuleInfo *dst, int fd,\n> +\t\t\t  const std::string &symbol)\n\nIf you want to make this function generic as its name implies, it should\ntake a void pointer and a size for the destination memory:\n\n\tint fd, const char *symbol, void *data, size_t size\n\n(note that I've replaced std::string with a char *, as you only use\nsymbol.c_str() here, and the caller passes a hardcoded string literal,\nso there's no point in converting back and forth)\n\n> +{\n> +\tunsigned long soSize = lseek(fd, 0, SEEK_END);\n\nHow about using stat() instead ?\n\n> +\tunsigned char *map = (unsigned char *)mmap(NULL, soSize, PROT_READ,\n> +\t\t\t\t\t\t   MAP_PRIVATE, fd, 0);\n\nstatic_cast<unsigned char *>()\n\n(or char * instead of unsigned char * as you only use map to do pointer\narithmetics). I'm afraid the same applies to all other casts below.\n\n> +\tif (map == MAP_FAILED)\n> +\t\treturn errno;\n\n\treturn -errno;\n\n> +\n> +\telfHeader *eHdr = (elfHeader *)map;\n\nYou will have to be a bit more careful than that, and verify that the\nfile size is big enough, here and below, to avoid overflows in case of a\ncorrupt (or even maliciously crafted) ELF file.\n\nOne option would be to create a function (or macro) that takes map,\noffset, size and file size and return a pointer to map + offset if\noffset + size < file size (with an appropriate overflow check on offset\n+ size) and nullptr otherwise (behold the template magic):\n\ntemplate<typename T>\ntypename std::remove_extent<T>::type *elfPointer(void *map, off_t offset,\n\t\t\t\t\t\t size_t fileSize)\n{\n\tsize_t size = offset + sizeof(T);\n\tif (size > fileSize || size < sizeof(T))\n\t\treturn nullptr;\n\n\treturn reinterpret_cast<typename std::remove_extent<T>::type *>\n\t\t(static_cast<char *>(map) + offset);\n}\n\n...\n\n\telfHeader *eHdr = elfPointer<elfHeader>(map, 0, soSize);\n\tif (!eHdr)\n\t\treturn -ENOEXEC;\n\n\toff_t offset = eHdr->e_shoff + eHdr->e_shentsize * eHdr->e_shstrndx;\n\tsecHeader *sHdr = elfPointer<secHeader>(map, offset, soSize);\n\tif (!sHdr)\n\t\treturn -ENOEXEC;\n\n\toff_t shnameoff = sHdr->sh_offset;\n\n\t/* Locate the .dynsym section. */\n\tbool found = false;\n\tsecHeader *dynsym;\n\tfor (unsigned int i = 0; i < eHdr->e_shnum; i++) {\n\t\toffset = eHdr->e_shoff + eHdr->e_shentsize * i;\n\t\tsHdr = elfPointer<secHeader>(map, offset, soSize);\n\t\tif (!sHdr)\n\t\t\treturn -ENOEXEC;\n\n\t\tchar *name = elfPointer<char[8]>(map, shnameoff + sHdr->sh_name,\n\t\t\t\t\t\t soSize);\n\t\tif (!name)\n\t\t\treturn -ENOEXEC;\n\n\t\tif (sHdr->sh_type == SHT_DYNSYM && !strcmp(name, \".dynsym\")) {\n\t\t\tfound = true;\n\t\t\tdynsym = sHdr;\n\t\t\tbreak;\n\t\t}\n\t}\n\nand so on. Other options are possible too, but please care about both\ncore readability and safety.\n\n> +\n> +\tunsigned long shnameoff =\n> +\t\t((secHeader *)(map + eHdr->e_shoff +\n> +\t\t\t       eHdr->e_shentsize * eHdr->e_shstrndx))\n> +\t\t\t->sh_offset;\n> +\n> +\t// walk section header table, find .dynsym\n> +\tbool found = false;\n> +\tsecHeader *dynsym;\n> +\tfor (unsigned int i = 0; i < eHdr->e_shnum; i++) {\n> +\t\tunsigned long i_shoff = eHdr->e_shoff + eHdr->e_shentsize * i;\n> +\t\tsecHeader *sHdr = (secHeader *)(map + i_shoff);\n> +\n> +\t\tchar *name = (char *)map + shnameoff + sHdr->sh_name;\n> +\n> +\t\tif (sHdr->sh_type == SHT_DYNSYM && !strcmp(name, \".dynsym\")) {\n> +\t\t\tfound = true;\n> +\t\t\tdynsym = sHdr;\n> +\t\t\tbreak;\n> +\t\t}\n> +\t}\n> +\n> +\tif (!found)\n> +\t\treturn -1;\n> +\n> +\tunsigned long dynsym_nameoff =\n> +\t\t((secHeader *)(map + eHdr->e_shoff +\n> +\t\t\t       eHdr->e_shentsize * dynsym->sh_link))\n> +\t\t\t->sh_offset;\n> +\n> +\t// walk dynsym symbol table, find desired symbol\n> +\tfound = false;\n> +\tsymHeader *target_symbol;\n> +\tunsigned int dynsym_num = dynsym->sh_size / dynsym->sh_entsize;\n> +\tfor (unsigned int i = 0; i < dynsym_num; i++) {\n> +\t\tunsigned long i_symoff = dynsym->sh_offset + dynsym->sh_entsize * i;\n> +\t\tsymHeader *sym = (symHeader *)(map + i_symoff);\n> +\n> +\t\tchar *name = (char *)map + dynsym_nameoff + sym->st_name;\n> +\n> +\t\tif (!strcmp(name, symbol.c_str()) &&\n> +\t\t    sym->st_info & STB_GLOBAL &&\n> +\t\t    sym->st_size == sizeof(struct IPAModuleInfo)) {\n> +\t\t\tfound = true;\n> +\t\t\ttarget_symbol = sym;\n> +\t\t\tbreak;\n> +\t\t}\n> +\t}\n> +\n> +\tif (!found)\n> +\t\treturn -1;\n> +\n> +\tsecHeader *target_section =\n> +\t\t(secHeader *)(map + eHdr->e_shoff +\n> +\t\t\t      target_symbol->st_shndx * eHdr->e_shentsize);\n> +\tunsigned long final_addr = target_section->sh_offset +\n> +\t\t\t\t   (target_symbol->st_value - target_section->sh_addr);\n> +\tmemcpy(dst, map + final_addr, sizeof(struct IPAModuleInfo));\n\nmunmap() is missing, both in the success and error cases. You could move\nit to the caller to make error handling easier and pass the mapped\naddress and size instead of the fd. This would have the advantage of\nmaking the mapped .so available to loadElfIdent() too.\n\n> +\n> +\treturn 0;\n> +}\n> +\n> +int LibLoader::loadIPAModuleInfo(const std::string &symbol)\n> +{\n> +\tint fd = open(lib_path_.c_str(), O_RDONLY);\n> +\tif (fd < 0)\n> +\t\treturn fd;\n> +\n> +\tint ret = loadElfIdent(fd);\n> +\tif (ret)\n> +\t\tgoto close;\n> +\n> +\tif (bitclass_ == ELFCLASS32)\n> +\t\tret = loadSymbol<Elf32_Ehdr, Elf32_Shdr, Elf32_Sym>(&info_, fd, symbol);\n> +\telse if (bitclass_ == ELFCLASS64)\n> +\t\tret = loadSymbol<Elf64_Ehdr, Elf64_Shdr, Elf64_Sym>(&info_, fd, symbol);\n> +\telse\n> +\t\tret = -1;\n\nThis case is not possible as loadElfIdent() returns an error if the\nbitclass is not valid.\n\n> +\tif (ret)\n> +\t\tgoto close;\n> +\n> +\tloaded_ = true;\n> +\n> +close:\n> +\tclose(fd);\n> +\treturn ret;\n> +}\n> +\n> +bool LibLoader::isLoaded() const\n> +{\n> +\treturn loaded_;\n> +}\n\nI would turn this into an isValid() function that returns true if the\nELF file could be parsed successfully and the IPAModuleInfo loaded, and\nfalse otherwise.\n\n> +\n> +struct LibLoader::IPAModuleInfo LibLoader::getIPAModuleInfo() const\n> +{\n> +\treturn info_;\n> +}\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index 8796f49..4e7ab10 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -10,6 +10,7 @@ libcamera_sources = files([\n>      'event_notifier.cpp',\n>      'formats.cpp',\n>      'geometry.cpp',\n> +    'lib_loader.cpp',\n>      'log.cpp',\n>      'media_device.cpp',\n>      'media_object.cpp',\n> @@ -31,6 +32,7 @@ libcamera_headers = files([\n>      'include/device_enumerator_udev.h',\n>      'include/event_dispatcher_poll.h',\n>      'include/formats.h',\n> +    'include/lib_loader.h',\n>      'include/log.h',\n>      'include/media_device.h',\n>      'include/media_object.h',","headers":{"Return-Path":"<laurent.pinchart@ideasonboard.com>","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6AE3860E4F\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSat, 11 May 2019 19:48:54 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A3D0AD5;\n\tSat, 11 May 2019 19:48:53 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1557596933;\n\tbh=QAqTlQ/E65VVktZ0ZPf0rLLmtQq9JwA/Uh2LDer9wwY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=pB+GcAsFuygAg1+Z0uLLRpLmbMd79dKJS20O61TFI8i9KaEI3zMPxhRv721DIwX64\n\t7ZO+Ew4csQ91pcOhDYPR5gc6ePKCMQTz5eY9LEhq98o/Fae/uBIJQda3O88vp7TKH7\n\tsWAHpfe6JGdALAD2zbwxjProJediEZlL6V2Lrh9M=","Date":"Sat, 11 May 2019 20:48:37 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Paul Elder <paul.elder@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190511174837.GA13043@pendragon.ideasonboard.com>","References":"<20190510232235.8724-1-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190510232235.8724-1-paul.elder@ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: lib_loader: add shared\n\tlibrary loader","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Sat, 11 May 2019 17:48:54 -0000"}}]