[{"id":2090,"web_url":"https://patchwork.libcamera.org/comment/2090/","msgid":"<20190701174349.GM5018@pendragon.ideasonboard.com>","date":"2019-07-01T17:43:49","subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: ipa_module:\n\telfLoadSymbol find symbol regardless of size","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 Mon, Jul 01, 2019 at 11:13:36PM +0900, Paul Elder wrote:\n> Make elfLoadSymbol more generic by making the symbol size an output\n> rather than an input. Also move the memcpy out of elfLoadSymbol.\n> \n> If the size of struct IPAModuleInfo changes between versions, we still\n> want to be able to load it and perhaps do conversions for backwards\n> compatibility. In this case the size should not be a restriction when\n> searching for the symbol.\n> \n> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>\n> ---\n>  src/libcamera/ipa_module.cpp | 64 ++++++++++++++++++++++--------------\n>  1 file changed, 40 insertions(+), 24 deletions(-)\n> \n> diff --git a/src/libcamera/ipa_module.cpp b/src/libcamera/ipa_module.cpp\n> index 5a46ec3..d82ac69 100644\n> --- a/src/libcamera/ipa_module.cpp\n> +++ b/src/libcamera/ipa_module.cpp\n> @@ -17,6 +17,8 @@\n>  #include <sys/types.h>\n>  #include <unistd.h>\n>  \n> +#include <tuple>\n> +\n\nNo need to separate this from the previous headers, it can fit between\nsys/types.h and unistd.h.\n\n>  #include \"log.h\"\n>  #include \"pipeline_handler.h\"\n>  \n> @@ -81,18 +83,27 @@ int elfVerifyIdent(void *map, size_t soSize)\n>  \treturn 0;\n>  }\n>  \n> +/**\n> + * \\brief Retrieve address and size of a symbol from an mmap'ed ELF file\n> + * \\param[in] map Address of mmap'ed ELF file\n> + * \\param[in] soSize Size of mmap'ed ELF file (in bytes)\n> + * \\param[in] symbol Symbol name\n> + *\n> + * \\return zero or error code, address or nullptr, size of symbol or zero,\n> + * respectively\n> + */\n>  template<class ElfHeader, class SecHeader, class SymHeader>\n> -int elfLoadSymbol(void *dst, size_t size, void *map, size_t soSize,\n> -\t\t\t  const char *symbol)\n> +std::tuple<int, void *, size_t>\n> +elfLoadSymbol(void *map, size_t soSize, const char *symbol)\n>  {\n>  \tElfHeader *eHdr = elfPointer<ElfHeader>(map, 0, soSize);\n>  \tif (!eHdr)\n> -\t\treturn -ENOEXEC;\n> +\t\treturn std::make_tuple(-ENOEXEC, nullptr, 0);\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> +\t\treturn std::make_tuple(-ENOEXEC, nullptr, 0);\n>  \toff_t shnameoff = sHdr->sh_offset;\n>  \n>  \t/* Locate .dynsym section header. */\n> @@ -101,12 +112,12 @@ int elfLoadSymbol(void *dst, size_t size, void *map, size_t soSize,\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> +\t\t\treturn std::make_tuple(-ENOEXEC, nullptr, 0);\n>  \n>  \t\toffset = shnameoff + sHdr->sh_name;\n>  \t\tchar *name = elfPointer<char[8]>(map, offset, soSize);\n>  \t\tif (!name)\n> -\t\t\treturn -ENOEXEC;\n> +\t\t\treturn std::make_tuple(-ENOEXEC, nullptr, 0);\n>  \n>  \t\tif (sHdr->sh_type == SHT_DYNSYM && !strcmp(name, \".dynsym\")) {\n>  \t\t\tdynsym = sHdr;\n> @@ -116,13 +127,13 @@ int elfLoadSymbol(void *dst, size_t size, void *map, size_t soSize,\n>  \n>  \tif (dynsym == nullptr) {\n>  \t\tLOG(IPAModule, Error) << \"ELF has no .dynsym section\";\n> -\t\treturn -ENOEXEC;\n> +\t\treturn std::make_tuple(-ENOEXEC, nullptr, 0);\n>  \t}\n>  \n>  \toffset = eHdr->e_shoff + eHdr->e_shentsize * dynsym->sh_link;\n>  \tsHdr = elfPointer<SecHeader>(map, offset, soSize);\n>  \tif (!sHdr)\n> -\t\treturn -ENOEXEC;\n> +\t\treturn std::make_tuple(-ENOEXEC, nullptr, 0);\n>  \toff_t dynsym_nameoff = sHdr->sh_offset;\n>  \n>  \t/* Locate symbol in the .dynsym section. */\n> @@ -132,16 +143,16 @@ int elfLoadSymbol(void *dst, size_t size, void *map, size_t soSize,\n>  \t\toffset = dynsym->sh_offset + dynsym->sh_entsize * i;\n>  \t\tSymHeader *sym = elfPointer<SymHeader>(map, offset, soSize);\n>  \t\tif (!sym)\n> -\t\t\treturn -ENOEXEC;\n> +\t\t\treturn std::make_tuple(-ENOEXEC, nullptr, 0);\n>  \n>  \t\toffset = dynsym_nameoff + sym->st_name;\n>  \t\tchar *name = elfPointer<char>(map, offset, soSize,\n>  \t\t\t\t\t      strlen(symbol) + 1);\n>  \t\tif (!name)\n> -\t\t\treturn -ENOEXEC;\n> +\t\t\treturn std::make_tuple(-ENOEXEC, nullptr, 0);\n>  \n>  \t\tif (!strcmp(name, symbol) &&\n> -\t\t    sym->st_info & STB_GLOBAL && sym->st_size == size) {\n> +\t\t    sym->st_info & STB_GLOBAL) {\n>  \t\t\ttargetSymbol = sym;\n>  \t\t\tbreak;\n>  \t\t}\n> @@ -149,24 +160,22 @@ int elfLoadSymbol(void *dst, size_t size, void *map, size_t soSize,\n>  \n>  \tif (targetSymbol == nullptr) {\n>  \t\tLOG(IPAModule, Error) << \"Symbol \" << symbol << \" not found\";\n> -\t\treturn -ENOEXEC;\n> +\t\treturn std::make_tuple(-ENOEXEC, nullptr, 0);\n>  \t}\n>  \n>  \t/* Locate and return data of symbol. */\n>  \tif (targetSymbol->st_shndx >= eHdr->e_shnum)\n> -\t\treturn -ENOEXEC;\n> +\t\treturn std::make_tuple(-ENOEXEC, nullptr, 0);\n>  \toffset = eHdr->e_shoff + targetSymbol->st_shndx * eHdr->e_shentsize;\n>  \tsHdr = elfPointer<SecHeader>(map, offset, soSize);\n>  \tif (!sHdr)\n> -\t\treturn -ENOEXEC;\n> +\t\treturn std::make_tuple(-ENOEXEC, nullptr, 0);\n>  \toffset = sHdr->sh_offset + (targetSymbol->st_value - sHdr->sh_addr);\n> -\tchar *data = elfPointer<char>(map, offset, soSize, size);\n> +\tchar *data = elfPointer<char>(map, offset, soSize, targetSymbol->st_size);\n>  \tif (!data)\n> -\t\treturn -ENOEXEC;\n> -\n> -\tmemcpy(dst, data, size);\n> +\t\treturn std::make_tuple(-ENOEXEC, nullptr, 0);\n>  \n> -\treturn 0;\n> +\treturn std::make_tuple(0, data, targetSymbol->st_size);\n\nAs the only error code that is returned is -ENOEXEC, would it make sense\nto drop it and base the error check on the address being nullptr ? I\ndon't really mind either way, so with or without the change (but with\nthe include issue fixed),\n\nReviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n\n>  }\n>  \n>  } /* namespace */\n> @@ -257,8 +266,10 @@ int IPAModule::loadIPAModuleInfo()\n>  \t\treturn ret;\n>  \t}\n>  \n> -\tsize_t soSize;\n> +\tvoid *data;\n> +\tsize_t dataSize;\n>  \tvoid *map;\n> +\tsize_t soSize;\n>  \tstruct stat st;\n>  \tint ret = fstat(fd, &st);\n>  \tif (ret < 0)\n> @@ -275,11 +286,16 @@ int IPAModule::loadIPAModuleInfo()\n>  \t\tgoto unmap;\n>  \n>  \tif (sizeof(unsigned long) == 4)\n> -\t\tret = elfLoadSymbol<Elf32_Ehdr, Elf32_Shdr, Elf32_Sym>\n> -\t\t\t\t   (&info_, sizeof(info_), map, soSize, \"ipaModuleInfo\");\n> +\t\tstd::tie(ret, data, dataSize) =\n> +\t\t\telfLoadSymbol<Elf32_Ehdr, Elf32_Shdr, Elf32_Sym>\n> +\t\t\t\t     (map, soSize, \"ipaModuleInfo\");\n>  \telse\n> -\t\tret = elfLoadSymbol<Elf64_Ehdr, Elf64_Shdr, Elf64_Sym>\n> -\t\t\t\t   (&info_, sizeof(info_), map, soSize, \"ipaModuleInfo\");\n> +\t\tstd::tie(ret, data, dataSize) =\n> +\t\t\telfLoadSymbol<Elf64_Ehdr, Elf64_Shdr, Elf64_Sym>\n> +\t\t\t\t     (map, soSize, \"ipaModuleInfo\");\n> +\n> +\tif (!ret && dataSize == sizeof(info_))\n> +\t\tmemcpy(&info_, data, dataSize);\n>  \n>  \tif (info_.moduleAPIVersion != IPA_MODULE_API_VERSION) {\n>  \t\tLOG(IPAModule, Error) << \"IPA module API version mismatch\";","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 1E13A60BF8\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon,  1 Jul 2019 19:44:09 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(dfj612yhrgyx302h3jwwy-3.rev.dnainternet.fi\n\t[IPv6:2001:14ba:21f5:5b00:ce28:277f:58d7:3ca4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A76DE524;\n\tMon,  1 Jul 2019 19:44:08 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1562003048;\n\tbh=mMHcd0oaUPItiP5Cxy3Pj+MDxgTto5wKxg5wmqM2e1I=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=iFTxWlhUhlfVfopv/sD4ZoxvbacNOAYtGaqvq9B9hpZCHLUd/Uj6Sf/fi6BaVC3hv\n\t0sdaTLe1sOuDrWeZhiPNjGprOGJC/86TaSqvS/NjO6FsDo7Tz/TvIbKv8bHuyOZLG7\n\t3hiS50BubYBa7BDBmF3pIud3Z2VOp7Szr5Vqxvhg=","Date":"Mon, 1 Jul 2019 20:43:49 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Paul Elder <paul.elder@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190701174349.GM5018@pendragon.ideasonboard.com>","References":"<20190701141336.10273-1-paul.elder@ideasonboard.com>\n\t<20190701141336.10273-2-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190701141336.10273-2-paul.elder@ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: ipa_module:\n\telfLoadSymbol find symbol regardless of size","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":"Mon, 01 Jul 2019 17:44:09 -0000"}}]